Bug 1238173 - remove ScriptStore in use findScripts in debugger r=fitzgen
authorJames Long <longster@gmail.com>
Wed, 20 Jul 2016 14:57:13 -0400
changeset 345906 f272791a6d75ec9d551f9c73493eaa39021e5827
parent 345905 0cca5d80caa9f04d491c28b388f2f396fd45fe5c
child 345907 d8384a5bd4dfd6371e3d0d17fa0a91367a20dc7a
push id6389
push userraliiev@mozilla.com
push dateMon, 19 Sep 2016 13:38:22 +0000
treeherdermozilla-beta@01d67bfe6c81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfitzgen
bugs1238173
milestone50.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 1238173 - remove ScriptStore in use findScripts in debugger r=fitzgen
devtools/server/actors/script.js
devtools/server/actors/source.js
devtools/server/actors/utils/ScriptStore.js
devtools/server/actors/utils/moz.build
devtools/server/tests/unit/test_ScriptStore.js
devtools/server/tests/unit/xpcshell.ini
--- a/devtools/server/actors/script.js
+++ b/devtools/server/actors/script.js
@@ -16,17 +16,16 @@ const { ObjectActor, createValueGrip, lo
 const { SourceActor, getSourceURL } = require("devtools/server/actors/source");
 const { DebuggerServer } = require("devtools/server/main");
 const { ActorClassWithSpec } = require("devtools/shared/protocol");
 const DevToolsUtils = require("devtools/shared/DevToolsUtils");
 const { assert, dumpn, update, fetch } = DevToolsUtils;
 const promise = require("promise");
 const PromiseDebugging = require("PromiseDebugging");
 const xpcInspector = require("xpcInspector");
-const ScriptStore = require("./utils/ScriptStore");
 const { DevToolsWorker } = require("devtools/shared/worker/worker");
 const object = require("sdk/util/object");
 const { threadSpec } = require("devtools/shared/specs/script");
 
 const { defer, resolve, reject, all } = promise;
 
 loader.lazyGetter(this, "Debugger", () => {
   let Debugger = require("Debugger");
@@ -489,24 +488,16 @@ const ThreadActor = ActorClassWithSpec(t
     if (!this._threadLifetimePool) {
       this._threadLifetimePool = new ActorPool(this.conn);
       this.conn.addActorPool(this._threadLifetimePool);
       this._threadLifetimePool.objectActors = new WeakMap();
     }
     return this._threadLifetimePool;
   },
 
-  get scripts() {
-    if (!this._scripts) {
-      this._scripts = new ScriptStore();
-      this._scripts.addScripts(this.dbg.findScripts());
-    }
-    return this._scripts;
-  },
-
   get sources() {
     return this._parent.sources;
   },
 
   get youngestFrame() {
     if (this.state != "paused") {
       return null;
     }
@@ -640,18 +631,16 @@ const ThreadActor = ActorClassWithSpec(t
     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._pushThreadPause();
 
@@ -1179,17 +1168,18 @@ const ThreadActor = ActorClassWithSpec(t
   _breakOnEnter: function (script) {
     let offsets = script.getAllOffsets();
     for (let line = 0, n = offsets.length; line < n; line++) {
       if (offsets[line]) {
         // N.B. Hidden breakpoints do not have an original location, and are not
         // stored in the breakpoint actor map.
         let actor = new BreakpointActor(this);
         this.threadLifetimePool.addActor(actor);
-        let scripts = this.scripts.getScriptsBySourceAndLine(script.source, line);
+
+        let scripts = this.dbg.findScripts({ source: script.source, line: line });
         let entryPoints = findEntryPointsForLine(scripts, line);
         setBreakpointAtEntryPoints(actor, entryPoints);
         this._hiddenBreakpoints.set(actor.actorID, actor);
         break;
       }
     }
   },
 
@@ -1313,17 +1303,18 @@ const ThreadActor = ActorClassWithSpec(t
   },
 
   /**
    * Get the script and source lists from the debugger.
    */
   _discoverSources: function () {
     // Only get one script per Debugger.Source.
     const sourcesToScripts = new Map();
-    const scripts = this.scripts.getAllScripts();
+    const scripts = this.dbg.findScripts();
+
     for (let i = 0, len = scripts.length; i < len; i++) {
       let s = scripts[i];
       if (s.source) {
         sourcesToScripts.set(s.source, s);
       }
     }
 
     return all([...sourcesToScripts.values()].map(script => {
@@ -1919,46 +1910,27 @@ const ThreadActor = ActorClassWithSpec(t
         from: this.actorID,
         type: name,
         source: source.form()
       });
     }
   },
 
   /**
-   * Restore any pre-existing breakpoints to the sources that we have access to.
-   */
-  _restoreBreakpoints: function () {
-    if (this.breakpointActorMap.size === 0) {
-      return;
-    }
-
-    for (let s of this.scripts.getSources()) {
-      this._addSource(s);
-    }
-  },
-
-  /**
    * Add the provided source to the server cache.
    *
    * @param aSource Debugger.Source
    *        The source that will be stored.
    * @returns true, if the source was added; false otherwise.
    */
   _addSource: function (aSource) {
     if (!this.sources.allowSource(aSource) || this._debuggerSourcesSeen.has(aSource)) {
       return false;
     }
 
-    // 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()];
 
     if (this._options.useSourceMaps) {
       let promises = [];
 
       // Go ahead and establish the source actors for this script, which
       // fetches sourcemaps if available and sends onNewSource
--- a/devtools/server/actors/source.js
+++ b/devtools/server/actors/source.js
@@ -180,17 +180,16 @@ let SourceActor = ActorClassWithSpec(sou
 
   get isInlineSource() {
     return this._isInlineSource;
   },
 
   get threadActor() { return this._threadActor; },
   get sources() { return this._threadActor.sources; },
   get dbg() { return this.threadActor.dbg; },
-  get scripts() { return this.threadActor.scripts; },
   get source() { return this._source; },
   get generatedSource() { return this._generatedSource; },
   get breakpointActorMap() { return this.threadActor.breakpointActorMap; },
   get url() {
     if (this.source) {
       return getSourceURL(this.source, this.threadActor._parent.window);
     }
     return this._originalUrl;
@@ -439,17 +438,17 @@ let SourceActor = ActorClassWithSpec(sou
   /**
    * Extract all executable offsets from the given script
    * @param String url - extract offsets of the script with this url
    * @param Boolean onlyLine - will return only the line number
    * @return Set - Executable offsets/lines of the script
    **/
   getExecutableOffsets: function (source, onlyLine) {
     let offsets = new Set();
-    for (let s of this.threadActor.scripts.getScriptsBySource(source)) {
+    for (let s of this.dbg.findScripts({ source })) {
       for (let offset of s.getAllColumnOffsets()) {
         offsets.add(onlyLine ? offset.lineNumber : offset);
       }
     }
 
     return offsets;
   },
 
@@ -713,20 +712,27 @@ let SourceActor = ActorClassWithSpec(sou
     const { originalLocation } = actor;
     const { originalLine, originalSourceActor } = originalLocation;
 
     if (!this.isSourceMapped) {
       if (!this._setBreakpointAtGeneratedLocation(
         actor,
         GeneratedLocation.fromOriginalLocation(originalLocation)
       )) {
-        const scripts = this.scripts.getScriptsBySourceActorAndLine(
-          this,
-          originalLine
-        );
+        const query = { line: originalLine };
+        // For most cases, we have a real source to query for. The
+        // only time we don't is for HTML pages. In that case we want
+        // to query for scripts in an HTML page based on its URL, as
+        // there could be several sources within an HTML page.
+        if (this.source) {
+          query.source = this.source;
+        } else {
+          query.url = this.url;
+        }
+        const scripts = this.dbg.findScripts(query);
 
         // Never do breakpoint sliding for column breakpoints.
         // Additionally, never do breakpoint sliding if no scripts
         // exist on this line.
         //
         // Sliding can go horribly wrong if we always try to find the
         // next line with valid entry points in the entire file.
         // Scripts may be completely GCed and we never knew they
@@ -831,21 +837,25 @@ let SourceActor = ActorClassWithSpec(sou
   _setBreakpointAtGeneratedLocation: function (actor, generatedLocation) {
     let {
       generatedSourceActor,
       generatedLine,
       generatedColumn,
       generatedLastColumn
     } = generatedLocation;
 
-    // Find all scripts that match the given source actor and line number.
-    let scripts = this.scripts.getScriptsBySourceActorAndLine(
-      generatedSourceActor,
-      generatedLine
-    );
+    // Find all scripts that match the given source actor and line
+    // number.
+    const query = { line: generatedLine };
+    if (generatedSourceActor.source) {
+      query.source = generatedSourceActor.source;
+    } else {
+      query.url = generatedSourceActor.url;
+    }
+    let scripts = this.dbg.findScripts(query);
 
     scripts = scripts.filter((script) => !actor.hasScript(script));
 
     // Find all entry points that correspond to the given location.
     let entryPoints = [];
     if (generatedColumn === undefined) {
       // This is a line breakpoint, so we are interested in all offsets
       // that correspond to the given line number.
deleted file mode 100644
--- a/devtools/server/actors/utils/ScriptStore.js
+++ /dev/null
@@ -1,219 +0,0 @@
-/* -*- indent-tabs-mode: nil; js-indent-level: 2; js-indent-level: 2 -*- */
-/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * 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 { noop } = require("devtools/shared/DevToolsUtils");
-
-/**
- * A `ScriptStore` is a cache of `Debugger.Script` instances. It holds strong
- * references to the cached scripts to alleviate the GC-sensitivity issues that
- * plague `Debugger.prototype.findScripts`, but this means that its lifetime
- * must be managed carefully. It is the `ScriptStore` user's responsibility to
- * ensure that the `ScriptStore` stays up to date.
- *
- * Implementation Notes:
- *
- * The ScriptStore's prototype methods are very hot, in general. To help the
- * JIT, they avoid ES6-isms and higher-order iteration functions, for the most
- * part. You might be wondering why we don't maintain indices on, say,
- * Debugger.Source for faster querying, if these methods are so hot. First, the
- * hottest method is actually just getting all scripts; second, populating the
- * store becomes prohibitively expensive. So we fall back to linear queries
- * (which isn't so bad, because Debugger.prototype.findScripts is also linear).
- */
-function ScriptStore() {
-  // Set of every Debugger.Script in the cache.
-  this._scripts = new NoDeleteSet;
-}
-
-module.exports = ScriptStore;
-
-ScriptStore.prototype = {
-  // Populating a ScriptStore.
-
-  /**
-   * Add one script to the cache.
-   *
-   * @param Debugger.Script script
-   */
-  addScript(script) {
-    this._scripts.add(script);
-  },
-
-  /**
-   * Add many scripts to the cache at once.
-   *
-   * @param Array scripts
-   *        The set of Debugger.Scripts to add to the cache.
-   */
-  addScripts(scripts) {
-    for (var i = 0, len = scripts.length; i < len; i++) {
-      this.addScript(scripts[i]);
-    }
-  },
-
-  // Querying a ScriptStore.
-
-  /**
-   * Get all the sources for which we have scripts cached.
-   *
-   * @returns Array of Debugger.Source
-   */
-  getSources() {
-    return [...new Set(this._scripts.items.map(s => s.source))];
-  },
-
-  /**
-   * Get all the scripts in the cache.
-   *
-   * @returns read-only Array of Debugger.Script.
-   *
-   * NB: The ScriptStore retains ownership of the returned array, and the
-   * ScriptStore's consumers MUST NOT MODIFY its contents!
-   */
-  getAllScripts() {
-    return this._scripts.items;
-  },
-
-  getScriptsBySourceActor(sourceActor) {
-    return sourceActor.source ?
-           this.getScriptsBySource(sourceActor.source) :
-           this.getScriptsByURL(sourceActor._originalUrl);
-  },
-
-  getScriptsBySourceActorAndLine(sourceActor, line) {
-    return sourceActor.source ?
-           this.getScriptsBySourceAndLine(sourceActor.source, line) :
-           this.getScriptsByURLAndLine(sourceActor._originalUrl, line);
-  },
-
-  /**
-   * Get all scripts produced from the given source.
-   *
-   * @oaram Debugger.Source source
-   * @returns Array of Debugger.Script
-   */
-  getScriptsBySource(source) {
-    var results = [];
-    var scripts = this._scripts.items;
-    var length = scripts.length;
-    for (var i = 0; i < length; i++) {
-      if (scripts[i].source === source) {
-        results.push(scripts[i]);
-      }
-    }
-    return results;
-  },
-
-  /**
-   * Get all scripts produced from the given source whose source code definition
-   * spans the given line.
-   *
-   * @oaram Debugger.Source source
-   * @param Number line
-   * @returns Array of Debugger.Script
-   */
-  getScriptsBySourceAndLine(source, line) {
-    var results = [];
-    var scripts = this._scripts.items;
-    var length = scripts.length;
-    for (var i = 0; i < length; i++) {
-      var script = scripts[i];
-      if (script.source === source &&
-          script.startLine <= line &&
-          (script.startLine + script.lineCount) > line) {
-        results.push(script);
-      }
-    }
-    return results;
-  },
-
-  /**
-   * Get all scripts defined by a source at the given URL.
-   *
-   * @param String url
-   * @returns Array of Debugger.Script
-   */
-  getScriptsByURL(url) {
-    var results = [];
-    var scripts = this._scripts.items;
-    var length = scripts.length;
-    for (var i = 0; i < length; i++) {
-      if (scripts[i].url === url) {
-        results.push(scripts[i]);
-      }
-    }
-    return results;
-  },
-
-  /**
-   * Get all scripts defined by a source a the given URL and whose source code
-   * definition spans the given line.
-   *
-   * @param String url
-   * @param Number line
-   * @returns Array of Debugger.Script
-   */
-  getScriptsByURLAndLine(url, line) {
-    var results = [];
-    var scripts = this._scripts.items;
-    var length = scripts.length;
-    for (var i = 0; i < length; i++) {
-      var script = scripts[i];
-      if (script.url === url &&
-          script.startLine <= line &&
-          (script.startLine + script.lineCount) > line) {
-        results.push(script);
-      }
-    }
-    return results;
-  },
-};
-
-
-/**
- * A set which can only grow, and does not support the delete operation.
- * Provides faster iteration than the native Set by maintaining an array of all
- * items, in addition to the internal set of all items, which allows direct
- * iteration (without the iteration protocol and calling into C++, which are
- * both expensive).
- */
-function NoDeleteSet() {
-  this._set = new Set();
-  this.items = [];
-}
-
-NoDeleteSet.prototype = {
-  /**
-   * An array containing every item in the set for convenience and faster
-   * iteration. This is public for reading only, and consumers MUST NOT modify
-   * this array!
-   */
-  items: null,
-
-  /**
-   * Add an item to the set.
-   *
-   * @param any item
-   */
-  add(item) {
-    if (!this._set.has(item)) {
-      this._set.add(item);
-      this.items.push(item);
-    }
-  },
-
-  /**
-   * Return true if the item is in the set, false otherwise.
-   *
-   * @param any item
-   * @returns Boolean
-   */
-  has(item) {
-    return this._set.has(item);
-  }
-};
--- a/devtools/server/actors/utils/moz.build
+++ b/devtools/server/actors/utils/moz.build
@@ -5,13 +5,12 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 DevToolsModules(
     'actor-registry-utils.js',
     'audionodes.json',
     'automation-timeline.js',
     'make-debugger.js',
     'map-uri-to-addon-id.js',
-    'ScriptStore.js',
     'stack.js',
     'TabSources.js',
     'walker-search.js'
 )
deleted file mode 100644
--- a/devtools/server/tests/unit/test_ScriptStore.js
+++ /dev/null
@@ -1,168 +0,0 @@
-/* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-// Test the functionality of ScriptStore.
-
-const ScriptStore = require("devtools/server/actors/utils/ScriptStore");
-
-// Fixtures
-
-const firstSource = "firstSource";
-const secondSource = "secondSource";
-const thirdSource = "thirdSource";
-
-const scripts = new Set([
-  {
-    url: "a.js",
-    source: firstSource,
-    startLine: 1,
-    lineCount: 100,
-    global: "g1"
-  },
-  {
-    url: "a.js",
-    source: firstSource,
-    startLine: 1,
-    lineCount: 40,
-    global: "g1"
-  },
-  {
-    url: "a.js",
-    source: firstSource,
-    startLine: 50,
-    lineCount: 100,
-    global: "g1"
-  },
-  {
-    url: "a.js",
-    source: firstSource,
-    startLine: 60,
-    lineCount: 90,
-    global: "g1"
-  },
-  {
-    url: "index.html",
-    source: secondSource,
-    startLine: 150,
-    lineCount: 1,
-    global: "g2"
-  },
-  {
-    url: "index.html",
-    source: thirdSource,
-    startLine: 200,
-    lineCount: 100,
-    global: "g2"
-  },
-  {
-    url: "index.html",
-    source: thirdSource,
-    startLine: 250,
-    lineCount: 10,
-    global: "g2"
-  },
-  {
-    url: "index.html",
-    source: thirdSource,
-    startLine: 275,
-    lineCount: 5,
-    global: "g2"
-  }
-]);
-
-function contains(script, line) {
-  return script.startLine <= line &&
-    line < script.startLine + script.lineCount;
-}
-
-function run_test() {
-  testAddScript();
-  testAddScripts();
-  testGetSources();
-  testGetScriptsBySource();
-  testGetScriptsBySourceAndLine();
-  testGetScriptsByURL();
-  testGetScriptsByURLAndLine();
-}
-
-function testAddScript() {
-  const ss = new ScriptStore();
-
-  for (let s of scripts) {
-    ss.addScript(s);
-  }
-
-  equal(ss.getAllScripts().length, scripts.size);
-
-  for (let s of ss.getAllScripts()) {
-    ok(scripts.has(s));
-  }
-}
-
-function testAddScripts() {
-  const ss = new ScriptStore();
-  ss.addScripts([...scripts]);
-
-  equal(ss.getAllScripts().length, scripts.size);
-
-  for (let s of ss.getAllScripts()) {
-    ok(scripts.has(s));
-  }
-}
-
-function testGetSources() {
-  const ss = new ScriptStore();
-  ss.addScripts([...scripts]);
-
-  const expected = new Set([firstSource, secondSource, thirdSource]);
-  const actual = ss.getSources();
-  equal(expected.size, actual.length);
-
-  for (let s of actual) {
-    ok(expected.has(s));
-    expected.delete(s);
-  }
-}
-
-function testGetScriptsBySource() {
-  const ss = new ScriptStore();
-  ss.addScripts([...scripts]);
-
-  const expected = [...scripts].filter(s => s.source === thirdSource);
-  const actual = ss.getScriptsBySource(thirdSource);
-
-  deepEqual(actual, expected);
-}
-
-function testGetScriptsBySourceAndLine() {
-  const ss = new ScriptStore();
-  ss.addScripts([...scripts]);
-
-  const expected = [...scripts].filter(
-    s => s.source === firstSource && contains(s, 65));
-  const actual = ss.getScriptsBySourceAndLine(firstSource, 65);
-
-  deepEqual(actual, expected);
-}
-
-function testGetScriptsByURL() {
-  const ss = new ScriptStore();
-  ss.addScripts([...scripts]);
-
-  const expected = [...scripts].filter(s => s.url === "index.html");
-  const actual = ss.getScriptsByURL("index.html");
-
-  deepEqual(actual, expected);
-}
-
-function testGetScriptsByURLAndLine() {
-  const ss = new ScriptStore();
-  ss.addScripts([...scripts]);
-
-  const expected = [...scripts].filter(
-    s => s.url === "index.html" && contains(s, 250));
-  const actual = ss.getScriptsByURLAndLine("index.html", 250);
-
-  deepEqual(actual, expected);
-}
--- a/devtools/server/tests/unit/xpcshell.ini
+++ b/devtools/server/tests/unit/xpcshell.ini
@@ -32,17 +32,16 @@ support-files =
   setBreakpoint-on-line-with-no-offsets-in-gcd-script.js
   addons/web-extension/manifest.json
   addons/web-extension2/manifest.json
 
 [test_addon_reload.js]
 [test_addons_actor.js]
 [test_animation_name.js]
 [test_animation_type.js]
-[test_ScriptStore.js]
 [test_actor-registry-actor.js]
 [test_nesting-01.js]
 [test_nesting-02.js]
 [test_nesting-03.js]
 [test_forwardingprefix.js]
 [test_getyoungestframe.js]
 [test_nsjsinspector.js]
 [test_dbgactor.js]