Closed Bug 799348 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: XPConnect, defect)

16 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox16 + wontfix
firefox17 + verified
firefox18 + verified
firefox19 + verified

People

(Reporter: hultmann, Assigned: bholley)

References

Details

(4 keywords, Whiteboard: regressed by Bug 774633)

Attachments

(4 files)

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
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.
Whiteboard: regressed by Bug 774633
Version: Trunk → 16 Branch
Attached file 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
Component: Event Handling → DOM: Events
Blocks: 774633
Assignee: nobody → bobbyholley+bmo
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.
Bobby - please prepare patches for all branches (including mozilla-release).
Component: DOM: Events → XPConnect
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?
(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.
Attached file 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.
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 .
(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.
Attached file popup html same domain
The load event does not fire regardless of the domain of contents
This bug seems to exist only when ChromeWindow.opener != null
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.
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.
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.
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.
(In reply to Jorge Villalobos [:jorgev] from comment #17)
> the bug landed a while ago.

the *fix* that regressed this landed a while ago.
Looks green. Flagging bz for review.
Attachment #670069 - Flags: review?(mrbkap)
> 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
+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 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.
Attachment #670069 - Flags: review?(mrbkap) → review?(bzbarsky)
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.
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 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.
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 on attachment 670069 [details] [diff] [review]
When creating a new content window, create the surrounding chrome docshell as system. v1

r=me
Attachment #670069 - Flags: review?(bzbarsky) → review+
(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.
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);
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.
https://hg.mozilla.org/mozilla-central/rev/f6d177ffd4ba
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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.
Attachment #670069 - Flags: approval-mozilla-beta?
Attachment #670069 - Flags: approval-mozilla-aurora?
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.
Attachment #670069 - Flags: approval-mozilla-beta?
Attachment #670069 - Flags: approval-mozilla-beta+
Attachment #670069 - Flags: approval-mozilla-aurora?
Attachment #670069 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Looks like this breaks RoboForm.

https://support.mozilla.org/en-US/questions/939398
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 ?
(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?
I've filed a new bug for the Developer Toolbar issue, bug 811254.
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
QA Contact: manuela.muntean
Depends on: 820338
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.