Many tab/window events do not work with `window.open` -- need to expose this somehow to solve the several issues that depend on this.

What exactly do you mean here? Windows opened with window.open should send the same kinds of observer and DOM events as any other window

(In reply to Dave Townsend (:Mossop) from comment #1) > What exactly do you mean here? Windows opened with window.open should send > the same kinds of observer and DOM events as any other window They should, but they don't -- a lot of issues related to not getting any window or tab events for opening a new window/tab using `window.open`. In general, try to steer developers to use something other than `window.open`[1], but this keeps coming up, and `window.open` gets a lot of use. [1] https://developer.mozilla.org/en-US/docs/Web/API/Window.open#Avoid_resorting_to_window.open.28.29

Thank you for filing this bug Jordan. It's very helpful to see all the issues tied together in one place. I have to mention that avoiding window.open is simply impossible as an extension developer. Our extension can currently be broken by any site on the internet since we have no control over the content of the sites out there, unless Firefox wants to remove that method :-)

Avoiding window.open is very inconvenient for developers as stated above. Please fix this as soon as possible. Thank you.

This is a HUGE issue!! Please fix as soon as possible!

What I already mentioned during the meeting that one thing that can cause this is that when you open a window it starts loading about:blank first, and I'm afraid we hook up the listeners to this one, and when the actual content starts being loaded this about:blank inner getting destroyed, leaving the new inner without listeners. This is just a theory, but if someone could point me to the code where we attach the listeners I could try to validate it. Do we use progress listeners? Because those should work. Also, a simplified version of what the SDK code does, that can be executed in scratch-pad, worth gold here if we are out of ideas.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6) > What I already mentioned during the meeting that one thing that can cause > this is that when you open a window it starts loading about:blank first, and > I'm afraid we hook up the listeners to this one, and when the actual content > starts being loaded this about:blank inner getting destroyed, leaving the > new inner without listeners. I don't know, it could be that we're simply using the wrong events. I just had the feeling, from the people's comments, that previously it worked and therefore something was changed in the Firefox platform, but it could be also that no one experienced that before. > Also, a simplified version of what the SDK code does, that can be executed > in scratch-pad, worth gold here if we are out of ideas. In short: we use a `WindowTracker` to keep trace of the windows opened: when a window opened is a browser window, we attach all the listeners we need (for tabs, etc). The problem: when a window is opened using `window.open` from a content script, the only window we get is the DOM Window, so it's not a browser window and therefore the events are not attached. I think this code already shows what is our issue: let observerService = Components.classes["@mozilla.org/observer-service;1"]. getService(Components.interfaces.nsIObserverService); let observer = { observe : (aSubject) => console.log(aSubject) } observerService.addObserver(observer, "toplevel-window-ready", false); If you open a new page from UI (menu or cmd/ctrl+n), you will see that the observer log a ChomeWindow. Even if we use a `windowWatcher.openWindow`. But if a window is opened via `window.open` from a content window, the only window logged is a (DOM) Window. You can test the last case here: http://www.benmccann.com/ff939496/

Notice that one thing I was trying, was check if the window obtained was a browser window: if it's not, try to get the browser window from that window; but I wasn't able to get it.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8) > Notice that one thing I was trying, was check if the window obtained was a > browser window: if it's not, try to get the browser window from that window; > but I wasn't able to get it. I just tried a couple of approaches; also I wasn't sure it was the right way to proceed anyway, it was merely to figure out the bug. But it could be a possible solution, I guess.

Could you please take a look at Comment 6 and tell me if this is a bug or if we're just doing something wrong? Seems like the difference between window.open and ctrl+n lays somewhere here: http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#712 But that just tells me that we get a different type of window from CreateChromeWindow2 in the two cases, and our way of attaching listeners works only in one case (where it returns an nsXULWindow). But I have no idea what could we do in the other (window.open) case.

I'm not sure which "this" comment 10 is talking about, but let me describe what window.open does. What happens with window.open is that we either create a new Firefox window or add a tab to an existing one. Either way, we have a new tab to work with. That tab typically synthesizes an about:blank document whose principal is the same as the code that called window.open. This allows the page that called window.open to immediately have a window to work with. Then window.open starts the load of the URI it was given, if any. When that load gets a server response, we look at the principal the resulting page would have. If that principal matches the principal of our about:blank, then we reuse the inner window and put a new document inside it. Any listeners that were registered on that window stay. So if you do this: var win = window.open("some-same-origin-URI"); win.addEventListener("load", something); this will call "something" when the load finishes. If the principal of the newly loaded page does _not_ match the principal of the about:blank, we throw out the old inner window and create a new one. In this situation listeners registered on the old window will go away with it. Does that help at all?

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #11) > I'm not sure which "this" comment 10 is talking about, but let me describe > what window.open does. Right, sorry for being vague. So the trick is that we are trying to find the tabbrowser window from the window we got from the observer-service and attach Tab event listeners (TabOpen, TabMove, TabClose) to it, but in case of window.open, we got a DOM window instead of a xul window so gBrowser is null on it, so we have no way to get a reference to the tabbrowser window it belongs too, so we cannot attach the listeners. What can we do to attach the tab event listeners in this case? > > If the principal of the newly loaded page does _not_ match the principal of > the about:blank, we throw out the old inner window and create a new one. In > this situation listeners registered on the old window will go away with it. > > Does that help at all? Yes this is exactly what I thought is our problem here, but after talking to Matteo, it seems that we have another problem I described above. So we don't want to attach listeners to the DOM window but to the tabbrowser window above it, which we cannot find.

> from the window we got from the observer-service and attach Tab event > listeners (TabOpen, TabMove, TabClose) to it, but in case of window.open, we > got a DOM window instead of a xul window so gBrowser is null on it, in case there isn't a simple method we can call to get that, maybe we could simply iterate over all existing tabs, get the content window and compare to the one we got? it's not ideal, but opening a tab/window is already a heavy operation that wont be called dozens of times per second..

Which observer notification are you listening for? Are these window.open() calls that are opening a new toplevel window or opening a new tab?

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #14) > Which observer notification are you listening for? Are these window.open() > calls that are opening a new toplevel window or opening a new tab? "toplevel-window-ready" and window.open opens a new toplevel window (but I think we will have to cover both cases).

I see. "toplevel-window-ready" is a slightly bizarre puppy, and only used by the addon sdk.... Anyway, you can go from the window that's passed to the XUL window by getting the docshell and going up the docshell parent chain, right?

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #16) > Anyway, you can go from the window that's passed to the XUL window by > getting the docshell and going up the docshell parent chain, right? Hmm... This can be the missing information we're looking for :) Matteo, could you give this approach a try and report back if that works or not?

As I said in comment 8, I tried briefly to get that window, but I wasn't able to do so – I tried the methods we have in window/utils, like `getXULWindow`, `getTopLevelWindow` etc. (https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/window/utils.js#L102) but without success Boris, if you could provide the code to get the ChromeWindow from the Window given, I will appreciate!

getTopLevelWindow() looks to me like it should do the right thing. What does it return in this case?

(In reply to Boris Zbarsky [:bz] from comment #19) > getTopLevelWindow() looks to me like it should do the right thing. What > does it return in this case? Sorry, my bad. The method itself actually returned a ChromeWindow, but it didn't work as expected – or, better, it's still behave in the same way as now, when we don't attach any listeners 'cause it's a DOM Window – I didn't check the middle step. So, even if we get the ChromeWindow from that DOM Window, and then we get the `tabContainer` where we listen for event such `TabClose`, `TabOpen` and `TabSelect`, they seems not emitted, never. Actually, they're emitted once the popup window is closed – see bug 939496 –, exactly how it happens now. As said previously, using `windowWatcher.openWindow` instead, works as expected.

> but it didn't work as expected OK, why not? This seems like the thing to figure out, I'd think.

(In reply to Boris Zbarsky [:bz] from comment #21) > > but it didn't work as expected Are you working on this, or want me to help and debug something on platform side for you? In that case could you give me something that I can easily debug?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22) > (In reply to Boris Zbarsky [:bz] from comment #21) > > > but it didn't work as expected > > Are you working on this, or want me to help and debug something on platform > side for you? That would be helpful, thanks. We could maybe work together tomorrow about that, what do you think? I'll ping you on IRC. > In that case could you give me something that I can easily debug? I'll try to expand the code I gave above, so you'll be able to test on scratchpad without SDK dependencies.

Hey guys, just wanted to let you know how extremely extremely grateful I am to have you looking at this. It's been so critical for us to enable our browser extension to work at all and I'd really love to see people enjoying our extension in FF as much as they do in Chrome. Hope you guys are able to connect and continue making great progress. Thanks so much for the help

Thanks again so much for looking at this guys. Matteo, do you think you could post the code example you were referring to creating?

It seems I'm not completely able to reproduce a simple test case from scratchpad; that could means that there is definitely something in the SDK itself that gives – or help – the current behavior. I'll dig more into the SDK, then, and see what other differences we have from a vanilla JS scratchpad code.

Matteo, could you post what you tried in the scratchpad that worked correctly and did not reproduce the problem? I'm not sure I'd make any progress if you didn't, but I'd certainly be willing to try reproducing this as well and that could help me since I'm rather unfamiliar with the SDK internals. I'm going to send you guys a big cake or a case of your favorite beer or something when we get this fixed!

Just wanted to check in on this again to see if there's been any progress or if you could post the code that attempts and fails to reproduce the problem. Thanks!

Hey Ben, I'm on it, I'll let you know as soon as I have some good result!

Thanks Matteo! Really really appreciate you looking at it!!!

Hey Matteo, this one fell off my radar for a bit, but I'm still really interested in it. Do you happen to have any tips in case I try looking at it?

Hey Matteo, I assume you're not still looking at this. Any tips if we try taking it over?

Jacobo is going to look at fixing this. He just got in a test to show that 922956 is fixed now and is working on creating tests for some of the other possible incarnations of this issue. Any tips to share on looking at this?

We have a PR for this if someone could review. Thanks! https://github.com/mozilla/addon-sdk/pull/2028

Oops, wrong link. PR with fix is here: https://github.com/mozilla/addon-sdk/pull/2030

Matteo, would you be able to take a look at the PR with the fix?

Pinging for this

Comment on attachment 8666939 [details] pr 2030 Looks good to me, I would also add tests for multiple dom windows, but it's r+ with all tests passed on recent master and with the once / async tests.

Tests are passing now. Please take another look

Hi, reviews appreciated on the following PRs, which should fix this family of bugs: https://github.com/mozilla/addon-sdk/pull/2028 https://github.com/mozilla/addon-sdk/pull/2030 Thanks in advance.

There's a fix for this. Could someone please review? Thanks! https://github.com/mozilla/addon-sdk/pull/2030

They're now review both; and PR 2028 is already landed on m-c. Ben, Jacobo, once both of them are landed, it means that also bug 942286, 989288, 989527 and 1003988 will be fixed? Thanks!

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #42) > They're now review both; and PR 2028 is already landed on m-c. Ben, Jacobo, > once both of them are landed, it means that also bug 942286, 989288, 989527 > and 1003988 will be fixed? Thanks! Bug 942286 -> these patches should fix it, but would need a test Bug 989288 -> fixed, test included in PR 2030 Bug 989527 -> fixed, test included in PR 2030 Bug 1003988 -> probably a duplicate of this one

Mossop, could you land this fix in m-c, so we can also mark as fixed Bug 989288 and Bug 989527? Thanks!

I shouldn't need to be the gate here. Here is all I do to land these things: hg import -p 1 --prefix addon-sdk/source -u "<user (github mangles UTF8 characters)>" -m "<commit message>" <PR URL>.patch https://hg.mozilla.org/integration/fx-team/rev/46fd541b3440

(In reply to Dave Townsend [:mossop] from comment #46) > I shouldn't need to be the gate here. Here is all I do to land these things: > hg import -p 1 --prefix addon-sdk/source -u "<user (github mangles UTF8 > characters)>" -m "<commit message>" <PR URL>.patch Sorry to bother you, it's still unclear to me the contributors workflow nowadays. I don't think I have commit access like I had in github repository: once the PR is converted into a patch, should I just attach it on the relative bug and then add the "checkin-needed" flag?

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #47) > (In reply to Dave Townsend [:mossop] from comment #46) > > > I shouldn't need to be the gate here. Here is all I do to land these things: > > > hg import -p 1 --prefix addon-sdk/source -u "<user (github mangles UTF8 > > characters)>" -m "<commit message>" <PR URL>.patch > > Sorry to bother you, it's still unclear to me the contributors workflow > nowadays. > > I don't think I have commit access like I had in github repository: once the > PR is converted into a patch, should I just attach it on the relative bug > and then add the "checkin-needed" flag? Oh huh, yeah that would work or yeah you can just bug me or anyone with commit access in that case. I had assumed you had commit access, you could always request that too!

Priority: confirmed