1637883 - [Instant Evaluation] Firefox becomes unresponsive after using an infinite loop twice while visiting about:config page

Status: VERIFIED FIXED

(DevTools :: Console, defect)

Description

[Alexandru Trif, QA [:atrif]]

Attachment: while_true.gif

Affected versions

• 78.0a1 (20200513215059)
• 77.0b5 (20200512163137)

Affected platforms

• Windows 10x64
• Windows 7x64

Steps to reproduce

1. Open Firefox with a new profile and Web console (Ctrl+ Shift + K)
2. Paste while(true); in web console
3. Go to about:config page while web console is opened.
4. Paste while(true); in the web console.

Expected result

• Firefox function as expected.

Actual result

• Firefox becomes unresponsive for a while and an error is presented.

Regression Range

• I am unable to reproduce the issue with Firefox 76.0.1 (20200507114007). Will search for one ASAP.

Notes

• Attached a screen recording.

Comment 1

[Alexandru Trif, QA [:atrif]]

Attaching regression range here:

Last good revision: e93494134ba8fac0d76f78cca01d7e589977a672
First bad revision: 43b914eee3e3e9fca5986ce6791d35939905f50f
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e93494134ba8fac0d76f78cca01d7e589977a672&tochange=43b914eee3e3e9fca5986ce6791d35939905f50f

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Logan, would you know what's happening here?

Comment 3

[Logan Smyth [:loganfsmyth]]

Looking at this from a high level, if you look at the console right as FF freezes, we have the following error:

console.error: "Error while calling actor 'console's method 'autocomplete'" "debugger and debuggee must be in different compartments"
console.error: "autocomplete@resource://devtools/server/actors/webconsole.js:1362:30\nhandler@resource://devtools/shared/protocol/Actor.js:154:37\nonPacket@resource://devtools/server/devtools-server-connection.js:380:58\nreceiveMessage@resource://devtools/shared/transport/child-transport.js:66:16\nMessageListener.receiveMessage*_addListener@resource://devtools/shared/transport/child-transport.js:40:14\nready@resource://devtools/shared/transport/child-transport.js:57:10\n_onConnection@resource://devtools/server/devtools-server.js:471:15\nconnectToParent@resource://devtools/server/devtools-server.js:345:17\nonConnect<@resource://devtools/server/startup/frame.js:62:35\nexports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:103:22\nMessageListener.receiveMessage*@resource://devtools/server/startup/frame.js:95:23\n@resource://devtools/server/startup/frame.js:177:5\nconnectToFrame/<@resource://devtools/server/connectors/frame-connector.js:45:8\nconnectToFrame@resource://devtools/server/connectors/frame-connector.js:41:10\ngetTarget/<@resource://devtools/server/actors/descriptors/tab.js:139:35\ngetTarget@resource://devtools/server/actors/descriptors/tab.js:123:12\nhandler@resource://devtools/shared/protocol/Actor.js:154:37\nonPacket@resource://devtools/server/devtools-server-connection.js:380:58\nsend/<@resource://devtools/shared/transport/local-transport.js:68:25\nexports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:103:22\nexports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:103:22\n"

and those are indicative of the overall issue.

Somehow, this.evalWindow in the WebConsole actor is referencing something that cannot be a debuggee of this.dbg because they are in the same compartment. That means that the debugger hooks can't actually watch the evalled code because it's not a debuggee, so it can't cancel the code after the timeout. This happens only when you start on one page, and then navigate to another like about:config. If you then close the debugger and open it again fresh on about:config, things work as expected.

The big question is, why isn't it a valid debuggee, and is that a bug in the devtools itself somewhere? I'd guess that there's some state transition during navigation that we're not handling properly, but honestly I don't know.

A small patch that we should probably land would to at least avoid locking the UI would be

diff --git a/devtools/server/actors/webconsole/eval-with-debugger.js b/devtools/server/actors/webconsole/eval-with-debugger.js
--- a/devtools/server/actors/webconsole/eval-with-debugger.js
+++ b/devtools/server/actors/webconsole/eval-with-debugger.js
@@ -207,6 +207,10 @@ function getEvalResult(
   noSideEffectDebugger
 ) {
   if (noSideEffectDebugger) {
+    if (!noSideEffectDebugger.hasDebuggee(dbgWindow.unsafeDereference())) {
+      return null;
+    }
+
     // When a sideeffect-free debugger has been created, we need to eval
     // in the context of that debugger in order for the side-effect tracking
     // to apply.

so we skip eager eval if it's not a debuggee.

Comment 4

[Pascal Chevrel:pascalc]

S3, untracking and wontfix for 77.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Logan Smyth [:loganfsmyth] from comment #3)

Somehow, this.evalWindow in the WebConsole actor is referencing something that cannot be a debuggee of this.dbg because they are in the same compartment. That means that the debugger hooks can't actually watch the evalled code because it's not a debuggee, so it can't cancel the code after the timeout. This happens only when you start on one page, and then navigate to another like about:config. If you then close the debugger and open it again fresh on about:config, things work as expected.

Thanks for the investigation Logan. I'll check what's happening, but I guess we may be missing a target switch in this specific case?

A small patch that we should probably land would to at least avoid locking the UI would be

diff --git a/devtools/server/actors/webconsole/eval-with-debugger.js b/devtools/server/actors/webconsole/eval-with-debugger.js
--- a/devtools/server/actors/webconsole/eval-with-debugger.js
+++ b/devtools/server/actors/webconsole/eval-with-debugger.js
@@ -207,6 +207,10 @@ function getEvalResult(
   noSideEffectDebugger
 ) {
   if (noSideEffectDebugger) {
+    if (!noSideEffectDebugger.hasDebuggee(dbgWindow.unsafeDereference())) {
+      return null;
+    }
+
     // When a sideeffect-free debugger has been created, we need to eval
     // in the context of that debugger in order for the side-effect tracking
     // to apply.

so we skip eager eval if it's not a debuggee.

Nice! Would you want to push that to review, or do you want me to do it? It would be nice to fix this in 78 (even if that won't fix the issue if you autocomplete/regular evaluate I guess?)

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

I can't reproduce the bug 🤔

Comment 7

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1637883 - Bail on eager eval for same-compartment windows. r=nchevobbe!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a08423af0a66
Bail on eager eval for same-compartment windows. r=nchevobbe

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a08423af0a66

Comment 10

[Alexandru Trif, QA [:atrif]]

Verified the issue using Firefox 80.0a1 (20200702094606) on Windows 10x64.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta and ESR78?

Comment 12

[Logan Smyth [:loganfsmyth]]

I think this should be totally safe to uplift.

@nicolas Does uplift sound good to you?

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Logan Smyth [:loganfsmyth] from comment #12)

I think this should be totally safe to uplift.

@nicolas Does uplift sound good to you?

sounds good to me

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9159774 [details]
Bug 1637883 - Bail on eager eval for same-compartment windows. r=nchevobbe!

Beta/Release Uplift Approval Request

• User impact if declined: Text typed in console on about: pages might bring the browser down
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Open Firefox with a new profile and Web console (Ctrl+ Shift + K)

1. Paste while(true); in web console
2. Go to about:config page while web console is opened.
3. Paste while(true); in the web console.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): small change to simply bail out of instant evaluation in a specific case
• String changes made/needed:

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

(clearing ni)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9159774 [details]
Bug 1637883 - Bail on eager eval for same-compartment windows. r=nchevobbe!

Approved for 79.0b5. Seems edge-case enough to not worry about uplifting to ESR78.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1955e22bd97e

Comment 18

[Cristian Fogel, QA [:cfogel]]

Verified with 79.0b5 as well.

As a clarification note:
By evaluating the expression(pressing the Enter key after the Paste action) brings up the Blocker notification messages as expected on both steps.
No issues encountered on that part as well, during the verification on my side; browser becoming responsive again after stopping them.