Bug 1189647 - Rename synchronize to unsafeSynchronize to encourage callers to think twice; r=jsantell
authorNick Fitzgerald <fitzgen@gmail.com>
Fri, 31 Jul 2015 17:22:28 -0700
changeset 287384 d4446e517b3aef8b3766882ceca912fa0e1b7c33
parent 287383 466ad45c683129be0274da16a505e6245293ab31
child 287385 de94f408a90191eaf57a1e3266f0f705a9b7be45
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjsantell
bugs1189647
milestone42.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 1189647 - Rename synchronize to unsafeSynchronize to encourage callers to think twice; r=jsantell
toolkit/devtools/server/actors/script.js
toolkit/devtools/server/actors/webbrowser.js
toolkit/devtools/server/tests/unit/test_nesting-01.js
toolkit/devtools/server/tests/unit/test_nesting-02.js
--- a/toolkit/devtools/server/actors/script.js
+++ b/toolkit/devtools/server/actors/script.js
@@ -757,32 +757,32 @@ ThreadActor.prototype = {
       error: "notImplemented",
       message: "forced completion is not yet implemented."
     };
   },
 
   _makeOnEnterFrame: function ({ pauseAndRespond }) {
     return aFrame => {
       const generatedLocation = this.sources.getFrameLocation(aFrame);
-      let { originalSourceActor } = this.synchronize(this.sources.getOriginalLocation(
+      let { originalSourceActor } = this.unsafeSynchronize(this.sources.getOriginalLocation(
         generatedLocation));
       let url = originalSourceActor.url;
 
       return this.sources.isBlackBoxed(url)
         ? undefined
         : pauseAndRespond(aFrame);
     };
   },
 
   _makeOnPop: function ({ thread, pauseAndRespond, createValueGrip }) {
     return function (aCompletion) {
       // onPop is called with 'this' set to the current frame.
 
       const generatedLocation = thread.sources.getFrameLocation(this);
-      const { originalSourceActor } = thread.synchronize(thread.sources.getOriginalLocation(
+      const { originalSourceActor } = thread.unsafeSynchronize(thread.sources.getOriginalLocation(
         generatedLocation));
       const url = originalSourceActor.url;
 
       if (thread.sources.isBlackBoxed(url)) {
         return undefined;
       }
 
       // Note that we're popping this frame; we need to watch for
@@ -814,17 +814,17 @@ ThreadActor.prototype = {
       };
     }
 
     // Otherwise take what a "step" means into consideration.
     return function () {
       // onStep is called with 'this' set to the current frame.
 
       const generatedLocation = thread.sources.getFrameLocation(this);
-      const newLocation = thread.synchronize(thread.sources.getOriginalLocation(
+      const newLocation = thread.unsafeSynchronize(thread.sources.getOriginalLocation(
         generatedLocation));
 
       // Cases when we should pause because we have executed enough to consider
       // a "step" to have occured:
       //
       // 1.1. We change frames.
       // 1.2. We change URLs (can happen without changing frames thanks to
       //      source mapping).
@@ -1036,32 +1036,36 @@ ThreadActor.prototype = {
         // packet.
         : error;
     });
   },
 
   /**
    * Spin up a nested event loop so we can synchronously resolve a promise.
    *
+   * DON'T USE THIS UNLESS YOU ABSOLUTELY MUST! Nested event loops suck: the
+   * world's state can change out from underneath your feet because JS is no
+   * longer run-to-completion.
+   *
    * @param aPromise
    *        The promise we want to resolve.
    * @returns The promise's resolution.
    */
-  synchronize: function(aPromise) {
+  unsafeSynchronize: function(aPromise) {
     let needNest = true;
     let eventLoop;
     let returnVal;
 
     aPromise
       .then((aResolvedVal) => {
         needNest = false;
         returnVal = aResolvedVal;
       })
       .then(null, (aError) => {
-        reportError(aError, "Error inside synchronize:");
+        reportError(aError, "Error inside unsafeSynchronize:");
       })
       .then(() => {
         if (eventLoop) {
           eventLoop.resolve();
         }
       });
 
     if (needNest) {
@@ -1781,17 +1785,17 @@ ThreadActor.prototype = {
    *
    * @param aFrame Debugger.Frame
    *        The stack frame that contained the debugger statement.
    */
   onDebuggerStatement: function (aFrame) {
     // Don't pause if we are currently stepping (in or over) or the frame is
     // black-boxed.
     const generatedLocation = this.sources.getFrameLocation(aFrame);
-    const { originalSourceActor } = this.synchronize(this.sources.getOriginalLocation(
+    const { originalSourceActor } = this.unsafeSynchronize(this.sources.getOriginalLocation(
       generatedLocation));
     const url = originalSourceActor ? originalSourceActor.url : null;
 
     return this.sources.isBlackBoxed(url) || aFrame.onStep
       ? undefined
       : this._pauseAndRespond(aFrame, { type: "debuggerStatement" });
   },
 
@@ -1813,17 +1817,17 @@ ThreadActor.prototype = {
       }
     }
 
     if (willBeCaught && this._options.ignoreCaughtExceptions) {
       return undefined;
     }
 
     const generatedLocation = this.sources.getFrameLocation(aFrame);
-    const { sourceActor } = this.synchronize(this.sources.getOriginalLocation(
+    const { sourceActor } = this.unsafeSynchronize(this.sources.getOriginalLocation(
       generatedLocation));
     const url = sourceActor ? sourceActor.url : null;
 
     if (this.sources.isBlackBoxed(url)) {
       return undefined;
     }
 
     try {
@@ -1900,22 +1904,22 @@ ThreadActor.prototype = {
     }
 
     let sourceActor = this.sources.createNonSourceMappedActor(aSource);
 
     // Go ahead and establish the source actors for this script, which
     // fetches sourcemaps if available and sends onNewSource
     // notifications.
     //
-    // We need to use synchronize here because if the page is being reloaded,
+    // We need to use unsafeSynchronize here because if the page is being reloaded,
     // this call will replace the previous set of source actors for this source
     // with a new one. If the source actors have not been replaced by the time
     // we try to reset the breakpoints below, their location objects will still
     // point to the old set of source actors, which point to different scripts.
-    this.synchronize(this.sources.createSourceActors(aSource));
+    this.unsafeSynchronize(this.sources.createSourceActors(aSource));
 
     // Set any stored breakpoints.
     let promises = [];
 
     for (let _actor of this.breakpointActorMap.findActors()) {
       // XXX bug 1142115: We do async work in here, so we need to
       // create a fresh binding because for/of does not yet do that in
       // SpiderMonkey
@@ -1933,17 +1937,17 @@ ThreadActor.prototype = {
               generatedLocations
             );
           }
         }));
       }
     }
 
     if (promises.length > 0) {
-      this.synchronize(promise.all(promises));
+      this.unsafeSynchronize(promise.all(promises));
     }
 
     return true;
   },
 
 
   /**
    * Get prototypes and properties of multiple objects.
@@ -3270,17 +3274,17 @@ BreakpointActor.prototype = {
    *
    * @param aFrame Debugger.Frame
    *        The stack frame that contained the breakpoint.
    */
   hit: function (aFrame) {
     // Don't pause if we are currently stepping (in or over) or the frame is
     // black-boxed.
     let generatedLocation = this.threadActor.sources.getFrameLocation(aFrame);
-    let { originalSourceActor } = this.threadActor.synchronize(
+    let { originalSourceActor } = this.threadActor.unsafeSynchronize(
       this.threadActor.sources.getOriginalLocation(generatedLocation));
     let url = originalSourceActor.url;
 
     if (this.threadActor.sources.isBlackBoxed(url)
         || aFrame.onStep) {
       return undefined;
     }
 
--- a/toolkit/devtools/server/actors/webbrowser.js
+++ b/toolkit/devtools/server/actors/webbrowser.js
@@ -1704,17 +1704,17 @@ TabActor.prototype = {
       return;
     }
 
     // Proceed normally only if the debuggee is not paused.
     // TODO bug 997119: move that code to ThreadActor by listening to will-navigate
     let threadActor = this.threadActor;
     if (request && threadActor.state == "paused") {
       request.suspend();
-      this.conn.send(threadActor.synchronize(Promise.resolve(threadActor.onResume())));
+      this.conn.send(threadActor.unsafeSynchronize(Promise.resolve(threadActor.onResume())));
       threadActor.dbg.enabled = false;
       this._pendingNavigation = request;
     }
     threadActor.disableAllBreakpoints();
 
     this.conn.send({
       from: this.actorID,
       type: "tabNavigated",
--- a/toolkit/devtools/server/tests/unit/test_nesting-01.js
+++ b/toolkit/devtools/server/tests/unit/test_nesting-01.js
@@ -1,14 +1,14 @@
 /* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // Test that we can nest event loops when needed in
-// ThreadActor.prototype.synchronize.
+// ThreadActor.prototype.unsafeSynchronize.
 
 var gClient;
 var gThreadActor;
 
 function run_test() {
   initTestDebuggerServer();
   let gDebuggee = addTestGlobal("test-nesting");
   gClient = new DebuggerClient(DebuggerServer.connectPipe());
@@ -27,22 +27,22 @@ function test_nesting() {
   const thread = gThreadActor;
   const { resolve, reject, promise: p } = promise.defer();
 
   let currentStep = 0;
 
   executeSoon(function () {
     // Should be on the first step
     do_check_eq(++currentStep, 1);
-    // We should have one nested event loop from synchronize
+    // We should have one nested event loop from unsfeSynchronize
     do_check_eq(thread._nestedEventLoops.size, 1);
     resolve(true);
   });
 
-  do_check_eq(thread.synchronize(p), true);
+  do_check_eq(thread.unsafeSynchronize(p), true);
 
   // Should be on the second step
   do_check_eq(++currentStep, 2);
   // There shouldn't be any nested event loops anymore
   do_check_eq(thread._nestedEventLoops.size, 0);
 
   finishClient(gClient);
 }
--- a/toolkit/devtools/server/tests/unit/test_nesting-02.js
+++ b/toolkit/devtools/server/tests/unit/test_nesting-02.js
@@ -9,70 +9,73 @@ var gClient;
 var gThreadActor;
 
 function run_test() {
   initTestDebuggerServer();
   let gDebuggee = addTestGlobal("test-nesting");
   gClient = new DebuggerClient(DebuggerServer.connectPipe());
   gClient.connect(function () {
     attachTestTabAndResume(gClient, "test-nesting", function (aResponse, aTabClient, aThreadClient) {
-      // Reach over the protocol connection and get a reference to the thread actor.
+      // Reach over the protocol connection and get a reference to the thread
+      // actor.
       gThreadActor = aThreadClient._transport._serverConnection.getActor(aThreadClient._actor);
 
       test_nesting();
     });
   });
   do_test_pending();
 }
 
 function test_nesting() {
   const thread = gThreadActor;
   const { resolve, reject, promise: p } = promise.defer();
 
   // The following things should happen (in order):
-  // 1. In the new event loop (created by synchronize)
+  // 1. In the new event loop (created by unsafeSynchronize)
   // 2. Resolve the promise (shouldn't exit any event loops)
-  // 3. Exit the event loop (should also then exit synchronize's event loop)
-  // 4. Be after the synchronize call
+  // 3. Exit the event loop (should also then exit unsafeSynchronize's event loop)
+  // 4. Be after the unsafeSynchronize call
   let currentStep = 0;
 
   executeSoon(function () {
     let eventLoop;
 
     executeSoon(function () {
       // Should be at step 2
       do_check_eq(++currentStep, 2);
-      // Before resolving, should have the synchronize event loop and the one just created.
+      // Before resolving, should have the unsafeSynchronize event loop and the
+      // one just created.
       do_check_eq(thread._nestedEventLoops.size, 2);
 
       executeSoon(function () {
         // Should be at step 3
         do_check_eq(++currentStep, 3);
         // Before exiting the manually created event loop, should have the
-        // synchronize event loop and the manual event loop.
+        // unsafeSynchronize event loop and the manual event loop.
         do_check_eq(thread._nestedEventLoops.size, 2);
         // Should have the event loop
         do_check_true(!!eventLoop);
         eventLoop.resolve();
       });
 
       resolve(true);
-      // Shouldn't exit any event loops because a new one started since the call to synchronize
+      // Shouldn't exit any event loops because a new one started since the call
+      // to unsafeSynchronize
       do_check_eq(thread._nestedEventLoops.size, 2);
     });
 
     // Should be at step 1
     do_check_eq(++currentStep, 1);
-    // Should have only the synchronize event loop
+    // Should have only the unsafeSynchronize event loop
     do_check_eq(thread._nestedEventLoops.size, 1);
     eventLoop = thread._nestedEventLoops.push();
     eventLoop.enter();
   });
 
-  do_check_eq(thread.synchronize(p), true);
+  do_check_eq(thread.unsafeSynchronize(p), true);
 
   // Should be on the fourth step
   do_check_eq(++currentStep, 4);
   // There shouldn't be any nested event loops anymore
   do_check_eq(thread._nestedEventLoops.size, 0);
 
   finishClient(gClient);
 }