678816 - web console re-attaches console to non-tab-browser contentWindows

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Panorama has a webProgressListener in a content script. That seems to get notified of a starting document load (NetworkPanel.xhtml). The webProgressListener accesses webProgress.DOMWindow which is lazily created and this sends "content-document-global-created" which in turn calls HUD_Service.windowInitializer() with the contentWindow from the NetworkPanel iframe. So the web console is re-attached and stops working or throws errors.

windowInitializer() fails to retrieve a valid <browser> for the given contentWindow so we should maybe return at this point? I don't know if this breaks anything that should work but it fixes my tests failures.

Try run with test failures:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=a2c07020625b

Changeset with the content script:

http://hg.mozilla.org/try/rev/93a2a922424f

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Exit windowInitializer() if the don't find a <browser> for the given contentWindow.

Comment 2

[Mihai Sucan [:msucan]]

Comment on attachment 552963 [details] [diff] [review]
patch v1

Explanation and fix make sense. Feedback+!

Good work on finding the problem!

Nit: please use curly brackets to wrap the return.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 552963 [details] [diff] [review]
patch v1

FTR, passes try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=8393d99d36ba

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Test added.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Sorry, wrong patch.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 553014 [details] [diff] [review]
patch v2

Review of attachment 553014 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks fine. r+ with the comments below addressed.

Thanks for your fix!

::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_678816.js
@@ +5,5 @@
> + *
> + * Contributor(s):
> + *  Tim Taubert <[e-mail]>
> + *
> + * ***** END LICENSE BLOCK ***** */

Please use the PD license boilerplate:

http://www.mozilla.org/MPL/boilerplate-1.1/

@@ +26,5 @@
> +  browser.contentWindow.location.reload();
> +}
> +
> +function tabLoad2(aEvent) {
> +  browser.removeEventListener(aEvent.type, arguments.callee, true);

Please do not use arguments.callee (for future es5 strict mode compat).
(the same comment applies in the networkPanelShown() event handler as well)

@@ +34,5 @@
> +  document.addEventListener("popupshown", networkPanelShown, false);
> +
> +  // Send the mousedown and click events such that the network panel opens.
> +  EventUtils.sendMouseEvent({type: "mousedown"}, outputItem);
> +  EventUtils.sendMouseEvent({type: "click"}, outputItem);

Doesn't EventUtils.synthesizeMouse() work here?

@@ +56,5 @@
> +  finishTest();
> +}
> +
> +function test() {
> +  messageManager.loadFrameScript(FRAME_SCRIPT_URI, true);

The global frame script is never removed/disabled.

Since there's no way to unload frame scripts, just send it a "kill" signal which tells the content script to unregister the progress listener and remove all the variables it defined.

::: browser/devtools/webconsole/test/browser/test-bug-678816-content.js
@@ +1,1 @@
> +let ifaceReq = docShell.QueryInterface(Ci.nsIInterfaceRequestor);

As far as I know (from gavin) content scripts do not share scope. Where is Ci defined? Also where was XPCOMUtils imported?

(do content scripts share scope?)

@@ +15,5 @@
> +  // ----------
> +  // Implements progress listener interface.
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
> +                                         Ci.nsISupportsWeakReference,
> +                                         Ci.nsISupports])

Afaik, nsISupports is not needed here, it's added automatically.

@@ +19,5 @@
> +                                         Ci.nsISupports])
> +};
> +
> +// add web progress listener
> +webProgress.addProgressListener(WebProgressListener, Ci.nsIWebProgress.NOTIFY_STATE_ALL);

This script needs to implement a kill signal, to remove the progress listener, as suggested above.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mihai Sucan [:msucan] from comment #6)
> > +  // Send the mousedown and click events such that the network panel opens.
> > +  EventUtils.sendMouseEvent({type: "mousedown"}, outputItem);
> > +  EventUtils.sendMouseEvent({type: "click"}, outputItem);
> 
> Doesn't EventUtils.synthesizeMouse() work here?

Works.

> The global frame script is never removed/disabled.
> 
> Since there's no way to unload frame scripts, just send it a "kill" signal
> which tells the content script to unregister the progress listener and
> remove all the variables it defined.

Good point! Totally forgot to do that. Done.

> As far as I know (from gavin) content scripts do not share scope. Where is
> Ci defined? Also where was XPCOMUtils imported?

They share scope. See bug 673569. We should fix this test after that one landed or else we get some re-definition errors now.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > > +  // Send the mousedown and click events such that the network panel opens.
> > > +  EventUtils.sendMouseEvent({type: "mousedown"}, outputItem);
> > > +  EventUtils.sendMouseEvent({type: "click"}, outputItem);
> > 
> > Doesn't EventUtils.synthesizeMouse() work here?
> 
> Works.
> 
> > The global frame script is never removed/disabled.
> > 
> > Since there's no way to unload frame scripts, just send it a "kill" signal
> > which tells the content script to unregister the progress listener and
> > remove all the variables it defined.
> 
> Good point! Totally forgot to do that. Done.

Great! Looking forward to see the updated patch. Thanks!

> > As far as I know (from gavin) content scripts do not share scope. Where is
> > Ci defined? Also where was XPCOMUtils imported?
> 
> They share scope. See bug 673569. We should fix this test after that one
> landed or else we get some re-definition errors now.

Hah, not nice. Too bad frame scripts share scope.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

(In reply to Mihai Sucan [:msucan] from comment #8)
> Great! Looking forward to see the updated patch. Thanks!

Sounds like you want to see that patch again before I push it - here it is! :)

Comment 10

[Mihai Sucan [:msucan]]

Thanks. That looks good!

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/c0e7a77e62d8

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/c0e7a77e62d8