Closed Bug 840156 Opened 8 years ago Closed 8 years ago

Inspector doesn't gracefully handle onDOMReady event that fires after the inspector has been destroyed

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: dcrewi, Assigned: dcrewi)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch simplest fix possible (obsolete) — Splinter Review
Blocks: 734664
Attachment #712518 - Flags: review?(jwalker)
I removed code from the test that doesn't actually contribute to the bug and added comments.
Attachment #712517 - Attachment is obsolete: true
Attachment #712518 - Flags: review?(jwalker) → review?(paul)
Attachment #712760 - Flags: review?(paul)
Attachment #712518 - Flags: review?(scrapmachines)
Attachment #712760 - Flags: review?(scrapmachines)
Asking for an informal review to Optimizer to speed up my review.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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+
Attachment #712518 - Flags: review?(scrapmachines) → review+
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 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+
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)
Attachment #714966 - Flags: review?(paul) → review+
Attached patch combinedSplinter Review
Combined patch that I'll see if I can land today
Attachment #712518 - Attachment is obsolete: true
Attachment #714966 - Attachment is obsolete: true
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/508f98d86cf0
Assignee: nobody → dcrewi
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.