Closed
Bug 1258333
Opened 8 years ago
Closed 8 years ago
browser_animation_timeline_rate_selector.js has unhandled promise rejections
Categories
(DevTools :: Inspector: Animations, defect, P1)
DevTools
Inspector: Animations
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jryans, Assigned: pbro)
References
Details
(Whiteboard: [btpp-fix-now])
Attachments
(1 file)
See a partial try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=47af3faac576. It appears to fail consistently. I have saw it fail on at least all Linux runs before terminating the try run. Please note you need to apply the patch in bug 1240804 to see the test failures. The failure log includes: A promise chain failed to handle a rejection: - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox-highlighter-utils.js:271 - TypeError: toolbox is null
Assignee | ||
Comment 1•8 years ago
|
||
I'll take a look at this one.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Assignee | ||
Comment 2•8 years ago
|
||
I found out the problem. Bug 1161072 introduced a race in the toolbox destroy code. In that bug, we made it so that we wouldn't wait for the promise returned by unhighlight (in toolbox-highlighter-utils.js) to resolve. I forget the specifics, but that means that there are chances that the toolbox gets destroyed fully before the async part in unhighlight is done, and since the rest of the code in unhighlight uses the toolbox, this may fail. It fails in the particular test because the test overrides some the highlighter's functions and so the timing is changed.
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42037/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42037/
Attachment #8733937 -
Flags: review?(zer0)
Updated•8 years ago
|
Attachment #8733937 -
Flags: review?(zer0) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8733937 [details] MozReview Request: Bug 1258333 - Prevent errors when unhighlighting when the toolbox is closed; r=zer0 https://reviewboard.mozilla.org/r/42037/#review38713 r+, but the comment below considered – if there is reason why the check it's good as is, maybe it should be written in a comment too; it doesn't seems obvious otherwise. ::: devtools/client/framework/toolbox-highlighter-utils.js:271 (Diff revision 1) > if (isNodeFrontHighlighted && forceHide && toolbox.highlighter && isRemoteHighlightable()) { > isNodeFrontHighlighted = false; > yield toolbox.highlighter.hideBoxModel(); > } > > + // unhighlight is called when destroying the toolbox, which means that by I'm not familiar with this code, but if if it's true what you said in your comment, then we should probably check that `toolbox` is still assigned before check `toolbox.highlighter` in line 266. Also, in that case, maybe it doesn't makes sense execute the whole body function if there is no `toolbox` assigned, so we could returns at the beginning?
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #4) > I'm not familiar with this code, but if if it's true what you said in your > comment, then we should probably check that `toolbox` is still assigned > before check `toolbox.highlighter` in line 266. unhighlight is only ever called synchronously when destroying the toolbox, so toolbox can't be null on that line. However, after we've done `yield toolbox.highlighter.hideBoxModel();` line 268, toolbox could by now be null, which is why I checked it there only.
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4289f3efb309&group_state=expanded
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e4ccb1f574d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•