1621572 - Eager evaluation can terminate evaluation inside of debugger hooks triggered by the evaluation itself

Status: VERIFIED FIXED

(DevTools :: Console, defect, P2)

Description

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: error_devtools.mkv

Affected versions

• 76.0a1 (20200310214454)

Affected platforms

• Ubuntu 18.04
• macOS 10.15
• Windows 10x64

Preconditions

• set devtools.chrome.enabled to true in about:config

Steps to reproduce

1. Open Firefox and Web Console (Ctrl+Shift+K) and Browser Console (Ctrl+Shift+J).
2. Type something in the web console.
3. Observe the browser console.

Expected result

• No errors are shown.

Actual result

• InvisibleToDebugger: Error ThreadSafeDevToolsUtils.js:90:13 is presented in browser console.

Regression Range

• Last good revision: 7b776bcc7576a5fbc0d85cd65c77682531458c15
• First bad revision: 36c3c857a5a749e6296470268371b06bd9bd0c6f
• Pushlog:
  https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7b776bcc7576a5fbc0d85cd65c77682531458c15&tochange=36c3c857a5a749e6296470268371b06bd9bd0c6f
• Possible regressor: bug 1620087.

Notes

• Attached a screen recording with the issue.

Comment 1

[Jason Laster [:jlast]]

CC logan, would you be able to look into this?

Comment 2

[Logan Smyth [:loganfsmyth]]

This is indeed caused by https://bugzilla.mozilla.org/show_bug.cgi?id=1620087. I looked into it for a while today and I know why it is happening, but it's much harder to decide how to fix it so we'll see how that all goes.

While frustrating as far as filling the console with errors, I don't think there's any actual problem here behavior-wise, at least.

Comment 3

[Logan Smyth [:loganfsmyth]]

So here's the issue:

1. You type "t" into the console, and eager-evaluation runs it.
2. The "onNewScript" hook in the server runs and calls createSourceActor
3. This function uses the isEvalSource, which isn't loaded yet because it is a lazyRequireGetter
4. We start loading the source-actor file to get the function and it causes some kind of mutation and terminates
5. Because of the way our module loader works, this termination does not propagate out of the require call, but the module is now just an empty object
6. createSourceActor tries to access (<emptyObject>).isEvalSource which is indeed undefined, and throws
7. That error is caught and logged and the debugger hook exits cleanly and there is no termination triggered
8. eager-eval works as intended because the error doesn't affect the code entered in the terminal, but we spam the console with errors because of the termination

This behavior of eager-eval causing hooks to run is also the cause of https://bugzilla.mozilla.org/show_bug.cgi?id=1614383

What I'm tentatively thinking is that we could have a new option on evalWithBindings and executeInGlobalWithBindings that explicitly requests that during synchronous execution of the given code, only run hooks allowed to run are the ones attached via the Debugger object being used to do the evaluation itself. We already have logic to do this automatically for onNativeCall because onNativeCall only runs for the debugger calling eval, which I personally have always felt was a bit of a weird special-case, so this would actually help clean that up too.

Comment 4

[Jason Laster [:jlast]]

Thanks for investigating. That makes sense.

Comment 5

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1621572 - Allow adopting a frame into another debugger. r=arai!

In keeping with adoptDebuggeeValue and adoptSource, this new adoptFrame
method is for use in cases where we explicitly want to create a new instance
of the Debugger type, and want to create a Frame associated with the new
Debugger that corresponds to the same Frame in the existing Debugger.

Comment 6

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1621572 - Only run evaling-debugger hooks during eager-eval. r=arai!,nchevobbe!

The primary issue here is that the intended usecase for onNativeCall is to
allow aborting execution in cases where we certain native functions are called.
Given that intended usecase, the issue here is that execution of Debugger
hooks themselves may trigger native functions or other cases where execution
will be terminated.

To avoid this issue, we've decided that the presence of an onNativeCall hook
should cause all Debugger hooks to be ignored other than those associated
with the Debugger object that is performing the evaluation.

In reality, we may want to make this even stricter in the future by
moving the implementation of "eager eval" to C++, making onNativeCall a
callback passed to the Debugger's eval-like APIs, and then disallowing all
hook execution during eager evaluation, but that would be a much larger task.

Depends on D67109

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2702634fdd8d
Allow adopting a frame into another debugger. r=arai
https://hg.mozilla.org/integration/autoland/rev/8abfd6e0163b
Only run evaling-debugger hooks during eager-eval. r=arai,nchevobbe

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2702634fdd8d
https://hg.mozilla.org/mozilla-central/rev/8abfd6e0163b

Comment 9

[Alexandru Trif, Desktop QA [:atrif]]

Hello, the issue is verified fixed using Firefox 76.0a1 (20200326213652) on Windows10x64, macOS 10.12 and Ubuntu 18.04. No InvisibleToDebugger: Error ThreadSafeDevToolsUtils.js:90:13 errors are thrown in the browser console after following STR from comment 0.
Also, the issue is reproducible with Firefox 75.0b10 (20200326191140) on Windows 10x64, macOS 10.12 and Ubuntu 18.04. Sorry for the confusion. Modifying the flags accordingly.