Bug 973579 - TelemetryFile should fail silently when not overwriting an existing file. r=Yoric
authorRoberto A. Vitillo <rvitillo@mozilla.com>
Tue, 18 Feb 2014 11:26:11 +0000
changeset 169701 5f87786ceab5a4aa3f8a6791a17b7b856929c756
parent 169700 6ea78f586fdcf27132ac33bdeeff87eea788d32c
child 169702 2f13d537fd49d5703eb9e89b0446b61471c321d1
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewersYoric
bugs973579
milestone30.0a1
Bug 973579 - TelemetryFile should fail silently when not overwriting an existing file. r=Yoric
toolkit/components/telemetry/TelemetryFile.jsm
toolkit/components/telemetry/TelemetryPing.jsm
toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
--- a/toolkit/components/telemetry/TelemetryFile.jsm
+++ b/toolkit/components/telemetry/TelemetryFile.jsm
@@ -59,50 +59,58 @@ this.TelemetryFile = {
     return OS.Path.join(OS.Constants.Path.profileDir, "saved-telemetry-pings");
   },
 
   /**
    * Save a single ping to a file.
    *
    * @param {object} ping The content of the ping to save.
    * @param {string} file The destination file.
-   * @param {bool} overwrite If |true|, the file will be overwritten
-   * if it exists.
+   * @param {bool} overwrite If |true|, the file will be overwritten if it exists,
+   * if |false| the file will not be overwritten and no error will be reported if
+   * the file exists.
    * @returns {promise}
    */
   savePingToFile: function(ping, file, overwrite) {
-    let pingString = JSON.stringify(ping);
-    return OS.File.writeAtomic(file, pingString, {tmpPath: file + ".tmp",
-                                noOverwrite: !overwrite});
+    return Task.spawn(function*() {
+      try {
+        let pingString = JSON.stringify(ping);
+        yield OS.File.writeAtomic(file, pingString, {tmpPath: file + ".tmp",
+                                  noOverwrite: !overwrite});
+      } catch(e if e.becauseExists) {
+      }
+    })
   },
 
   /**
    * Save a ping to its file.
    *
    * @param {object} ping The content of the ping to save.
    * @param {bool} overwrite If |true|, the file will be overwritten
    * if it exists.
    * @returns {promise}
    */
   savePing: function(ping, overwrite) {
     return Task.spawn(function*() {
       yield getPingDirectory();
       let file = pingFilePath(ping);
-      return this.savePingToFile(ping, file, overwrite);
+      yield this.savePingToFile(ping, file, overwrite);
     }.bind(this));
   },
 
   /**
    * Save all pending pings.
    *
    * @param {object} sessionPing The additional session ping.
    * @returns {promise}
    */
   savePendingPings: function(sessionPing) {
     let p = pendingPings.reduce((p, ping) => {
+      // Restore the files with the previous pings if for some reason they have
+      // been deleted, don't overwrite them otherwise.
       p.push(this.savePing(ping, false));
       return p;}, [this.savePing(sessionPing, true)]);
 
     pendingPings = [];
     return Promise.all(p);
   },
 
   /**
--- a/toolkit/components/telemetry/TelemetryPing.jsm
+++ b/toolkit/components/telemetry/TelemetryPing.jsm
@@ -784,23 +784,23 @@ let Impl = {
     request.open("POST", url, true);
     request.overrideMimeType("text/plain");
     request.setRequestHeader("Content-Type", "application/json; charset=UTF-8");
 
     let startTime = new Date();
 
     function handler(success) {
       return function(event) {
-        this.finishPingRequest(success, startTime, ping);
-
-        if (success) {
-          deferred.resolve();
-        } else {
-          deferred.reject(event);
-        }
+        this.finishPingRequest(success, startTime, ping).then(() => {
+          if (success) {
+            deferred.resolve();
+          } else {
+            deferred.reject(event);
+          }
+        });
       };
     }
     request.addEventListener("error", handler(false).bind(this), false);
     request.addEventListener("load", handler(true).bind(this), false);
 
     request.setRequestHeader("Content-Encoding", "gzip");
     let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
                     .createInstance(Ci.nsIScriptableUnicodeConverter);
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ -13,16 +13,17 @@ const Ci = Components.interfaces;
 const Cu = Components.utils;
 const Cr = Components.results;
 
 Cu.import("resource://testing-common/httpd.js", this);
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/LightweightThemeManager.jsm", this);
 Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
 Cu.import("resource://gre/modules/TelemetryPing.jsm", this);
+Cu.import("resource://gre/modules/TelemetryFile.jsm", this);
 Cu.import("resource://gre/modules/Task.jsm", this);
 Cu.import("resource://gre/modules/Promise.jsm", this);
 
 const IGNORE_HISTOGRAM = "test::ignore_me";
 const IGNORE_HISTOGRAM_TO_CLONE = "MEMORY_HEAP_ALLOCATED";
 const IGNORE_CLONED_HISTOGRAM = "test::ignore_me_also";
 const ADDON_NAME = "Telemetry test addon";
 const ADDON_HISTOGRAM = "addon-histogram";
@@ -394,16 +395,24 @@ function actualTest() {
   LightweightThemeManager.currentTheme = dummyTheme("1234");
 
   // fake plugin host for consistent flash version data
   registerFakePluginHost();
 
   run_next_test();
 }
 
+// Ensure that not overwriting an existing file fails silently
+add_task(function* test_overwritePing() {
+  let ping = {slug: "foo"}
+  yield TelemetryFile.savePing(ping, true);
+  yield TelemetryFile.savePing(ping, false);
+  yield TelemetryFile.cleanupPingFile(ping);
+});
+
 // Ensures that expired histograms are not part of the payload.
 add_task(function* test_expiredHistogram() {
   let histogram_id = "FOOBAR";
   let dummy = Telemetry.newHistogram(histogram_id, "30", 1, 2, 3, Telemetry.HISTOGRAM_EXPONENTIAL);
 
   dummy.add(1);
 
   do_check_eq(TelemetryPing.getPayload()["histograms"][histogram_id], undefined);