Closed
Bug 1262014
Opened 8 years ago
Closed 8 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)
DevTools
Debugger
Tracking
(firefox48 fixed, firefox49 fixed)
RESOLVED
FIXED
Firefox 49
People
(Reporter: KWierso, Assigned: jlast)
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 1 obsolete file)
4.77 KB,
patch
|
Details | Diff | Splinter Review |
Comment 1•8 years ago
|
||
Eddy, do you know what could be causing this? Its our #2 orange at the moment.
Flags: needinfo?(ejpbruel)
Comment 2•8 years ago
|
||
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•8 years ago
|
Assignee: ejpbruel → jlaster
Comment hidden (Intermittent Failures Robot) |
Updated•8 years ago
|
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) |
Comment 9•8 years ago
|
||
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•8 years ago
|
||
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 15•8 years ago
|
||
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•8 years ago
|
||
Attachment #8741802 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 20•8 years ago
|
||
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 21•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/86de03b543e9
Keywords: checkin-needed
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/86de03b543e9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•8 years ago
|
Target Milestone: Firefox 48 → Firefox 49
Comment 23•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b57ff9533be3351cf1540754b373a4de3b820baa
status-firefox48:
--- → fixed
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•