1842701 - Fallback to no-extra-bindings-mode in Debugger.Object.prototype.executeInGlobalWithBindings if possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Tooru Fujisawa [:arai]]

Debugger.Object.prototype.executeInGlobalWithBindings has the following problems:

• bug 1841964 / bug 1842441: not-intuitive variable access when syntactic variable conflicts with bindings
• bug 793345: slow performance due to not-optimized variable access

Both comes from the non-syntactic scope that causes the access to global/unqualified names getting de-optimized.

Given Debugger.Object.prototype.executeInGlobalWithBindings performance is important for DevTools and WebDriver, we could add dedicate mode and code path for this specific case.

We could apply some optimization given:

• the list of extra bindings is known at the point of compilation, and the list doesn't change for the entire execution
• if the conflict happens, users will most likely expect syntactic variables in their code always shadows the extra bindings

so, the following optimization can be applied:

• syntactic global variables and global lexicals access can use regular opcodes (JSOp::GetGName etc)
• unqualified names which doesn't appear in the extra bindings don't need to look into the extra bindings's environment

this will require the following:

• extra compilation/execution mode for "global script with extra bindings"
• a new scope and environment kind other than WithEnvironmentObject, to skip it during unqualified names lookup

possible limitation is the case where the code contains eval, which will need to keep using the current code path.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: WIP: Bug 1842701 - WIP: Add frontend::CompileGlobalScriptWithExtraBindings.

Comment 2

[Tooru Fujisawa [:arai]]

Attached WIP, which seems to solve both bug 1841964 and bug 793345.
the environment/scope handling is still rough and it needs some more work.

Comment 3

[Tooru Fujisawa [:arai]]

the main issue with the environment/scope handling is the JIT interaction with non-syntactic scope or the new environment type.

possible options I can think of are:

• (a) use dedicate environment/scope for extra-bindings, and add special handling for extra-bindings to baseline, Ion, bailout code, etc
• (b) use existing non-syntactic scope (with a tweak to omit conflict bindings), but only if any extra-binding is used (or possibly used via eval). if if none of extra-bindings are accessed, compile the code as regular global script

(b) is a quick workaround for the slow-ness, but users may find it strange that using $ etc makes the script slow.
(a) is more straight-forward solution but needs extra work.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: WIP: Bug 1842701 - WIP: Prototype B: Add frontend::CompileGlobalScriptWithExtraBindings with fallback to frontend::CompileGlobalScript if no binding is used.

Comment 5

[Tooru Fujisawa [:arai]]

attached another WIP, which keeps using WithEnvironmentObject, but without conflicting bindings, and also fallback to regular global script if no binding is used in the script.
this solves bug 1841964, and also solves bug 793345 if the script doesn't use the extra bindings.

Comment 6

[Tooru Fujisawa [:arai]]

So, I have a question regarding DevTools usage and WebDriver usage.
If the performance optimization for Debugger.Object.prototype.executeInGlobalWithBindings works only when the script has no reference to the extra bindings, how much beneficial would it be?
For example, if the reference for the extra binding always exists in the script, this won't improve any performance.

The detail about the "when the script has no reference to the extra bindings" is the following:

The following is an example where the script has a reference to an extra binding X, and the optimization doens't apply.

executeInGlobalWithBindings(`
console.log(X);
for (var i = 0; i < 10000000; i++) {} // slow
`, { X: 10 });

The condition is shared across all scripts (top-level, and all functions).
In the following case, only the function f has reference to X, but in this case, the optimization doesn't apply also to top-level and function g.
(although, the problem doesn't affect function script too much (see bug 793345 comment #25)

executeInGlobalWithBindings(`
function f() { console.log(X); }
function g() { }
for (var i = 0; i < 10000000; i++) {} // slow
`, { X: 10 });

The existence of the reference affects even if the execution doesn't reach the reference, and the optimization doesn't apply.

executeInGlobalWithBindings(`
if (some_falsy_condition) {
  console.log(X);
}
for (var i = 0; i < 10000000; i++) {} // slow
`, { X: 10 });

Also, eval can access the binding, and the existence of direct eval prevents the optimization:

executeInGlobalWithBindings(`
eval(unknown_code);
for (var i = 0; i < 10000000; i++) {} // slow
`, { X: 10 });

Otherwise, the optimization works.

executeInGlobalWithBindings(`
for (var i = 0; i < 10000000; i++) {} // fast
`, { X: 10 });

Comment 7

[Julian Descottes [:jdescottes]]

I think that for WebDriver it won't make much difference because the only time we are using executeInGlobalWithBindings, it's with an IIFE and that seems optimized already.

However for devtools I think this would be super beneficial. If I remember correctly, the bindings we inject are all the helpers that you expect to have available in the console (eg $, $$ ... I guess the full list is more or less: devtools/server/actors/webconsole/commands/manager.js). I won't say it's rare to use those helpers, but I think if we can get a performance improvement for all the DevTools console scripts which don't happen to use the helpers it would be great.

Unless I'm missing something and we somehow end up automatically referencing some of those bindings? If the patch is ready to be tested, simply testing the example from Bug 793345 should give us a good idea whether this is useful or not.

Comment 8

[Tooru Fujisawa [:arai]]

Thank you for your input :)

Okay, I'll look into the prototype B approach first.
with the patch, bug 793345 testcase runs in the same speed between the web content and devtools console.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Bug 1842701 - Part 1: Add CompileGlobalScriptWithExtraBindings. r?nbp!

Depends on D184195

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Bug 1842701 - Part 2: Use CompileGlobalScriptWithExtraBindings in Debugger.Object.prototype.executeInGlobalWithBindings unless useInnerBindings option is specified. r?nbp!

Depends on D184196

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1842701 - Part 3: Fallback to CompileGlobalScript without bindings from CompileGlobalScriptWithExtraBindings if no binding is used by the script. r?nbp!

Depends on D184197

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1842701 - Part 4: Add tests. r?nbp!

Depends on D184198

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1842701 - Part 5: Add tests for devtools. r?nchevobbe!

Depends on D184199

Comment 15

[Tooru Fujisawa [:arai]]

Actually, this doesn't work with devtools command (e.g. :screenshot), because it relies on the existing shadowing behavior (bindings shadows globals).

https://searchfox.org/mozilla-central/rev/ee35c5249171669c4428739f84ee7a7eb0f30316/devtools/server/actors/webconsole/commands/manager.js#203-208

// When we are running command via `:` prefix, no user code is being ran and only the command executes,
// so always expose the commands as the command will try to call its JavaScript method (see getEvalInput).
// Otherwise, when we run user code, we want to avoid overriding existing symbols with commands.
// Also ignore commands which can only be run with the `:` prefix.
if (
  !isCmd &&

the following test fails with the patches because global screenshot shadows the extra binding screenshot

https://searchfox.org/mozilla-central/rev/ee35c5249171669c4428739f84ee7a7eb0f30316/devtools/client/webconsole/test/browser/browser_jsterm_screenshot_command_user.js#9,25

function screenshot() {
...
const command = `:screenshot --clipboard`;

Comment 16

[Tooru Fujisawa [:arai]]

if :screenshot --clipboard is entered to the console, it's converted into screenshot({"clipboard":true}) and evaluated with executeInGlobalWithBindings.

possible workaround:

• (a) Add an option to executeInGlobalWithBindings to keep using the old behavior (CompileGlobalScript with non-syntactic scope), and use it if it was command
• (b) Avoid the collision by looking for not-colliding name
  • (b.1) add prefix to the function to avoid conflict: e.g. devtools_0_screenshot({"clipboard":true})
  • (b.2) use with: e.g. with (devtools_bindings_0) { screenshot({"clipboard":true}); }
• (c) do not use executeInGlobalWithBindings for command

Comment 17

[Tooru Fujisawa [:arai]]

:nchevobbe, can I have your input here, how to approach the above issue?
can the command be implemented in different way?

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Bug 1842701 - Part 1.9: WIP: Use unique name when evaluating command. r?nchevobbe!

Comment 19

[Tooru Fujisawa [:arai]]

so far, implemented the (b.1) way, but not sure if this is the right way.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Bug 1842701 - Part 1.9: Directly call command function for :command style. r?nchevobbe!

Comment 21

[Tooru Fujisawa [:arai]]

Added yet another patch with (c) way. I think the command-only case doesn't have to use the debugee, given all parameters are known, and don't come from debuggee.

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tooru Fujisawa [:arai] from comment #21)

Added yet another patch with (c) way. I think the command-only case doesn't have to use the debugee, given all parameters are known, and don't come from debuggee.

Sorry for the late response, I was crafting a response but seems like you went with something I was going to suggest :) So yes, I agree, we shouldn't need to involve the debuggee for those commands.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65b5aad07c1b
Part 1: Add CompileGlobalScriptWithExtraBindings. r=nbp
https://hg.mozilla.org/integration/autoland/rev/57d1b9c07b5f
Part 1.9: Directly call command function for :command style. r=nchevobbe,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/3e0836ade02c
Part 2: Use CompileGlobalScriptWithExtraBindings in Debugger.Object.prototype.executeInGlobalWithBindings. r=nbp
https://hg.mozilla.org/integration/autoland/rev/aca0fc6203a1
Part 3: Fallback to CompileGlobalScript without bindings from CompileGlobalScriptWithExtraBindings if no binding is used by the script. r=nbp
https://hg.mozilla.org/integration/autoland/rev/12002bd5e6a2
Part 4: Add tests. r=nbp
https://hg.mozilla.org/integration/autoland/rev/493b4e3a7627
Part 5: Add tests for devtools. r=nchevobbe,devtools-reviewers

Comment 24

[Tooru Fujisawa [:arai]]

Just realized that there are more bindings that may affect other feature.
I'll ask for backout and re-think about the binding shadowing.

https://searchfox.org/mozilla-central/rev/85269d4444c2553e7f4c669fe4de72d64f4fe438/devtools/server/actors/webconsole/eval-with-debugger.js#131-142

// '_self' refers to the JS object references via options.selectedObjectActor.
// This isn't exposed on typical console evaluation, but only when "Store As Global"
// runs an invisible script storing `_self` into `temp${i}`.
if (bindSelf) {
  bindings._self = bindSelf;
}

// Log points calls this method from the server side and pass additional variables
// to be exposed to the evaluated JS string
if (options.bindings) {
  bindings = { ...bindings, ...options.bindings };
}

Comment 25

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4237dd5e43b9
Backed out 6 changesets in order to re-think about the binding shadowing

Comment 26

[Tooru Fujisawa [:arai]]

There are some cases that's affected by this change, which results in breaking the behavior if the page as a conflicting binding.
Also there are some cases that's already affected by conflicting binding, because of existing shadowing behavior.

It seems to be that, there are 3 types of bindings:

• (A) Not used by internal code, and provided only for users
  • This kind of bindings can always be shadowed
• (B) Used only by internal code
  • This kind of bindings shouldn't be shadowed. Otherwise the feature breaks
• (C) Sometimes used by internal code, but also provided for users
  • This is complex case. If this is used by internal code, the binding shouldn't be shadowed, but if used by users, this should be shadowed

(A) case works perfectly with the patch's change.

(B) case doesn't work with the patch's change, and we'll need another way, such like, having 2 set of bindings, one for (A) and one for (B), and (A) gets shadowed but (B) doesn't get shadowed.

(C) case would need some kind of refactoring on DevTools side, not to rely on the current bindings, but to introduce (B)-kind bindings.

:nchevobbe, can I have your input what to do here?

Details of the bindings in the following:

This change affects when evalWithDebugger is called without options.frameActor provided.
If options.frameActor is provided, frame.evalWithBindings is used instead and the provided bindings shadows any global/local variables.

Each related consumer listed below.
Sections with "AFFECTED" are affected by this patch.
Sections with "PRE-EXISTING" have similar problem which is introduced by this patch.

_self binding

_self binding is defined when options.selectedObjectActor is provided.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/server/actors/webconsole/eval-with-debugger.js#120,134-135

const { dbgGlobal, bindSelf } = getDbgGlobal(options, dbg, webConsole);
...
if (bindSelf) {
  bindings._self = bindSelf;

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/server/actors/webconsole/eval-with-debugger.js#582,614-616,623-624

function getDbgGlobal(options, dbg, webConsole) {
...
  const jsVal = actor instanceof LongStringActor ? actor.str : actor.rawValue();
  if (!isObject(jsVal)) {
    return { bindSelf: jsVal, dbgGlobal };
...
  const bindSelf = dbgGlobal.makeDebuggeeValue(jsVal);
  return { bindSelf, dbgGlobal };

Also, options.selectedObjectActor is specified when ScriptCommand.execute is called with options.selectedObjectActor or options.selectedNodeActor or options.frameActor.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/shared/commands/script/script-command.js#47-53,65-66,107

async execute(expression, options = {}) {
  const {
    selectedObjectActor,
    selectedNodeActor,
    frameActor,
    selectedTargetFront,
  } = options;
...
  const selectedActor =
    selectedObjectActor || selectedNodeActor || frameActor;
...
        selectedObjectActor,

"Store as global variable" menu item (AFFECTED)

This uses commands.scriptCommand.execute with options.selectedObjectActor and without options.frameActor,
thus this uses executeInGlobalWithBindings path.

The expression relies on _self binding, which hadn't been shadowed by global, and
this feature breaks with the change.

tempN is directly defined on this, and not affected by the bindings change.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/client/webconsole/actions/object.js#16-22,24-27

const evalString = `{ let i = 0;
  while (this.hasOwnProperty("temp" + i) && i < 1000) {
    i++;
  }
  this["temp" + i] = _self;
  "temp" + i;
}`;
...
const res = await commands.scriptCommand.execute(evalString, {
  selectedObjectActor: actor,
  disableBreaks: true,
});

"Copy Object" menu item (AFFECTED/PRE-EXISTING)

This also uses commands.scriptCommand.execute with options.selectedObjectActor and without options.frameActor,
thus this uses executeInGlobalWithBindings path.

The expression also relies on _self binding, which hadn't been shadowed by global.
This feature breaks when _self binding gets shadowed.

Also, the expression relies on copy binding, which had been shadowed by global.
This is pre-existing issue, but the behavior doesn't change with the bindings change.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/client/webconsole/actions/object.js#50-53

const res = await commands.scriptCommand.execute("copy(_self)", {
  selectedObjectActor: actor,
  disableBreaks: true,
});

options.bindings

Caller can provide extra bindings by options.bindings.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/server/actors/webconsole/eval-with-debugger.js#140-141

if (options.bindings) {
  bindings = { ...bindings, ...options.bindings };

logEvent on worker

On worker, logEvent uses _consoleActor.evaluateJS without options.frameActor.
(on non-worker, this uses frame.evalWithBindings and is not affected)

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/server/actors/utils/logEvent.js#30,33,36-46,52,59

function logEvent({ threadActor, frame, level, expression, bindings }) {
...
  const displayName = formatDisplayName(frame);
...
  if (isWorker) {
    threadActor._parent._consoleActor.evaluateJS({
      text: `console.log(...${expression})`,
      bindings: { displayName, ...bindings },
      url: sourceActor.url,
      lineNumber: line,
      disableBreaks: true,
    });

    return undefined;
  }
...
    completion = frame.evalWithBindings(
...
    );

BreakpointActor.hit (AFFECTED)

This uses [${logValue}] expression where logValue is options.logValue of the breakpoint.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/server/actors/breakpoint.js#195,212-217

const { condition, logValue } = this.options || {};
...
  return logEvent({
    threadActor: this.threadActor,
    frame,
    level: "logPoint",
    expression: `[${logValue}]`,
  });

addBreakpointAtLine uses displayName as logValue, which hadn't been shadowed by global, and
this feature breaks with the change if this is used on worker.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/client/debugger/src/actions/breakpoints/index.js#229,245

export function addBreakpointAtLine(line, shouldLog = false, disabled = false) {
...
      options.logValue = "displayName";

ThreadActor._pauseAndRespondEventBreakpoint (AFFECTED)

This relies on _event binding which is provided via options.bindings, which hadn't been shadowed by global, and
this feature breaks with the change if this is used on worker.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/server/actors/thread.js#877,881-882

return logEvent({
...
  expression: `[_event]`,
  bindings: { _event: frame.arguments[0] },

Other observation

"Use in Console" menu item (NOT-AFFECTED/PRE-EXISTING)

Relies on $0, which had already been shadowed by global,
This is pre-existing issue, but the behavior doesn't change with the bindings change.

tempN is directly defined on window, and not affected by the bindings change.

https://searchfox.org/mozilla-central/rev/2bf90dc51ce7e8274ce208fbb9d68b3ff535185e/devtools/client/inspector/markup/markup-context-menu.js#364-370,372-375

const evalString = `{ let i = 0;
  while (window.hasOwnProperty("temp" + i) && i < 1000) {
    i++;
  }
  window["temp" + i] = $0;
  "temp" + i;
}`;
...
const res = await this.toolbox.commands.scriptCommand.execute(evalString, {
  selectedNodeActor: this.selection.nodeFront.actorID,
  disableBreaks: true,
});

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Thanks for the detailed information arai.
My initial feeling is that all those internal methods shouldn't have to use script evaluation to work, and would rather have dedicated webconsole actor methods that would use the Debugger API to achieve what they should do.

I think the disableBreaks option (which Alex added not long ago), does highlight this. We have this special flag for internal features, and so it's a bit of a smell, showing the evaluation isn't the appropriate mean.

With that being said, I'm wondering if we could use that to our advantage for now and prevent the bindings to be shadowed when disableBreaks is true? I feel like this would work here but maybe I'm missing something?

Comment 28

[Tooru Fujisawa [:arai]]

so, the idea is to keep the existing shadowing behavior (extra bindings shadows global variables) if options.disableBreaks is true, right?
I'll look into implementing the switch into executeInGlobalWithBindings.

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tooru Fujisawa [:arai] from comment #28)

so, the idea is to keep the existing shadowing behavior (extra bindings shadows global variables) if options.disableBreaks is true, right?

Correct. I think this should work (disableBreaks is only false on actual (webconsole) user evaluation)

I'll look into implementing the switch into executeInGlobalWithBindings.

Thanks a lot!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77899a08642e
Part 1: Add CompileGlobalScriptWithExtraBindings. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ceb7ff9dbef9
Part 1.9: Directly call command function for :command style. r=nchevobbe,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/8bf98957d7ab
Part 2: Use CompileGlobalScriptWithExtraBindings in Debugger.Object.prototype.executeInGlobalWithBindings unless useInnerBindings option is specified. r=nbp,devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/81b23bbbd047
Part 3: Fallback to CompileGlobalScript without bindings from CompileGlobalScriptWithExtraBindings if no binding is used by the script. r=nbp
https://hg.mozilla.org/integration/autoland/rev/a77d6cfde62d
Part 4: Add tests. r=nbp
https://hg.mozilla.org/integration/autoland/rev/07943b119126
Part 5: Add tests for devtools. r=nchevobbe,devtools-reviewers

Comment 31

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/77899a08642e
https://hg.mozilla.org/mozilla-central/rev/ceb7ff9dbef9
https://hg.mozilla.org/mozilla-central/rev/8bf98957d7ab
https://hg.mozilla.org/mozilla-central/rev/81b23bbbd047
https://hg.mozilla.org/mozilla-central/rev/a77d6cfde62d
https://hg.mozilla.org/mozilla-central/rev/07943b119126