Closed Bug 1262014 Opened 4 years ago Closed 4 years ago

Intermittent browser_dbg_stack-03.js | Test timed out - after DevToolsUtils.assert threw an exception: Error: Assertion failure: Should have an event loop. in browser_dbg_debugger-statement.js

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: KWierso, Assigned: jlast)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Eddy, do you know what could be causing this?  Its our #2 orange at the moment.
Flags: needinfo?(ejpbruel)
No, but I'll take a look at it next week.
Assignee: nobody → ejpbruel
Flags: needinfo?(ejpbruel)
Assignee: ejpbruel → jlaster
Summary: Intermittent browser_dbg_stack-03.js | Test timed out - after DevToolsUtils.assert threw an exception: Error: Assertion failure: Should have an event loop. → Intermittent browser_dbg_stack-03.js | Test timed out - after DevToolsUtils.assert threw an exception: Error: Assertion failure: Should have an event loop. in browser_dbg_debugger-statement.js
Jason said he wanted to take a stab at this, so I let him take the bug.
Priority: -- → P3
Attached patch intermittent.1.patch (obsolete) — Splinter Review
I believe the intermittent came from the stack frame scrolling, which at the wrong time could attempt to fetch call stack frames, while the debugger is resuming. This is fixed by stopping the scrolling before resuming the debugger.

There are also two small refactors:

1. refactored tests to use Task.spawn/yield
2. converted gThreadClient.resume/closeDebugger to resumeDebuggerThenCloseAndFinish
Attachment #8741802 - Flags: review?(ejpbruel)
Comment on attachment 8741802 [details] [diff] [review]
intermittent.1.patch

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

How did you figure this one out? I'm quite impressed by how quickly you managed to come up with a patch. Thanks for taking this bug off my plate!

On a side note, for small refactors that still touch on a lot of lines, such as refactoring a test to Task.jsm, I prefer it if you do that in a separate patch. If you put it all in a single patch it obscures the thing you were actually trying to fix.

::: devtools/client/debugger/test/mochitest/browser_dbg_stack-03.js
@@ +12,4 @@
>  
>  function test() {
>    initDebugger(TAB_URL).then(([aTab, aDebuggee, aPanel]) => {
> +    const gTab = aTab;

The g prefix is used for global variables. I never really liked that prefix to begin with, and since these variables are no longer global, you should probably just remove it.

@@ +21,3 @@
>  
> +    Task.spawn(function*() {
> +      gFramesScrollingInterval = window.setInterval(() => {

This looks like it doesn't have to be a global anymore either, does it?

@@ +46,5 @@
>          "Should have reached the recurse limit.");
>        is(gClassicFrames.itemCount, gDebuggee.gRecurseLimit,
>          "Should have reached the recurse limit in the mirrored view as well.");
>  
> +      window.clearInterval(gFramesScrollingInterval);

Please put a comment here explaining why this needs to happen in this specific order.
Attachment #8741802 - Flags: review?(ejpbruel) → review+
Attachment #8741802 - Attachment is obsolete: true
Keywords: checkin-needed
For future reference, the commit message in your patch should be summarizing what it's actually doing rather than restating the problem being fixed.
Flags: needinfo?(jlaster)
https://hg.mozilla.org/mozilla-central/rev/86de03b543e9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Target Milestone: Firefox 48 → Firefox 49
Thanks Ryan.
Flags: needinfo?(jlaster)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.