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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[David Creswick]

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.

Comment 1

[David Creswick]

Attachment: browser mochitest that showcases the problem

Comment 2

[David Creswick]

Attachment: simplest fix possible

Comment 3

[David Creswick]

Attachment: browser mochitest that demonstrates the problem

I removed code from the test that doesn't actually contribute to the bug and added comments.

Comment 4

[Paul Rouget [:paul]]

Asking for an informal review to Optimizer to speed up my review.

Comment 5

[Girish Sharma [:Optimizer]]

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.

Comment 6

[Paul Rouget [:paul]]

Comment on attachment 712518 [details] [diff] [review]
simplest fix possible

I've tried other approaches. This one sounds like the best one.

Comment 7

[Paul Rouget [:paul]]

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.

Comment 8

[David Creswick]

Attachment: browser mochitest, moved to where it should be

I had to copy the "addTab" functionality from browser/devtools/framework/test/head.js since it's missing in the inspector tree.

Comment 9

[Joe Walker [:jwalker]]

Attachment: combined

Combined patch that I'll see if I can land today

Comment 10

[Joe Walker [:jwalker]]

https://hg.mozilla.org/integration/fx-team/rev/508f98d86cf0

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/508f98d86cf0