Last Comment Bug 799348 - [Firefox 16] load event is not always triggered by XUL windows
: [Firefox 16] load event is not always triggered by XUL windows
Status: VERIFIED FIXED
regressed by Bug 774633
: addon-compat, regression, relnote, verifyme
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 16 Branch
: x86_64 Windows 7
: -- normal with 3 votes (vote)
: mozilla19
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
: Manuela Muntean [Away]
Mentors:
: 809741 (view as bug list)
Depends on: 820338
Blocks: 774633
  Show dependency treegraph
 
Reported: 2012-10-08 22:12 PDT by Jeferson Hultmann
Modified: 2013-01-23 06:54 PST (History)
39 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
verified
+
verified
+
verified


Attachments
popup html (102 bytes, text/html)
2012-10-09 04:31 PDT, Alice0775 White
no flags Details
test.xpi (1.52 KB, application/x-xpinstall)
2012-10-09 10:13 PDT, Alice0775 White
no flags Details
popup html same domain (147 bytes, text/html)
2012-10-09 14:24 PDT, Alice0775 White
no flags Details
When creating a new content window, create the surrounding chrome docshell as system. v1 (5.51 KB, patch)
2012-10-10 12:06 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jeferson Hultmann 2012-10-08 22:12:58 PDT
Not sure if the "load" event doesn't fire or it is fired before the "domwindowopened" notification.

STR:

- paste the following code into Error Console:

Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
Components.utils.import("resource://gre/modules/Services.jsm");
Services.ww.registerNotification({
  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIObserver]),
  observe: function(win, topic, data) {
    Services.console.logStringMessage("observe " + topic);
    win.addEventListener("load", function(evt) {
      Services.console.logStringMessage("load");
    }, false);
  }
});


- open http://www.bbc.co.uk/news/health-19869673

- click on Twitter icon

- a popup will appear

- Fx15: "load" will appear in the Firefox error console

- Fx16/Nightly: no message will appear
Comment 1 Alice0775 White 2012-10-09 01:47:56 PDT
Regression window(Aurora channel)
Good
http://hg.mozilla.org/releases/mozilla-aurora/rev/c2cc63c864e4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120906042009
Bad:
http://hg.mozilla.org/releases/mozilla-aurora/rev/41eab5a0e537
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120907042009
Pushlog:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=c2cc63c864e4&tochange=41eab5a0e537


Regression window(Beta channel)
Good
http://hg.mozilla.org/releases/mozilla-beta/rev/dd8f72317c3f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120906 Firefox/16.0 ID:20120906142234
Bad:
http://hg.mozilla.org/releases/mozilla-beta/rev/6fbaf3aed7a0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120907 Firefox/16.0 ID:20120907112417
Pushlog:
http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=dd8f72317c3f&tochange=6fbaf3aed7a0



Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/3974efe8d584
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120905165101
Bad:
http://hg.mozilla.org/mozilla-central/rev/501f4e46a88c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120905192801
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e3e594837a30&tochange=75af94f85a98

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8d5589b88c8b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120905103804
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfc884e6e414
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120905113502
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d5589b88c8b&tochange=cfc884e6e414

In local build
Last Good: 32ecf0cec2c3
First bad: b33228bc231a

Regressed by:
b33228bc231a	Bobby Holley — Bug 774633 - Remove "is chrome window" condition for inner window reuse. r=jst WouldReuseInnerWindow also returns true if the new window is same-origin with the old one about:blank document. This condition exists in order to handle some sloppiness with respect to the principals on initial about:blank documents. Chrome callers sometimes parent chrome windows (with XUL document) to content windows. But this parenting causes us to push the cx of the content window during window creation, meaning that the subsequent load of chrome://foo.xul blows away the old inner window and any expandos on it. We can handle this case more precisely by skipping the cx push for type="chrome" windows. Furthermore, this was also necessary to prevent the inner window from being blown away in the call to SetOpenerScriptPrincipal once nsWindowWatcher gets the window back from the window creator (and after it's already told consumers about the window via "domwindowcreated"). But we fixed this nastiness in the previous patches. So we can remove this case. By doing so, we can prevent inner windows from ever changing origins, which is very important for compartment security invariants.
Comment 2 Alice0775 White 2012-10-09 04:31:13 PDT
Created attachment 669474 [details]
popup html

Add-ons such as adblock plus are affected.

Steps to reproduce: 
1. install https://addons.mozilla.org/en-US/firefox/addon/adblock-plus/
2. open attached popup html and click button
   --- observe add-ons bar

Actual results:
adblock plus icon does not appear in the add-ons bar

Expected results:
adblock plus icon should appear in the add-ons bar
Comment 3 Alex Keybl [:akeybl] 2012-10-09 06:03:52 PDT
We'll track for release, but we haven't heard of significant user impact over the last month. We'll have to hear of more critical regressions before deciding to take a fix in a 16.0.1.
Comment 4 Alex Keybl [:akeybl] 2012-10-09 06:04:33 PDT
Bobby - please prepare patches for all branches (including mozilla-release).
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-10-09 09:32:52 PDT
So, the bbc window does:

window.open("", "bbcshare_twitter", "width=550, height=420, status=no, resizable=yes, scrollbars=yes, toolbar=no, left=945, top=225");

We then fire domwindowopened, and the code in comment 0 attaches an "load" event listener to the window.

Then, we return to the script, which navigates that window to a twitter URL. That window is not same-origin, so we don't reuse the inner window and the event listener is blown away.

This all seems quite correct to me. It's not clear why this was working before, but it shouldn't, I don't think.

Alice, can you elaborate on the AdBlock breakage? Is AdBlock trying to do something similar?
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-09 09:45:02 PDT
(In reply to Bobby Holley (:bholley) from comment #5)
> We then fire domwindowopened, and the code in comment 0 attaches an "load"
> event listener to the window.
> 
> Then, we return to the script, which navigates that window to a twitter URL.
> That window is not same-origin, so we don't reuse the inner window and the
> event listener is blown away.

Isn't "win" in comment 0's code snippet a chrome window? The site navigating the contained content window shouldn't impact the chrome window's listener.
Comment 7 Alice0775 White 2012-10-09 10:13:18 PDT
Created attachment 669610 [details]
test.xpi

(In reply to Bobby Holley (:bholley) from comment #5)

> Alice, can you elaborate on the AdBlock breakage? Is AdBlock trying to do
> something similar?

I do not know a lot about adblock plus.

However, I can attach test.xpi which is add a button to nav-bar.

Whenever new browser window is opened up, a button should appear to nav-bar in Firefox15.

In firefox16+, bootstrap.js does not work as expected for the popup window.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-09 10:29:27 PDT
The "windowListener" code in that addon (jar:https://bug799348.bugzilla.mozilla.org/attachment.cgi?id=669610!/bootstrap.js) just does the same thing as comment 0, essentially.

That technique is very commonly used by bootstrapped addons: it's recommended for use by https://developer.mozilla.org/en-US/docs/Extensions/Mobile/Initialization_and_Cleanup#template_code and used in a helper library that's very widely used/copied: https://github.com/Mardak/restartless/blob/watchWindows/bootstrap.js .
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-10-09 12:01:22 PDT
(In reply to Bobby Holley (:bholley) from comment #5)
> This all seems quite correct to me. It's not clear why this was working
> before, but it shouldn't, I don't think.
We're not reusing inner window anymore?
EventListenerManager is per inner window.
Comment 10 Alice0775 White 2012-10-09 14:24:18 PDT
Created attachment 669735 [details]
popup html same domain
Comment 11 Alice0775 White 2012-10-09 14:27:44 PDT
The load event does not fire regardless of the domain of contents
Comment 12 Jeferson Hultmann 2012-10-09 19:00:09 PDT
This bug seems to exist only when ChromeWindow.opener != null
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-10-10 06:30:16 PDT
Ah, ok. So here's another stab at what's happening.

(1) The content calls window.open with flags such that a popup is created.
(2) in nsWindowWatcher::OpenWindowInternal, we correctly determine that we want to be creating a content window.
(2) We call nsAppStartup::CreateChromeWindow2, which calls nsXULWindow::CreateNewWindow, which calls nsXULWindow::CreateNewContentWindow.
(3) This first calls nsAppShellService::CreateTopLevelWindow, which creates a chrome window, And calls nsAppShellService::RegisterTopLevelWindow
(4) This calls SetInitialPrincipalToSubject, which replaces the about:blank chrome window with one of a different (non-system) principal, and then fires domwindowopened.
(5) We can't return the blank content window until browser.xul has loaded, so nsXULWindow::CreateNewContentWindow spins the event loop until that happens. Meanwhile, the load happens, and browser.xul needs to replace the about:blank viewer. Because browser.xul is system principal and the current about:blank document is not, we eject that document.

So the issue here is that we're creating the initial chrome about:blank document with the content caller's principal. We should be smarter and recognize that this window is about to have browser.xul, and use a system principal while creating it. I think SetInitialPrincipalToSubject is smart enough this can be accomplished with a simple null cx push. Working up a patch now.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-10-10 07:29:12 PDT
https://tbpl.mozilla.org/?tree=Try&rev=fd9920ab1a80
Comment 15 Alex Keybl [:akeybl] 2012-10-10 09:57:46 PDT
The impact here is too small as we understand it today, and the possibility of regression too large, to take a fix here in 16.0.1.
Comment 16 Kris Maglione [:kmag] 2012-10-10 11:14:46 PDT
Alex, what's the basis of your opinion that the impact is too small? Given how common this pattern is in add-ons, I'd expect it to affect millions of users.
Comment 17 Jorge Villalobos [:jorgev] 2012-10-10 11:38:34 PDT
The reasoning is that the bug is not that easy to reproduce, and it would only affect some add-ons and only on the affected windows. Also, we haven't heard any major complaints about it so far, even though the bug landed a while ago.
Comment 18 Jorge Villalobos [:jorgev] 2012-10-10 11:39:12 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #17)
> the bug landed a while ago.

the *fix* that regressed this landed a while ago.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-10-10 12:06:57 PDT
Created attachment 670069 [details] [diff] [review]
When creating a new content window, create the surrounding chrome docshell as system. v1

Looks green. Flagging bz for review.
Comment 20 Sebastian Zartner [:sebo] 2012-10-11 00:17:45 PDT
> Also, we haven't heard any major complaints about it so far, even though the bug landed a while ago.

Then you missed the complaints for Firebug:
http://code.google.com/p/fbug/issues/detail?id=5949

Sebastian
Comment 21 Hartmut Arlt 2012-10-11 01:33:53 PDT
+1 for a fix asap.

I'm a developer of an add-on that uses a similar technique. I really rely on it to work in order to be able to get notified when new windows are opened and loaded. IMHO, dropping the execution of a registered "load" listener is a serious issue.
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-10-11 02:13:31 PDT
Comment on attachment 670069 [details] [diff] [review]
When creating a new content window, create the surrounding chrome docshell as system. v1

Crap, I meant to flag bz for review on this, not mrbkap. mrbkap is on vacation.

FWIW I think this fix is low-risk.
Comment 23 Veiga 2012-10-11 06:49:16 PDT
Hi, I'm a web developer that relies on the window.open in a web application.
With this bug if I do, window.open("", "_blank", "channelmode=1");, I lose Firebug.
BTW, the new Developer Toolbar also doesn't seem to work in a new window created this way.
Comment 24 Johan 2012-10-11 07:47:35 PDT
Now Mozilla has withdrawn Fx 16 because it leaks URLs, is there any chance that this fix will come with the patch for that (in Firefox 16.0.1)?
Comment 25 Andrew McCreight [:mccr8] 2012-10-11 08:12:39 PDT
(In reply to Johan from comment #24)
> Now Mozilla has withdrawn Fx 16 because it leaks URLs, is there any chance
> that this fix will come with the patch for that (in Firefox 16.0.1)?

In comment 15, akeybl says they aren't going to take a patch for this issue in 16.0.1.
Comment 26 Jorge Villalobos [:jorgev] 2012-10-11 08:14:05 PDT
Firefox 16.0.1 won't include this fix. We might need to consider creating a 16.0.2, given the new evidence that this breaks Firebug and possibly our own Developer Tools.
Comment 27 Boris Zbarsky [:bz] 2012-10-11 10:28:08 PDT
Comment on attachment 670069 [details] [diff] [review]
When creating a new content window, create the surrounding chrome docshell as system. v1

r=me
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2012-10-11 10:45:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d177ffd4ba
Comment 29 Jorge Villalobos [:jorgev] 2012-10-11 14:22:25 PDT
(In reply to Veiga from comment #23)
> BTW, the new Developer Toolbar also doesn't seem to work in a new window
> created this way.

I can confirm that the Developer Toolbar doesn't show up at all in a new window opened with the testcase in this bug. The first-run popup that explains how it works does appear, but located outside of the window.

The Inspect Tool and Responsive Design Tool work partially. Some of their UI is missing. The Debugger seems to be working correctly.
Comment 30 Jeferson Hultmann 2012-10-11 20:31:05 PDT
Extension developers should be able to fix their code by using a "chrome-document-global-created" observer:

===8<---

Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
Components.utils.import("resource://gre/modules/Services.jsm");

var obs	= {
  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIObserver]),
  observe: function(win, topic,	data) {
    if (win.location.href !== "about:blank") { // bug 795961?
      win.addEventListener("load", function(evt) { 
	// load listener not necessary once bug 800677 is fixed
        var win = evt.currentTarget;
	Services.console.logStringMessage("url:	" + win.location);
      }, false);
    }
  }
};

Services.obs.addObserver(obs, "chrome-document-global-created",	false);
Comment 31 Kris Maglione [:kmag] 2012-10-11 20:40:12 PDT
chrome-document-global-created observers should definitely be the preferred way of doing this type of thing, yes, but expecting even a significant number of affected add-on authors to fix their code before this fix lands is unfortunately a pipe dream.

P.S. nsIObserver is a [function] interface. The QI is not necessary.
Comment 32 Ed Morley [:emorley] 2012-10-12 04:19:00 PDT
https://hg.mozilla.org/mozilla-central/rev/f6d177ffd4ba
Comment 33 Bobby Holley (:bholley) (busy with Stylo) 2012-10-15 03:13:37 PDT
Comment on attachment 670069 [details] [diff] [review]
When creating a new content window, create the surrounding chrome docshell as system. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 774633
User impact if declined: Broken addons. Given that were were considering chemspilling for this, I think we should at least get it on branches.
Testing completed (on m-c, etc.): Baked on m-c.
Risk to taking this patch (and alternatives if risky): Not super risky. No alternatives.
String or UUID changes made by this patch: None.
Comment 34 Alex Keybl [:akeybl] 2012-10-15 10:58:10 PDT
Comment on attachment 670069 [details] [diff] [review]
When creating a new content window, create the surrounding chrome docshell as system. v1

[Triage Comment]
Approving for branches, but maintaining the wontfix status of this bug for FF16.
Comment 36 the-edmeister 2012-10-20 17:37:20 PDT
Looks like this breaks RoboForm.

https://support.mozilla.org/en-US/questions/939398
Comment 37 Kevin Cox [:kevincox] 2012-11-08 06:34:42 PST
*** Bug 809741 has been marked as a duplicate of this bug. ***
Comment 38 Manuela Muntean [Away] 2012-11-12 02:20:52 PST
While going through the STR in comment 2, the problem is fixed on Firefox 17b5, Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0, Build ID: 20121106195758.

But the Developer Toolbar still doesn't show up at all (with Firefox 17b5) in a new window opened with the testcase in this bug (popup html).

In this situation, can I mark the bug verified, for status-firefox17 ?
Comment 39 Bobby Holley (:bholley) (busy with Stylo) 2012-11-12 11:32:36 PST
(In reply to Manuela Muntean from comment #38)
> While going through the STR in comment 2, the problem is fixed on Firefox
> 17b5, Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0,
> Build ID: 20121106195758.
> 
> But the Developer Toolbar still doesn't show up at all (with Firefox 17b5)
> in a new window opened with the testcase in this bug (popup html).
> 
> In this situation, can I mark the bug verified, for status-firefox17 ?

Well, sounds the developer toolbar is still broken, right? Maybe separate that out into a separate bug with STR and flag it for tracking?
Comment 40 Manuela Muntean [Away] 2012-11-13 04:09:57 PST
I've filed a new bug for the Developer Toolbar issue, bug 811254.
Comment 41 Manuela Muntean [Away] 2012-11-22 04:12:48 PST
Following STR from comment 2, this issue is fixed on Firefox 18 beta 1.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20121121075611
Comment 42 Manuela Muntean [Away] 2013-01-23 06:44:20 PST
Following STR from comment 2, this issue is fixed on Firefox 19 beta 2.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130116072953

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