Closed
Bug 840156
Opened 12 years ago
Closed 12 years ago
Inspector doesn't gracefully handle onDOMReady event that fires after the inspector has been destroyed
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 22
People
(Reporter: dcrewi, Assigned: dcrewi)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
I think this is the source of the test failure caused by the patch in bug 734664. InspectorPanel#onNavigatedAway sets a listener for onDOMReady, but that listener is not cleaned up in InspectorPanel#destroy. This causes a reference to null in the case when the inspector is destroyed soon after there has been navigation by the target but before the new page has finished loading.
I'll attach a browser mochitest that shows the problem and the simplest fix I could think of.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #712518 -
Flags: review?(jwalker)
Assignee | ||
Comment 3•12 years ago
|
||
I removed code from the test that doesn't actually contribute to the bug and added comments.
Attachment #712517 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #712518 -
Flags: review?(jwalker) → review?(paul)
Updated•12 years ago
|
Attachment #712760 -
Flags: review?(paul)
Updated•12 years ago
|
Attachment #712518 -
Flags: review?(scrapmachines)
Updated•12 years ago
|
Attachment #712760 -
Flags: review?(scrapmachines)
Comment 4•12 years ago
|
||
Asking for an informal review to Optimizer to speed up my review.
Updated•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•12 years ago
|
||
Comment on attachment 712760 [details] [diff] [review]
browser mochitest that demonstrates the problem
Review of attachment 712760 [details] [diff] [review]:
-----------------------------------------------------------------
Apart from the funky use of .then(), everything is good.
Attachment #712760 -
Flags: review?(scrapmachines) → review+
Updated•12 years ago
|
Attachment #712518 -
Flags: review?(scrapmachines) → review+
Comment 6•12 years ago
|
||
Comment on attachment 712518 [details] [diff] [review]
simplest fix possible
I've tried other approaches. This one sounds like the best one.
Attachment #712518 -
Flags: review?(paul) → review+
Comment 7•12 years ago
|
||
Comment on attachment 712760 [details] [diff] [review]
browser mochitest that demonstrates the problem
Please move this test in `devtools/inspector/test`.
r=me with this change.
Attachment #712760 -
Flags: review?(paul) → review+
Assignee | ||
Comment 8•12 years ago
|
||
I had to copy the "addTab" functionality from browser/devtools/framework/test/head.js since it's missing in the inspector tree.
Attachment #712760 -
Attachment is obsolete: true
Attachment #714966 -
Flags: review?(paul)
Updated•12 years ago
|
Attachment #714966 -
Flags: review?(paul) → review+
Comment 9•12 years ago
|
||
Combined patch that I'll see if I can land today
Attachment #712518 -
Attachment is obsolete: true
Attachment #714966 -
Attachment is obsolete: true
Updated•12 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
Assignee: nobody → dcrewi
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•