Bug 1225160 - Carefully avoid unsafeSynchronize when source maps are disabled;r=jlong
authorEddy Bruel <ejpbruel@mozilla.com>
Thu, 14 Apr 2016 21:57:58 +0200
changeset 331072 43d9dbd33ab796c4abb1cf973bb6c9d3d94a6e39
parent 331071 276092dfe88d39f27247a2c103842eab3a7e0cab
child 331073 009b0d561a9c10452c674a54aff993db3aeffdcd
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlong
bugs1225160
milestone48.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 1225160 - Carefully avoid unsafeSynchronize when source maps are disabled;r=jlong
devtools/server/actors/script.js
--- a/devtools/server/actors/script.js
+++ b/devtools/server/actors/script.js
@@ -1952,59 +1952,76 @@ const ThreadActor = ActorClass({
 
     // The scripts must be added to the ScriptStore before restoring
     // breakpoints. If we try to add them to the ScriptStore any later, we can
     // accidentally set a breakpoint in a top level script as a "closest match"
     // because we wouldn't have added the child scripts to the ScriptStore yet.
     this.scripts.addScripts(this.dbg.findScripts({ source: aSource }));
 
     let sourceActor = this.sources.createNonSourceMappedActor(aSource);
+    let bpActors = [...this.breakpointActorMap.findActors()];
 
-    // Set any stored breakpoints.
-    let bpActors = [...this.breakpointActorMap.findActors()];
-    let promises = [];
+    if (this._options.useSourceMaps) {
+      let promises = [];
 
-    // Go ahead and establish the source actors for this script, which
-    // fetches sourcemaps if available and sends onNewSource
-    // notifications.
-    let sourceActorsCreated = this.sources.createSourceActors(aSource);
+      // Go ahead and establish the source actors for this script, which
+      // fetches sourcemaps if available and sends onNewSource
+      // notifications.
+      let sourceActorsCreated = this.sources._createSourceMappedActors(aSource);
 
-    if (bpActors.length) {
-      // 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.unsafeSynchronize(sourceActorsCreated);
-    }
+      if (bpActors.length) {
+        // 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.unsafeSynchronize(sourceActorsCreated);
+      }
+
+      for (let _actor of bpActors) {
+        // 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.
+        let actor = _actor;
 
-    for (let _actor of bpActors) {
-      // 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.
-      let actor = _actor;
+        if (actor.isPending) {
+          promises.push(actor.originalLocation.originalSourceActor._setBreakpoint(actor));
+        } else {
+          promises.push(this.sources.getAllGeneratedLocations(actor.originalLocation)
+                                    .then((generatedLocations) => {
+            if (generatedLocations.length > 0 &&
+                generatedLocations[0].generatedSourceActor.actorID === sourceActor.actorID) {
+              sourceActor._setBreakpointAtAllGeneratedLocations(
+                actor,
+                generatedLocations
+              );
+            }
+          }));
+        }
+      }
 
-      if (actor.isPending) {
-        promises.push(actor.originalLocation.originalSourceActor._setBreakpoint(actor));
-      } else {
-        promises.push(this.sources.getAllGeneratedLocations(actor.originalLocation)
-                                  .then((generatedLocations) => {
-          if (generatedLocations.length > 0 &&
-              generatedLocations[0].generatedSourceActor.actorID === sourceActor.actorID) {
-            sourceActor._setBreakpointAtAllGeneratedLocations(
-              actor,
-              generatedLocations
-            );
-          }
-        }));
+      if (promises.length > 0) {
+        this.unsafeSynchronize(promise.all(promises));
       }
-    }
-
-    if (promises.length > 0) {
-      this.unsafeSynchronize(promise.all(promises));
+    } else {
+      // Bug 1225160: If addSource is called in response to a new script
+      // notification, and this notification was triggered by loading a JSM from
+      // chrome code, calling unsafeSynchronize could cause a debuggee timer to
+      // fire. If this causes the JSM to be loaded a second time, the browser
+      // will crash, because loading JSMS is not reentrant, and the first load
+      // has not completed yet.
+      //
+      // The root of the problem is that unsafeSynchronize can cause debuggee
+      // code to run. Unfortunately, fixing that is prohibitively difficult. The
+      // best we can do at the moment is disable source maps for the browser
+      // debugger, and carefully avoid the use of unsafeSynchronize in this
+      // function when source maps are disabled.
+      for (let actor of bpActors) {
+        actor.originalLocation.originalSourceActor._setBreakpoint(actor);
+      }
     }
 
     this._debuggerSourcesSeen.add(aSource);
     return true;
   },
 
 
   /**