Bug 1528654 - Select the first column breakpoint for _all_ evaluations of a Debugger.Source, not just the first. r=jlast
authorLogan Smyth <loganfsmyth@gmail.com>
Thu, 21 Feb 2019 16:44:54 +0000
changeset 460333 cd28688c1642889442179c459ce5e6cce9a3317e
parent 460332 c23dbcea5508a173b9d7d5cf265110763fc15943
child 460334 d116b37215697707fd6043f33182a70c2a40e214
child 460509 93c4470ee3af339226fe5ccb8db273be87b9935a
push id78690
push userlsmyth@mozilla.com
push dateThu, 21 Feb 2019 22:05:23 +0000
treeherderautoland@cd28688c1642 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlast
bugs1528654
milestone67.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 1528654 - Select the first column breakpoint for _all_ evaluations of a Debugger.Source, not just the first. r=jlast A JSScript can be explicitly cloned and applied to a new realm. When this is done within the same compartment as the original JSScript, it maintains its reference to the original ScriptSourceObject. This can lead to the potentially surprising fact that using Debugger.findScripts({ source }) can return multiple Debugger.Script objects representing the same function, but in multiple realms. When we query for breakpoints in a given source, we want to take the first column breakpoint on a given line, but that needs to apply to all potential instances of the Debugger.Script for that location, not just the first one. Differential Revision: https://phabricator.services.mozilla.com/D20431
devtools/server/actors/source.js
devtools/server/tests/unit/head_dbg.js
devtools/server/tests/unit/source-03.js
devtools/server/tests/unit/test_source-03.js
devtools/server/tests/unit/testactors.js
devtools/server/tests/unit/xpcshell.ini
--- a/devtools/server/actors/source.js
+++ b/devtools/server/actors/source.js
@@ -440,21 +440,36 @@ const SourceActor = ActorClassWithSpec(s
     let scripts = this._findDebuggeeScripts({ line });
     scripts = scripts.filter((script) => !actor.hasScript(script));
 
     // Find all entry points that correspond to the given location.
     const entryPoints = [];
     if (column === undefined) {
       // This is a line breakpoint, so we add a breakpoint on the first
       // breakpoint on the line.
+      const lineMatches = [];
       for (const script of scripts) {
-        const offsets = script.getPossibleBreakpointOffsets({ line });
-        if (offsets.length > 0) {
-          entryPoints.push({ script, offsets: [offsets[0]] });
-          break;
+        const possibleBreakpoints = script.getPossibleBreakpoints({ line });
+        for (const possibleBreakpoint of possibleBreakpoints) {
+          lineMatches.push({ ...possibleBreakpoint, script });
+        }
+      }
+      lineMatches.sort((a, b) => a.columnNumber - b.columnNumber);
+
+      if (lineMatches.length > 0) {
+        // A single Debugger.Source may have _multiple_ Debugger.Scripts
+        // at the same position from multiple evaluations of the source,
+        // so we explicitly want to take all of the matches for the matched
+        // column number.
+        const firstColumn = lineMatches[0].columnNumber;
+        const firstColumnMatches = lineMatches
+          .filter(m => m.columnNumber === firstColumn);
+
+        for (const { script, offset } of firstColumnMatches) {
+          entryPoints.push({ script, offsets: [offset] });
         }
       }
     } else {
       // Compute columnToOffsetMaps for each script so that we can
       // find matching entrypoints for the column breakpoint.
       const columnToOffsetMaps = scripts.map(script =>
         [
           script,
--- a/devtools/server/tests/unit/head_dbg.js
+++ b/devtools/server/tests/unit/head_dbg.js
@@ -925,17 +925,17 @@ function threadClientTest(test, options 
     await client.connect();
 
     // Attach to the fake tab target and retrieve the ThreadClient instance.
     // Automatically resume as the thread is paused by default after attach.
     const { targetFront, threadClient } =
       await attachTestTabAndResume(client, scriptName);
 
     // Run the test function
-    await test({ threadClient, debuggee, client, targetFront });
+    await test({ threadClient, debuggee, client, server, targetFront });
 
     // Cleanup the client after the test ran
     await client.close();
 
     server.removeTestGlobal(debuggee);
 
     // Also cleanup the created server
     server.destroy();
new file mode 100644
--- /dev/null
+++ b/devtools/server/tests/unit/source-03.js
@@ -0,0 +1,7 @@
+/* eslint-disable */
+
+function init() {
+  var a = foo();
+}
+
+function foo() {}
new file mode 100644
--- /dev/null
+++ b/devtools/server/tests/unit/test_source-03.js
@@ -0,0 +1,78 @@
+/* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+/* eslint-disable no-shadow, max-nested-callbacks */
+
+"use strict";
+
+const SOURCE_URL = getFileUrl("source-03.js");
+
+add_task(threadClientTest(async ({ threadClient, server }) => {
+  const promise = waitForNewSource(threadClient, SOURCE_URL);
+
+  // Create a two globals in the default junk sandbox compartment so that
+  // both globals are part of the same compartment.
+  server.allowNewThreadGlobals();
+  const debuggee1 = Cu.Sandbox(systemPrincipal);
+  debuggee1.__name = "debuggee2.js";
+  const debuggee2 = Cu.Sandbox(systemPrincipal);
+  debuggee2.__name = "debuggee2.js";
+  server.disallowNewThreadGlobals();
+
+  // Load two copies of the source file. The first call to "loadSubScript" will
+  // create a ScriptSourceObject and a JSScript which references it.
+  // The second call will attempt to re-use JSScript objects because that is
+  // what loadSubScript does for instances of the same file that are loaded
+  // in the system principal in the same compartment.
+  //
+  // We explicitly want this because it is an edge case of the server. Most
+  // of the time a Debugger.Source will only have a single Debugger.Script
+  // associated with a given function, but in the context of explicitly
+  // cloned JSScripts, this is not the case, and we need to handle that.
+  loadSubScript(SOURCE_URL, debuggee1);
+  loadSubScript(SOURCE_URL, debuggee2);
+
+  await promise;
+
+  // We want to set a breakpoint and make sure that the breakpoint is properly
+  // set on _both_ files backed
+  await setBreakpoint(threadClient, {
+    sourceUrl: SOURCE_URL,
+    line: 4,
+  });
+
+  const { sources } = await getSources(threadClient);
+  Assert.equal(sources.length, 1);
+
+  // Ensure that the breakpoint was properly applied to the JSScipt loaded
+  // in the first global.
+  let pausedOne = false;
+  threadClient.addOneTimeListener("paused", function(event, packet) {
+    pausedOne = true;
+    resume(threadClient);
+  });
+  Cu.evalInSandbox(
+    "init()",
+    debuggee1,
+    "1.8",
+    "test.js",
+    1
+  );
+  Assert.equal(pausedOne, true);
+
+  // Ensure that the breakpoint was properly applied to the JSScipt loaded
+  // in the second global.
+  let pausedTwo = false;
+  threadClient.addOneTimeListener("paused", function(event, packet) {
+    pausedTwo = true;
+    resume(threadClient);
+  });
+  Cu.evalInSandbox(
+    "init()",
+    debuggee2,
+    "1.8",
+    "test.js",
+    1
+  );
+  Assert.equal(pausedTwo, true);
+}, { doNotRunWorker: true }));
--- a/devtools/server/tests/unit/testactors.js
+++ b/devtools/server/tests/unit/testactors.js
@@ -24,16 +24,24 @@ DebuggerServer.getTestGlobal = function(
     if (g.__name == name) {
       return g;
     }
   }
 
   return null;
 };
 
+var gAllowNewThreadGlobals = false;
+DebuggerServer.allowNewThreadGlobals = function() {
+  gAllowNewThreadGlobals = true;
+};
+DebuggerServer.disallowNewThreadGlobals = function() {
+  gAllowNewThreadGlobals = false;
+};
+
 // A mock tab list, for use by tests. This simply presents each global in
 // gTestGlobals as a tab, and the list is fixed: it never calls its
 // onListChanged handler.
 //
 // As implemented now, we consult gTestGlobals when we're constructed, not
 // when we're iterated over, so tests have to add their globals before the
 // root actor is created.
 function TestTabList(connection) {
@@ -79,20 +87,25 @@ function TestTargetActor(connection, glo
   this._global = global;
   this._global.wrappedJSObject = global;
   this.threadActor = new ThreadActor(this, this._global);
   this.conn.addActor(this.threadActor);
   this._attached = false;
   this._extraActors = {};
   this.makeDebugger = makeDebugger.bind(null, {
     findDebuggees: () => [this._global],
-    shouldAddNewGlobalAsDebuggee: g => g.hostAnnotations &&
-                                       g.hostAnnotations.type == "document" &&
-                                       g.hostAnnotations.element === this._global,
+    shouldAddNewGlobalAsDebuggee: g => {
+      if (gAllowNewThreadGlobals) {
+        return true;
+      }
 
+      return g.hostAnnotations &&
+        g.hostAnnotations.type == "document" &&
+        g.hostAnnotations.element === this._global;
+    },
   });
 }
 
 TestTargetActor.prototype = {
   constructor: TestTargetActor,
   actorPrefix: "TestTargetActor",
 
   get window() {
--- a/devtools/server/tests/unit/xpcshell.ini
+++ b/devtools/server/tests/unit/xpcshell.ini
@@ -12,16 +12,17 @@ support-files =
   post_init_target_scoped_actors.js
   pre_init_global_actors.js
   pre_init_target_scoped_actors.js
   registertestactors-lazy.js
   sourcemapped.js
   testactors.js
   hello-actor.js
   stepping.js
+  source-03.js
   setBreakpoint-on-column.js
   setBreakpoint-on-column-minified.js
   setBreakpoint-on-column-in-gcd-script.js
   setBreakpoint-on-column-with-no-offsets.js
   setBreakpoint-on-column-with-no-offsets-in-gcd-script.js
   setBreakpoint-on-line.js
   setBreakpoint-on-line-in-gcd-script.js
   setBreakpoint-on-line-with-multiple-offsets.js
@@ -204,16 +205,17 @@ skip-if = true # breakpoint sliding is n
 [test_pause_exceptions-01.js]
 [test_pause_exceptions-02.js]
 [test_pause_exceptions-03.js]
 [test_longstringactor.js]
 [test_longstringgrips-01.js]
 [test_longstringgrips-02.js]
 [test_source-01.js]
 [test_source-02.js]
+[test_source-03.js]
 [test_wasm_source-01.js]
 [test_breakpoint-actor-map.js]
 skip-if = true # tests for breakpoint actors are obsolete bug 1524374
 [test_unsafeDereference.js]
 [test_add_actors.js]
 [test_ignore_caught_exceptions.js]
 [test_ignore_no_interface_exceptions.js]
 [test_requestTypes.js]