Last Comment Bug 678816 - web console re-attaches console to non-tab-browser contentWindows
: web console re-attaches console to non-tab-browser contentWindows
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 9
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-14 05:51 PDT by Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
Modified: 2011-08-18 12:35 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (999 bytes, patch)
2011-08-14 05:56 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
mihai.sucan: feedback+
Details | Diff | Splinter Review
patch v2 (28.13 KB, patch)
2011-08-14 13:28 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
patch v2 (5.98 KB, patch)
2011-08-14 13:30 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
mihai.sucan: review+
Details | Diff | Splinter Review
patch v3 (6.12 KB, patch)
2011-08-17 07:40 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-14 05:51:51 PDT
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
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-14 05:56:10 PDT
Created attachment 552963 [details] [diff] [review]
patch v1

Exit windowInitializer() if the don't find a <browser> for the given contentWindow.
Comment 2 Mihai Sucan [:msucan] 2011-08-14 08:37:19 PDT
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] (on PTO, back Aug 29th) 2011-08-14 11:32:43 PDT
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
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-14 13:28:54 PDT
Created attachment 553013 [details] [diff] [review]
patch v2

Test added.
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-14 13:30:19 PDT
Created attachment 553014 [details] [diff] [review]
patch v2

Sorry, wrong patch.
Comment 6 Mihai Sucan [:msucan] 2011-08-16 05:22:23 PDT
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.
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-16 14:39:14 PDT
(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] 2011-08-17 03:20:58 PDT
(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] (on PTO, back Aug 29th) 2011-08-17 07:40:22 PDT
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! :)
Comment 10 Mihai Sucan [:msucan] 2011-08-17 07:57:36 PDT
Thanks. That looks good!
Comment 11 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-17 14:07:47 PDT
http://hg.mozilla.org/integration/fx-team/rev/c0e7a77e62d8
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-18 12:35:10 PDT
http://hg.mozilla.org/mozilla-central/rev/c0e7a77e62d8

Note You need to log in before you can comment on or make changes to this bug.