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

RESOLVED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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=tim.taubert@gmx.de&rev=a2c07020625b

Changeset with the content script:

http://hg.mozilla.org/try/rev/93a2a922424f
Created attachment 552963 [details] [diff] [review]
patch v1

Exit windowInitializer() if the don't find a <browser> for the given contentWindow.
Attachment #552963 - Flags: feedback?(mihai.sucan)
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.
Attachment #552963 - Flags: feedback?(mihai.sucan) → feedback+
Comment on attachment 552963 [details] [diff] [review]
patch v1

FTR, passes try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=8393d99d36ba
Created attachment 553013 [details] [diff] [review]
patch v2

Test added.
Assignee: nobody → tim.taubert
Attachment #552963 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #553013 - Flags: review?(mihai.sucan)
Created attachment 553014 [details] [diff] [review]
patch v2

Sorry, wrong patch.
Attachment #553013 - Attachment is obsolete: true
Attachment #553013 - Flags: review?(mihai.sucan)
Attachment #553014 - Flags: review?(mihai.sucan)
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 <ttaubert@mozilla.com>
> + *
> + * ***** 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.
Attachment #553014 - Flags: review?(mihai.sucan) → review+
(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.
(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.
Created attachment 553765 [details] [diff] [review]
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! :)
Attachment #553014 - Attachment is obsolete: true
Thanks. That looks good!
http://hg.mozilla.org/integration/fx-team/rev/c0e7a77e62d8
Whiteboard: [fixed-in-fx-team]
http://hg.mozilla.org/mozilla-central/rev/c0e7a77e62d8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 9
You need to log in before you can comment on or make changes to this bug.