Bug 1111058 - Clean up initialization of source actors and sourcemaps to handle race conditions and invalid sourcemaps better. r=fitzgen
authorJames Long <longster@gmail.com>
Wed, 07 Jan 2015 18:13:00 -0500
changeset 248539 d68aaa3f8e5c4586e4f126e65a551e1237563600
parent 248538 0f6c7aaaee350eeee7599473d091cd2be3a7d96d
child 248540 083244e156748d4864ce572f21ceadf6f69028a9
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfitzgen
bugs1111058
milestone37.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 1111058 - Clean up initialization of source actors and sourcemaps to handle race conditions and invalid sourcemaps better. r=fitzgen
browser/devtools/debugger/test/browser_dbg_pretty-print-07.js
browser/devtools/debugger/test/code_math_bogus_map.js
toolkit/devtools/server/actors/script.js
--- a/browser/devtools/debugger/test/browser_dbg_pretty-print-07.js
+++ b/browser/devtools/debugger/test/browser_dbg_pretty-print-07.js
@@ -30,26 +30,28 @@ function findSource() {
   });
 }
 
 function prettyPrintSource() {
   gThreadClient.source(gSource).prettyPrint(4, testPrettyPrinted);
 }
 
 function testPrettyPrinted({ error, source }) {
-  ok(!error);
-  ok(source.contains("\n    "));
+  ok(!error, "Should not get an error while pretty-printing");
+  ok(source.contains("\n    "),
+    "Source should be pretty-printed");
   disablePrettyPrint();
 }
 
 function disablePrettyPrint() {
   gThreadClient.source(gSource).disablePrettyPrint(testUgly);
 }
 
 function testUgly({ error, source }) {
-  ok(!error);
-  ok(!source.contains("\n    "));
+  ok(!error, "Should not get an error while disabling pretty-printing");
+  ok(!source.contains("\n    "),
+     "Source should not be pretty after disabling pretty-printing");
   closeDebuggerAndFinish(gPanel);
 }
 
 registerCleanupFunction(function() {
   gTab = gPanel = gClient = gThreadClient = gSource = null;
 });
--- a/browser/devtools/debugger/test/code_math_bogus_map.js
+++ b/browser/devtools/debugger/test/code_math_bogus_map.js
@@ -1,4 +1,4 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 function stopMe(){throw Error("boom");}try{stopMe();var a=1;a=a*2;}catch(e){};
-
+//# sourceMappingURL=bogus.map
--- a/toolkit/devtools/server/actors/script.js
+++ b/toolkit/devtools/server/actors/script.js
@@ -1192,17 +1192,17 @@ ThreadActor.prototype = {
     return listeners;
   },
 
   /**
    * Set a breakpoint on the first bytecode offset in the provided script.
    */
   _breakOnEnter: function(script) {
     let offsets = script.getAllOffsets();
-    let sourceActor = this.sources.source({ source: script.source });
+    let sourceActor = this.sources.createNonSourceMappedActor(script.source);
 
     for (let line = 0, n = offsets.length; line < n; line++) {
       if (offsets[line]) {
         let location = { line: line };
         let resp = sourceActor.setBreakpoint(location);
         dbg_assert(!resp.actualLocation, "No actualLocation should be returned");
         if (resp.error) {
           reportError(new Error("Unable to set breakpoint on event listener"));
@@ -1343,17 +1343,17 @@ ThreadActor.prototype = {
     const scripts = this.scripts.getAllScripts();
     for (let i = 0, len = scripts.length; i < len; i++) {
       let s = scripts[i];
       if (s.source) {
         sourcesToScripts.set(s.source, s);
       }
     }
 
-    return all([this.sources.sourcesForScript(script)
+    return all([this.sources.createSourceActors(script.source)
                 for (script of sourcesToScripts.values())]);
   },
 
   onSources: function (aRequest) {
     return this._discoverSources().then(() => {
       return {
         sources: [s.form() for (s of this.sources.iter())]
       };
@@ -1958,18 +1958,16 @@ ThreadActor.prototype = {
    * scope of the specified debuggee global.
    *
    * @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 (aScript, aGlobal) {
-    this.sources.sourcesForScript(aScript);
-
     // XXX: The scripts must be added to the ScriptStore before restoring
     // breakpoints in _addScript. If we try to add them to the ScriptStore
     // inside _addScript, 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.addScript(aScript);
     this.scripts.addScripts(aScript.getChildScripts());
 
@@ -2024,27 +2022,31 @@ ThreadActor.prototype = {
    * @returns true, if the script was added; false otherwise.
    */
   _addScript: function (aScript) {
     if (!this._allowSource(aScript.source)) {
       return false;
     }
 
     // Set any stored breakpoints.
-
     let endLine = aScript.startLine + aScript.lineCount - 1;
-    let source = this.sources.source({ source: aScript.source });
+    let source = this.sources.createNonSourceMappedActor(aScript.source);
     for (let bpActor of this.breakpointActorMap.findActors({ source: source.form() })) {
       // Limit the search to the line numbers contained in the new script.
       if (bpActor.location.line >= aScript.startLine
           && bpActor.location.line <= endLine) {
         source.setBreakpoint(bpActor.location, aScript);
       }
     }
 
+    // Go ahead and establish the source actors for this script, which
+    // fetches sourcemaps if available and sends onNewSource
+    // notifications
+    this.sources.createSourceActors(aScript.source);
+
     return true;
   },
 
 
   /**
    * Get prototypes and properties of multiple objects.
    */
   onPrototypesAndProperties: function (aRequest) {
@@ -5118,17 +5120,20 @@ exports.AddonThreadActor = AddonThreadAc
  * sources, etc for ThreadActors.
  */
 function ThreadSources(aThreadActor, aOptions, aAllowPredicate,
                        aOnNewSource) {
   this._thread = aThreadActor;
   this._useSourceMaps = aOptions.useSourceMaps;
   this._autoBlackBox = aOptions.autoBlackBox;
   this._allow = aAllowPredicate;
-  this._onNewSource = aOnNewSource;
+  this._onNewSource = DevToolsUtils.makeInfallible(
+    aOnNewSource,
+    "ThreadSources.prototype._onNewSource"
+  );
   this._anonSourceMapId = 1;
 
   // generated Debugger.Source -> promise of SourceMapConsumer
   this._sourceMaps = new Map();
   // sourceMapURL -> promise of SourceMapConsumer
   this._sourceMapCache = Object.create(null);
   // Debugger.Source -> SourceActor
   this._sourceActors = new Map();
@@ -5233,44 +5238,58 @@ ThreadSources.prototype = {
 
     if (source) {
       this._sourceActors.set(source, actor);
     }
     else {
       this._sourceMappedSourceActors[originalUrl] = actor;
     }
 
-    // Don't notify a new source if it's a generated one, as it has
-    // sourcemapped sources. The generated one is created to set
-    // breakpoints.
-    if (!source || !this._sourceMaps.has(source)) {
-      try {
-        this._onNewSource(actor);
-      } catch (e) {
-        reportError(e);
-      }
-    }
-
+    this._emitNewSource(actor);
     return actor;
   },
 
-  getSource: function(source) {
+  _emitNewSource: function(actor) {
+    if(!actor.source) {
+      // Always notify if we don't have a source because that means
+      // it's something that has been sourcemapped, or it represents
+      // the HTML file that contains inline sources.
+      this._onNewSource(actor);
+    }
+    else {
+      // If sourcemapping is enabled and a source has sourcemaps, we
+      // create `SourceActor` instances for both the original and
+      // generated sources. The source actors for the generated
+      // sources are only for internal use, however; breakpoints are
+      // managed by these internal actors. We only want to notify the
+      // user of the original sources though, so if the actor has a
+      // `Debugger.Source` instance and a valid source map (meaning
+      // it's a generated source), don't send the notification.
+      this.fetchSourceMap(actor.source).then(map => {
+        if(!map) {
+          this._onNewSource(actor);
+        }
+      });
+    }
+  },
+
+  getSourceActor: function(source) {
     if (source.url in this._sourceMappedSourceActors) {
       return this._sourceMappedSourceActors[source.url];
     }
 
     if (this._sourceActors.has(source)) {
       return this._sourceActors.get(source);
     }
 
     throw new Error('getSource: could not find source actor for ' +
                     (source.url || 'source'));
   },
 
-  getSourceByURL: function(url) {
+  getSourceActorByURL: function(url) {
     if (url) {
       for (let [source, actor] of this._sourceActors) {
         if (source.url === url) {
           return actor;
         }
       }
 
       if (url in this._sourceMappedSourceActors) {
@@ -5297,25 +5316,33 @@ ThreadSources.prototype = {
     } catch (e) {
       // Not a valid URL so don't try to parse out the filename, just test the
       // whole thing with the minified source regexp.
       return MINIFIED_SOURCE_REGEXP.test(aURL);
     }
   },
 
   /**
-   * Only to be used when we aren't source mapping.
+   * Create a source actor representing this source. This ignores
+   * source mapping and always returns an actor representing this real
+   * source. Use `createSourceActors` if you want to respect source maps.
+   *
+   * @param Debugger.Source aSource
+   *        The source instance to create an actor for.
+   * @returns SourceActor
    */
-  _sourceForScript: function (aScript) {
+  createNonSourceMappedActor: function (aSource) {
     // Don't use getSourceURL because we don't want to consider the
-    // displayURL property if it's an eval source
-    let url = isEvalSource(aScript.source) ? null : aScript.source.url;
-    let spec = {
-      source: aScript.source
-    };
+    // displayURL property if it's an eval source. We only want to
+    // consider real URLs, otherwise if there is a URL but it's
+    // invalid the code below will not set the content type, and we
+    // will later try to fetch the contents of the URL to figure out
+    // the content type, but it's a made up URL for eval sources.
+    let url = isEvalSource(aSource) ? null : aSource.url;
+    let spec = { source: aSource };
 
     // XXX bug 915433: We can't rely on Debugger.Source.prototype.text
     // if the source is an HTML-embedded <script> tag. Since we don't
     // have an API implemented to detect whether this is the case, we
     // need to be conservative and only treat valid js files as real
     // sources. Otherwise, use the `originalUrl` property to treat it
     // as an HTML source that manages multiple inline sources.
     if (url) {
@@ -5335,68 +5362,88 @@ ThreadSources.prototype = {
       // Assume the content is javascript if there's no URL
       spec.contentType = "text/javascript";
     }
 
     return this.source(spec);
   },
 
   /**
-   * Return a promise of an array of source actors representing all the
-   * sources of |aScript|.
+   * This is an internal function that returns a promise of an array
+   * of source actors representing all the source mapped sources of
+   * `aSource`, or `null` if the source is not sourcemapped or
+   * sourcemapping is disabled. Users should call `createSourceActors`
+   * instead of this.
    *
-   * If source map handling is enabled and |aScript| has a source map, then
-   * use it to find all of |aScript|'s *original* sources; return a promise
-   * of an array of source actors for those.
+   * @param Debugger.Source aSource
+   *        The source instance to create actors for.
+   * @return Promise of an array of source actors
    */
-  sourcesForScript: function (aScript) {
-    if (!this._useSourceMaps || !aScript.source.sourceMapURL) {
-      return resolve([this._sourceForScript(aScript)].filter(isNotNull));
-    }
-
-    return this.fetchSourceMap(aScript.source)
+  _createSourceMappedActors: function (aSource) {
+    if (!this._useSourceMaps || !aSource.sourceMapURL) {
+      return resolve(null);
+    }
+
+    return this.fetchSourceMap(aSource)
       .then(map => {
         if (map) {
           return [
-            this.source({ originalUrl: s,
-                          generatedSource: aScript.source })
+            this.source({ originalUrl: s, generatedSource: aSource })
             for (s of map.sources)
-          ];
+          ].filter(isNotNull);
         }
-
-        return [this._sourceForScript(aScript)];
-      })
-      .then(ss => ss.filter(isNotNull));
+        return null;
+      });
+  },
+
+  /**
+   * Creates the source actors representing the appropriate sources
+   * of `aSource`. If sourcemapped, returns actors for all of the original
+   * sources, otherwise returns a 1-element array with the actor for
+   * `aSource`.
+   *
+   * @param Debugger.Source aSource
+   *        The source instance to create actors for.
+   * @param Promise of an array of source actors
+   */
+  createSourceActors: function(aSource) {
+    return this._createSourceMappedActors(aSource).then(actors => {
+      let actor = this.createNonSourceMappedActor(aSource);
+      return (actors || [actor]).filter(isNotNull);
+    });
   },
 
   /**
    * Return a promise of a SourceMapConsumer for the source map for
    * `aSource`; if we already have such a promise extant, return that.
    * This will fetch the source map if we don't have a cached object
    * and source maps are enabled (see `_fetchSourceMap`).
+   *
+   * @param Debugger.Source aSource
+   *        The source instance to get sourcemaps for.
+   * @return Promise of a SourceMapConsumer
    */
   fetchSourceMap: function (aSource) {
     if (this._sourceMaps.has(aSource)) {
       return this._sourceMaps.get(aSource);
     }
     else if (!aSource || !aSource.sourceMapURL) {
       return resolve(null);
     }
 
     let sourceMapURL = aSource.sourceMapURL;
     if (aSource.url) {
       sourceMapURL = this._normalize(sourceMapURL, aSource.url);
     }
-
-    let map = this._fetchSourceMap(sourceMapURL, aSource.url);
-    if (map) {
-      this._sourceMaps.set(aSource, map);
-      return map;
-    }
-    return resolve(null);
+    let result = this._fetchSourceMap(sourceMapURL, aSource.url);
+
+    // The promises in `_sourceMaps` must be the exact same instances
+    // as returned by `_fetchSourceMap` for `clearSourceMapCache` to work.
+    this._sourceMaps.set(aSource, result);
+    return result;
   },
 
   /**
    * Return a promise of a SourceMapConsumer for the source map for
    * `aSource`. The resolved result may be null if the source does not
    * have a source map or source maps are disabled.
    */
   getSourceMap: function(aSource) {
@@ -5424,28 +5471,28 @@ ThreadSources.prototype = {
    *        source map, and the source map's sources are relative, we resolve
    *        them from aScriptURL.
    */
   _fetchSourceMap: function (aAbsSourceMapURL, aSourceURL) {
     if (this._sourceMapCache[aAbsSourceMapURL]) {
       return this._sourceMapCache[aAbsSourceMapURL];
     }
     else if (!this._useSourceMaps) {
-      return null;
+      return resolve(null);
     }
 
     let fetching = fetch(aAbsSourceMapURL, { loadFromCache: false })
       .then(({ content }) => {
         let map = new SourceMapConsumer(content);
         this._setSourceMapRoot(map, aAbsSourceMapURL, aSourceURL);
         return map;
       })
       .then(null, error => {
         if (!DevToolsUtils.reportingDisabled) {
-          DevToolsUtils.reportException("ThreadSources.prototype.getOriginalLocation", error);
+          DevToolsUtils.reportException("ThreadSources.prototype._fetchSourceMap", error);
         }
         return null;
       });
     this._sourceMapCache[aAbsSourceMapURL] = fetching;
     return fetching;
   },
 
   /**
@@ -5528,17 +5575,20 @@ ThreadSources.prototype = {
 
     // Forcefully set the sourcemap cache. This will be used even if
     // sourcemaps are disabled.
     this._sourceMapCache[url] = resolve(aMap);
   },
 
   /**
    * Returns a promise of the location in the original source if the source is
-   * source mapped, otherwise a promise of the same location.
+   * source mapped, otherwise a promise of the same location. This can
+   * be called with a source from *any* Debugger instance and we make
+   * sure to that it works properly, reusing source maps if already
+   * fetched. Use this from any actor that needs sourcemapping.
    */
   getOriginalLocation: function ({ source, line, column }) {
     // In certain scenarios the source map may have not been fetched
     // yet (or at least tied to this Debugger.Source instance), so use
     // `fetchSourceMap` instead of `getSourceMap`. This allows this
     // function to be called from anywere (across debuggers) and it
     // should just automatically work.
     return this.fetchSourceMap(source).then(sm => {
@@ -5549,29 +5599,37 @@ ThreadSources.prototype = {
           column: sourceCol,
           name: sourceName
         } = sm.originalPositionFor({
           line: line,
           column: column == null ? Infinity : column
         });
 
         return {
-          sourceActor: sourceUrl && this.source({ originalUrl: sourceUrl }),
+          // Since the `Debugger.Source` instance may come from a
+          // different `Debugger` instance (any actor can call this
+          // method), we can't rely on any of the source discovery
+          // setup (`_discoverSources`, etc) to have been run yet. So
+          // we have to assume that the actor may not already exist,
+          // and we might need to create it, so use `source` and give
+          // it the required parameters for a sourcemapped source.
+          sourceActor: (!sourceUrl) ? null : this.source({
+            originalUrl: sourceUrl,
+            generatedSource: source
+          }),
           url: sourceUrl,
           line: sourceLine,
           column: sourceCol,
           name: sourceName
         };
       }
 
       // No source map
       return resolve({
-        // Don't use `getSource` because sources may have not been
-        // created yet
-        sourceActor: this.source({ source }),
+        sourceActor: this.createNonSourceMappedActor(source),
         url: source.url,
         line: line,
         column: column
       });
     });
   },
 
   /**
@@ -5595,19 +5653,17 @@ ThreadSources.prototype = {
       if (sm) {
         let { line: genLine, column: genColumn } = sm.generatedPositionFor({
           source: sourceActor.url,
           line: line,
           column: column == null ? Infinity : column
         });
 
         return {
-          // Don't use `getSource` because this could intentionally
-          // create a generated source
-          sourceActor: this.source({ source: source }),
+          sourceActor: this.createNonSourceMappedActor(source),
           line: genLine,
           column: genColumn
         };
       }
 
       return resolve({
         sourceActor: sourceActor,
         line: line,