Bug 757408 - Opening debugger hangs completely on large scripts; r=past
authorNick Fitzgerald <fitzgen@gmail.com>
Thu, 20 Jun 2013 10:43:30 -0700
changeset 147286 feeeb2cb6a461e828e17d7358dae27e720de8717
parent 147285 c6a841a3e36f15236b9756e8232856ec7ca92132
child 147287 f88ebb5b2dc143db6fb709734bdacb0a17605f46
push id2697
push userbbajaj@mozilla.com
push dateMon, 05 Aug 2013 18:49:53 +0000
treeherdermozilla-beta@dfec938c7b63 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspast
bugs757408
milestone24.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 757408 - Opening debugger hangs completely on large scripts; r=past
toolkit/devtools/server/actors/script.js
toolkit/devtools/server/tests/unit/testcompatactors.js
--- a/toolkit/devtools/server/actors/script.js
+++ b/toolkit/devtools/server/actors/script.js
@@ -213,16 +213,18 @@ ThreadActor.prototype = {
     try {
       // Put ourselves in the paused state.
       let packet = this._paused();
       if (!packet) {
         return { error: "notAttached" };
       }
       packet.why = { type: "attached" };
 
+      this._restoreBreakpoints();
+
       // Send the response to the attach request now (rather than
       // returning it), because we're going to start a nested event loop
       // here.
       this.conn.send(packet);
 
       // Start a nested event loop.
       this._nest();
 
@@ -735,46 +737,33 @@ ThreadActor.prototype = {
     return {
       error: "noCodeAtLineColumn",
       actor: actor.actorID
     };
   },
 
   /**
    * Get the script and source lists from the debugger.
+   *
+   * TODO bug 637572: we should be dealing with sources directly, not inferring
+   * them through scripts.
    */
-  _discoverScriptsAndSources: function TA__discoverScriptsAndSources() {
-    let promises = [];
-    let foundSourceMaps = false;
-    let scripts = this.dbg.findScripts();
-    for (let s of scripts) {
-      if (s.sourceMapURL && !foundSourceMaps) {
-        foundSourceMaps = true;
-        break;
-      }
+  _discoverSources: function TA__discoverSources() {
+    // Only get one script per url.
+    let scriptsByUrl = {};
+    for (let s of this.dbg.findScripts()) {
+      scriptsByUrl[s.url] = s;
     }
-    if (this._options.useSourceMaps && foundSourceMaps) {
-      for (let s of scripts) {
-        promises.push(this._addScript(s));
-      }
-      return all(promises);
-    }
-    // When source maps are not enabled or not used in the page _addScript is
-    // synchronous, since it doesn't need to wait for fetching source maps, so
-    // resolves immediately. This eliminates a huge slowdown in script-heavy
-    // pages like G+ or chrome debugging, where the GC takes a long time to
-    // clean up after Promise.all.
-    for (let s of scripts) {
-      this._addScriptSync(s);
-    }
-    return resolve(null);
+
+    return all([this.sources.sourcesForScript(scriptsByUrl[s])
+                for (s of Object.keys(scriptsByUrl))]);
   },
 
   onSources: function TA_onSources(aRequest) {
-    return this._discoverScriptsAndSources().then(() => {
+    return this._discoverSources().then(() => {
       return {
         sources: [s.form() for (s of this.sources.iter())]
       };
     });
   },
 
   /**
    * Disassociate all breakpoint actors from their scripts and clear the
@@ -1264,16 +1253,17 @@ ThreadActor.prototype = {
    *
    * @param aScript Debugger.Script
    *        The source script that has been loaded into a debuggee compartment.
    * @param aGlobal Debugger.Object
    *        A Debugger.Object instance whose referent is the global object.
    */
   onNewScript: function TA_onNewScript(aScript, aGlobal) {
     this._addScript(aScript);
+    this.sources.sourcesForScript(aScript);
   },
 
   onNewSource: function TA_onNewSource(aSource) {
     this.conn.send({
       from: this.actorID,
       type: "newSource",
       source: aSource.form()
     });
@@ -1298,86 +1288,55 @@ ThreadActor.prototype = {
     // Ignore about:* pages for content debugging.
     if (aSourceUrl.indexOf("about:") == 0) {
       return false;
     }
     return true;
   },
 
   /**
-   * Add the provided script to the server cache synchronously, without checking
-   * for any declared source maps.
+   * Restore any pre-existing breakpoints to the scripts that we have access to.
+   */
+  _restoreBreakpoints: function TA__restoreBreakpoints() {
+    for (let s of this.dbg.findScripts()) {
+      this._addScript(s);
+    }
+  },
+
+  /**
+   * Add the provided script to the server cache.
    *
    * @param aScript Debugger.Script
    *        The source script that will be stored.
    * @returns true, if the script was added; false otherwise.
    */
-  _addScriptSync: function TA__addScriptSync(aScript) {
+  _addScript: function TA__addScript(aScript) {
     if (!this._allowSource(aScript.url)) {
       return false;
     }
 
-    this.sources.source(aScript.url);
     // Set any stored breakpoints.
     let existing = this._breakpointStore[aScript.url];
     if (existing) {
       let endLine = aScript.startLine + aScript.lineCount - 1;
       // Iterate over the lines backwards, so that sliding breakpoints don't
       // affect the loop.
-      for (let line = existing.length - 1; line >= 0; line--) {
+      for (let line = existing.length - 1; line >= aScript.startLine; line--) {
         let bp = existing[line];
         // Only consider breakpoints that are not already associated with
         // scripts, and limit search to the line numbers contained in the new
         // script.
-        if (bp && !bp.actor.scripts.length &&
-            line >= aScript.startLine && line <= endLine) {
+        if (bp && !bp.actor.scripts.length && line <= endLine) {
           this._setBreakpoint(bp);
         }
       }
     }
     return true;
   },
 
-  /**
-   * Add the provided script to the server cache.
-   *
-   * @param aScript Debugger.Script
-   *        The source script that will be stored.
-   * @returns a promise of true, if the script was added; of false otherwise.
-   */
-  _addScript: function TA__addScript(aScript) {
-    if (!this._allowSource(aScript.url)) {
-      return resolve(false);
-    }
-
-    // TODO bug 637572: we should be dealing with sources directly, not
-    // inferring them through scripts.
-    return this.sources.sourcesForScript(aScript).then(() => {
-
-      // Set any stored breakpoints.
-      let existing = this._breakpointStore[aScript.url];
-      if (existing) {
-        let endLine = aScript.startLine + aScript.lineCount - 1;
-        // Iterate over the lines backwards, so that sliding breakpoints don't
-        // affect the loop.
-        for (let line = existing.length - 1; line >= 0; line--) {
-          let bp = existing[line];
-          // Only consider breakpoints that are not already associated with
-          // scripts, and limit search to the line numbers contained in the new
-          // script.
-          if (bp && !bp.actor.scripts.length &&
-              line >= aScript.startLine && line <= endLine) {
-            this._setBreakpoint(bp);
-          }
-        }
-      }
-
-      return true;
-    });
-  },
 };
 
 ThreadActor.prototype.requestTypes = {
   "attach": ThreadActor.prototype.onAttach,
   "detach": ThreadActor.prototype.onDetach,
   "reconfigure": ThreadActor.prototype.onReconfigure,
   "resume": ThreadActor.prototype.onResume,
   "clientEvaluate": ThreadActor.prototype.onClientEvaluate,
@@ -2633,17 +2592,18 @@ ThreadSources.prototype = {
       return resolve([this.source(aScript.url)].filter(isNotNull));
     }
 
     return this.sourceMap(aScript)
       .then((aSourceMap) => {
         return [
           this.source(s, aSourceMap) for (s of aSourceMap.sources)
         ];
-      }, (e) => {
+      })
+      .then(null, (e) => {
         reportError(e);
         delete this._sourceMaps[this._normalize(aScript.sourceMapURL, aScript.url)];
         delete this._sourceMapsByGeneratedSource[aScript.url];
         return [this.source(aScript.url)];
       })
       .then(function (aSources) {
         return aSources.filter(isNotNull);
       });
@@ -2677,17 +2637,17 @@ ThreadSources.prototype = {
    * |aAbsSourceMapURL|, which must be absolute. If there is already such a
    * promise extant, return it.
    */
   _fetchSourceMap: function TS__fetchSourceMap(aAbsSourceMapURL) {
     if (aAbsSourceMapURL in this._sourceMaps) {
       return this._sourceMaps[aAbsSourceMapURL];
     } else {
       let promise = fetch(aAbsSourceMapURL).then((rawSourceMap) => {
-        let map =  new SourceMapConsumer(rawSourceMap);
+        let map = new SourceMapConsumer(rawSourceMap);
         let base = aAbsSourceMapURL.replace(/\/[^\/]+$/, '/');
         if (base.indexOf("data:") !== 0) {
           map.sourceRoot = map.sourceRoot
             ? this._normalize(map.sourceRoot, base)
             : base;
         }
         return map;
       });
--- a/toolkit/devtools/server/tests/unit/testcompatactors.js
+++ b/toolkit/devtools/server/tests/unit/testcompatactors.js
@@ -25,17 +25,17 @@ function createRootActor()
           return {
             from: actor.actorID,
             type: "tabAttached",
             threadActor: actor.thread.actorID
           };
         };
 
         actor.thread.requestTypes["scripts"] = function (aRequest) {
-          return this._discoverScriptsAndSources().then(function () {
+          return this._discoverSources().then(function () {
             let scripts = [];
             for (let s of this.dbg.findScripts()) {
               if (!s.url) {
                 continue;
               }
               let script = {
                 url: s.url,
                 startLine: s.startLine,