Closed Bug 814683 Opened 7 years ago Closed 5 years ago

infinite loop in watch expression hangs browser

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: luke, Assigned: past)

Details

Attachments

(1 file, 1 obsolete file)

We were using the FF debugger (which I'm happy to see now does a pretty good job on big scripts :) and noticed that if our watch expression happens to call a function that infinite loops, the browser hangs and never offers a slow-script dialog.  This may well be a bug in DOM, but I thought I'd start and get a nice mochitest.
I don't know of anything in Debugger that would prevent SpiderMonkey from acting on slow script timeouts.

My first suspicion is that, when we "pause" the debuggee, we also disable the slow script timeout. This seems appropriate for cases like breakpoint hits, when the event handler won't return until the user decides to let it continue --- which could legitimately be a very long time. Perhaps we don't re-enable it when doing nested evaluation?

Yes, a nice mochitest would be perfect.
The simplest STR is:
 - stop at a breakpoint
 - enter the watch expression "while(true);"
Filter on BLACKEAGLE2.
Assignee: nobody → past
Priority: -- → P2
Re-pinging jimb on this; it'd certainly be great to fix.
Attached patch Test (obsolete) — Splinter Review
Here is a test that exhibits the behavior noted above. I tried to make the preNest() and postNest() hooks NOPs, but that didn't change anything. Any other ideas Jim?
We need to find out why the slow script timeout doesn't apply to JS called from the debugger. Debugger doesn't mess with this, and as far as I have seen, neither does the debugger's JS code. If we can catch the timeout being disabled, then we can make some progress here.
I just re-tested and the slow script dialog now kicks in as expected. I'm getting unexpected errors when I stop it and return to the debugger:

onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'clientEvaluate: TypeError: invalid 'in' operand aCompletion\nStack: ThreadActor.prototype.createProtocolCompletionValue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2054:1\nThreadActor.prototype.onClientEvaluate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:1336:35\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1229:15\nLocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:545:11\nmakeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14\nmakeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14\nEventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:432:5\nThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:602:5\nThreadActor.prototype._pauseAndRespond@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:882:7\nThreadActor.prototype.onDebuggerStatement@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2229:9\n@http://htmlpad.org/debugger/ line 33 > eval:2:1\ntest@http://htmlpad.org/debugger/:33:13\nload@http://htmlpad.org/debugger/:97:1\nLine: 2054, column: 0"}
Stack: DebuggerClient.prototype.onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:865:1
resolve@resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:861:1
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:545:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
EventLoop.prototype.enter@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:432:5
ThreadActor.prototype._pushThreadPause@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:602:5
ThreadActor.prototype._pauseAndRespond@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:882:7
ThreadActor.prototype.onDebuggerStatement@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:2229:9
@http://htmlpad.org/debugger/ line 33 > eval:2:1
test@http://htmlpad.org/debugger/:33:13
load@http://htmlpad.org/debugger/:97:1
Line: 865, column: 0

I'll investigate some more.
The debugger api returns `null` when execution is forcibly terminated, so it would seem we aren't handling that case and trying to check `"return" in aCompletion` or whatever.
... like the completion was an object (as it is in every case except forced termination).
The fix was quite straightforward. I've tested with infinite loops in both watch expressions and conditional breakpoints and the debugger now does the right thing.

Try: https://tbpl.mozilla.org/?tree=Try&rev=220090896b64
Attachment #8456235 - Flags: review?(nfitzgerald)
Status: NEW → ASSIGNED
Comment on attachment 8456235 [details] [diff] [review]
Properly handle terminated scripts in the debugger

Review of attachment 8456235 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/debugger/debugger-controller.js
@@ +973,5 @@
>  
>      // If an error was thrown during the evaluation of the watch expressions,
>      // then at least one expression evaluation could not be performed. So
>      // remove the most recent watch expression and try again.
> +    if (this._currentEvaluation.throw || this._currentEvaluation.terminated) {

Perhaps update the above comment, as well.
Attachment #8456235 - Flags: review?(nfitzgerald) → review+
Attachment #754804 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c427c4b9b6f5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.