799348 - [Firefox 16] load event is not always triggered by XUL windows

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Jeferson Hultmann]

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]

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]

Attachment: 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]]

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]]

Bobby - please prepare patches for all branches (including mozilla-release).

Comment 5

[Bobby Holley (:bholley)]

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: [e-mail]]]

(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]

Attachment: 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: [e-mail]]]

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][[e-mail]]]

(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]

Attachment: popup html same domain

Comment 11

[Alice0775 White]

The load event does not fire regardless of the domain of contents

Comment 12

[Jeferson Hultmann]

This bug seems to exist only when ChromeWindow.opener != null

Comment 13

[Bobby Holley (:bholley)]

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)]

https://tbpl.mozilla.org/?tree=Try&rev=fd9920ab1a80

Comment 15

[Alex Keybl [:akeybl]]

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]]

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] (he/him)]

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] (he/him)]

(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)]

Attachment: 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]]

> 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]

+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)]

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]

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]

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]]

(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] (he/him)]

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 [:bzbarsky]]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d177ffd4ba

Comment 29

[Jorge Villalobos [:jorgev] (he/him)]

(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]

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]]

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]]

https://hg.mozilla.org/mozilla-central/rev/f6d177ffd4ba

Comment 33

[Bobby Holley (:bholley)]

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]]

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 35

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc50cbd5f2bc
https://hg.mozilla.org/releases/mozilla-beta/rev/360ca36f8abe

Comment 36

[the-edmeister]

Looks like this breaks RoboForm.

https://support.mozilla.org/en-US/questions/939398

Comment 38

[Manuela Muntean [Away]]

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)]

(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]]

I've filed a new bug for the Developer Toolbar issue, bug 811254.

Comment 41

[Manuela Muntean [Away]]

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]]

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