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

RESOLVED FIXED in Firefox 48

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: KWierso, Assigned: jlast)

Tracking

({intermittent-failure})

unspecified
Firefox 49

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Assignee

Updated

3 years ago
Assignee: ejpbruel → jlaster
Comment hidden (Intermittent Failures Robot)
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
Comment hidden (Intermittent Failures Robot)
Jason said he wanted to take a stab at this, so I let him take the bug.
Priority: -- → P3
Comment hidden (Intermittent Failures Robot)
Assignee

Comment 11

3 years ago
Posted 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 hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
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+
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Assignee

Comment 19

3 years ago
Attachment #8741802 - Attachment is obsolete: true
Assignee

Updated

3 years ago
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)

Comment 22

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/86de03b543e9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Target Milestone: Firefox 48 → Firefox 49
Assignee

Comment 24

3 years ago
Thanks Ryan.
Flags: needinfo?(jlaster)

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.