Bug 1319702 - Fix races in the CrashManager and CrashSubmit objects and in the related tests r=bsmedberg
authorGabriele Svelto <gsvelto@mozilla.com>
Wed, 23 Nov 2016 11:28:04 +0100
changeset 372510 5d8855350a728b9577171a8d12e2a3dce13871a2
parent 372509 e589f801d2211b67449475b1b8ad82cc937ca1a0
child 372511 bd9e81439725f3d4135652cc3d65f2bfba527b7b
child 372520 e54021050e77f9672496192fb6b5a67fed641619
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg
bugs1319702
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1319702 - Fix races in the CrashManager and CrashSubmit objects and in the related tests r=bsmedberg
dom/plugins/test/mochitest/test_crash_submit.xul
dom/plugins/test/mochitest/test_hang_submit.xul
toolkit/components/crashes/CrashManager.jsm
toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
toolkit/crashreporter/CrashSubmit.jsm
--- a/dom/plugins/test/mochitest/test_crash_submit.xul
+++ b/dom/plugins/test/mochitest/test_crash_submit.xul
@@ -76,16 +76,17 @@ var testObserver = {
     env.set("MOZ_CRASHREPORTER_NO_REPORT", "1");
 
     // Finally re-set crashreporter URL
     crashReporter.serverURL = oldServerURL;
 
     // Check and cleanup CrashManager.
     Task.spawn(function* () {
       let cm = Services.crashmanager;
+      yield cm.ensureCrashIsPresent(crashID);
       let store = yield cm._getStore();
       is(store.crashesCount, 1, "Store should have only 1 item");
 
       let crash = store.getCrash(crashID);
       ok(!!crash, "Store should have the crash record");
       ok(crash.isOfType(cm.PROCESS_TYPE_PLUGIN, cm.CRASH_TYPE_CRASH),
          "Crash type should be plugin-crash");
       is(crash.remoteID, remoteID, "Crash remoteID should match");
--- a/dom/plugins/test/mochitest/test_hang_submit.xul
+++ b/dom/plugins/test/mochitest/test_hang_submit.xul
@@ -84,16 +84,17 @@ var testObserver = {
 
     // Finally re-set prefs
     crashReporter.serverURL = oldServerURL;
     Services.prefs.setIntPref("dom.ipc.plugins.timeoutSecs", oldTimeoutPref);
 
     // Check and cleanup CrashManager.
     Task.spawn(function* () {
       let cm = Services.crashmanager;
+      yield cm.ensureCrashIsPresent(crashID);
       let store = yield cm._getStore();
       is(store.crashesCount, 1, "Store should have only 1 item");
 
       let crash = store.getCrash(crashID);
       ok(!!crash, "Store should have the crash record");
       ok(crash.isOfType(cm.PROCESS_TYPE_PLUGIN, cm.CRASH_TYPE_HANG),
          "Crash type should be plugin-hang");
       is(crash.remoteID, remoteID, "Crash remoteID should match");
--- a/toolkit/components/crashes/CrashManager.jsm
+++ b/toolkit/components/crashes/CrashManager.jsm
@@ -2,25 +2,25 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 const myScope = this;
 
+Cu.import("resource://gre/modules/KeyValueParser.jsm");
 Cu.import("resource://gre/modules/Log.jsm", this);
 Cu.import("resource://gre/modules/osfile.jsm", this);
-Cu.import("resource://gre/modules/Promise.jsm", this);
+Cu.import("resource://gre/modules/PromiseUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm", this);
 Cu.import("resource://gre/modules/Task.jsm", this);
+Cu.import("resource://gre/modules/TelemetryController.jsm");
 Cu.import("resource://gre/modules/Timer.jsm", this);
 Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
-Cu.import("resource://gre/modules/TelemetryController.jsm");
-Cu.import("resource://gre/modules/KeyValueParser.jsm");
 
 this.EXPORTED_SYMBOLS = [
   "CrashManager",
 ];
 
 /**
  * How long to wait after application startup before crash event files are
  * automatically aggregated.
@@ -106,16 +106,19 @@ this.CrashManager = function(options) {
         throw new Error("Unknown property in options: " + k);
     }
   }
 
   // Promise for in-progress aggregation operation. We store it on the
   // object so it can be returned for in-progress operations.
   this._aggregatePromise = null;
 
+  // Map of crash ID / promise tuples used to track adding new crashes.
+  this._crashPromises = new Map();
+
   // The CrashStore currently attached to this object.
   this._store = null;
 
   // A Task to retrieve the store. This is needed to avoid races when
   // _getStore() is called multiple times in a short interval.
   this._getStoreTask = null;
 
   // The timer controlling the expiration of the CrashStore instance.
@@ -340,17 +343,17 @@ this.CrashManager.prototype = Object.fre
 
   /**
    * Schedule maintenance tasks for some point in the future.
    *
    * @param delay
    *        (integer) Delay in milliseconds when maintenance should occur.
    */
   scheduleMaintenance: function(delay) {
-    let deferred = Promise.defer();
+    let deferred = PromiseUtils.defer();
 
     setTimeout(() => {
       this.runMaintenanceTasks().then(deferred.resolve, deferred.reject);
     }, delay);
 
     return deferred.promise;
   },
 
@@ -364,25 +367,56 @@ this.CrashManager.prototype = Object.fre
    * @param crashType (string) One of the CRASH_TYPE constants.
    * @param id (string) Crash ID. Likely a UUID.
    * @param date (Date) When the crash occurred.
    * @param metadata (dictionary) Crash metadata, may be empty.
    *
    * @return promise<null> Resolved when the store has been saved.
    */
   addCrash: function(processType, crashType, id, date, metadata) {
-    return Task.spawn(function* () {
+    let promise = Task.spawn(function* () {
       let store = yield this._getStore();
       if (store.addCrash(processType, crashType, id, date, metadata)) {
         yield store.save();
       }
+
+      let deferred = this._crashPromises.get(id);
+
+      if (deferred) {
+        this._crashPromises.delete(id);
+        deferred.resolve();
+      }
     }.bind(this));
+
+    return promise;
   },
 
   /**
+   * Returns a promise that is resolved only the crash with the specified id
+   * has been fully recorded.
+   *
+   * @param id (string) Crash ID. Likely a UUID.
+   *
+   * @return promise<null> Resolved when the crash is present.
+   */
+  ensureCrashIsPresent: Task.async(function* (id) {
+    let store = yield this._getStore();
+    let crash = store.getCrash(id);
+
+    if (crash) {
+      return Promise.resolve();
+    }
+
+    let deferred = PromiseUtils.defer();
+
+    this._crashPromises.set(id, deferred);
+    return deferred.promise;
+  }),
+
+  /**
    * Record the remote ID for a crash.
    *
    * @param crashID (string) Crash ID. Likely a UUID.
    * @param remoteID (Date) Server/Breakpad ID.
    *
    * @return boolean True if the remote ID was recorded.
    */
   setRemoteCrashID: Task.async(function* (crashID, remoteID) {
--- a/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
+++ b/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
@@ -469,16 +469,46 @@ add_task(function* test_addSubmissionAtt
 
   let submission = submissions.get("submission");
   Assert.ok(!!submission);
   Assert.equal(submission.requestDate.getTime(), DUMMY_DATE.getTime());
   Assert.equal(submission.responseDate.getTime(), DUMMY_DATE_2.getTime());
   Assert.equal(submission.result, m.SUBMISSION_RESULT_OK);
 });
 
+add_task(function* test_addSubmissionAttemptEarlyCall() {
+  let m = yield getManager();
+
+  let crashes = yield m.getCrashes();
+  Assert.equal(crashes.length, 0);
+
+  let p = m.ensureCrashIsPresent("main-crash").then(() => {
+    return m.addSubmissionAttempt("main-crash", "submission", DUMMY_DATE);
+  }).then(() => {
+    return m.addSubmissionResult("main-crash", "submission", DUMMY_DATE_2,
+                                 m.SUBMISSION_RESULT_OK);
+  });
+
+  yield m.addCrash(m.PROCESS_TYPE_MAIN, m.CRASH_TYPE_CRASH,
+                   "main-crash", DUMMY_DATE);
+
+  crashes = yield m.getCrashes();
+  Assert.equal(crashes.length, 1);
+
+  yield p;
+  let submissions = crashes[0].submissions;
+  Assert.ok(!!submissions);
+
+  let submission = submissions.get("submission");
+  Assert.ok(!!submission);
+  Assert.equal(submission.requestDate.getTime(), DUMMY_DATE.getTime());
+  Assert.equal(submission.responseDate.getTime(), DUMMY_DATE_2.getTime());
+  Assert.equal(submission.result, m.SUBMISSION_RESULT_OK);
+});
+
 add_task(function* test_setCrashClassifications() {
   let m = yield getManager();
 
   yield m.addCrash(m.PROCESS_TYPE_MAIN, m.CRASH_TYPE_CRASH,
                    "main-crash", DUMMY_DATE);
   yield m.setCrashClassifications("main-crash", ["a"]);
   let classifications = (yield m.getCrashes())[0].classifications;
   Assert.ok(classifications.indexOf("a") != -1);
--- a/toolkit/crashreporter/CrashSubmit.jsm
+++ b/toolkit/crashreporter/CrashSubmit.jsm
@@ -303,41 +303,48 @@ Submitter.prototype = {
     let manager = Services.crashmanager;
     let submissionID = manager.generateSubmissionID();
 
     xhr.addEventListener("readystatechange", (evt) => {
       if (xhr.readyState == 4) {
         let ret =
           xhr.status == 200 ? parseKeyValuePairs(xhr.responseText) : {};
         let submitted = !!ret.CrashID;
+        let p = Promise.resolve();
 
         if (this.recordSubmission) {
           let result = submitted ? manager.SUBMISSION_RESULT_OK :
                                    manager.SUBMISSION_RESULT_FAILED;
-          manager.addSubmissionResult(this.id, submissionID, new Date(),
-                                      result);
+          p = manager.addSubmissionResult(this.id, submissionID, new Date(),
+                                          result);
           if (submitted) {
             manager.setRemoteCrashID(this.id, ret.CrashID);
           }
         }
 
-        if (submitted) {
-          this.submitSuccess(ret);
-        }
-        else {
-           this.notifyStatus(FAILED);
-           this.cleanup();
-        }
+        p.then(() => {
+          if (submitted) {
+            this.submitSuccess(ret);
+          } else {
+            this.notifyStatus(FAILED);
+            this.cleanup();
+          }
+        });
       }
     }, false);
 
+    let p = Promise.resolve();
+    let id = this.id;
+
     if (this.recordSubmission) {
-      manager.addSubmissionAttempt(this.id, submissionID, new Date());
+      p = manager.ensureCrashIsPresent(id).then(() => {
+        return manager.addSubmissionAttempt(id, submissionID, new Date());
+      });
     }
-    xhr.send(formData);
+    p.then(() => { xhr.send(formData); });
     return true;
   },
 
   notifyStatus: function Submitter_notify(status, ret)
   {
     let propBag = Cc["@mozilla.org/hash-property-bag;1"].
                   createInstance(Ci.nsIWritablePropertyBag2);
     propBag.setPropertyAsAString("minidumpID", this.id);