Closed Bug 939496 Opened 7 years ago Closed 4 years ago

tabs.on('open') fires when window is closed

Categories

(Add-on SDK Graveyard :: General, defect, P1)

defect

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: benjamin.j.mccann, Assigned: jaragunde, NeedInfo)

References

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.57 Safari/537.36

Steps to reproduce:

Create an extension which tracks tab opening

// Silence extraneous warnings
// https://groups.google.com/forum/?fromgroups=#!topic/mozilla-labs-jetpack/Wze1ouhyN4o
const preferences = require("sdk/preferences/service");
preferences.set('javascript.options.strict', false);

const tabs = require('sdk/tabs');
tabs.on('open', function(tab) {
  console.log('opened a tab');
});

Visit this page and click the link that says "Open a popup window"
http://www.quackit.com/html/codes/html_popup_window_code.cfm

Close the popup


Actual results:

"opened a tab" prints when you close the popup window


Expected results:

closing a window should not fire a tab opened event
Repro'd
Status: UNCONFIRMED → NEW
Ever confirmed: true
I really appreciate you guys confirming this bug and marking it as P1. Do you think there's someone we could assign this to? I'm really eager to get this one fixed as I have not been able to find any workaround despite lots of trying.
Flags: needinfo?(jgriffiths) → needinfo?(dtownsend+bugmail)
Irakli, does your windows/tabs refactoring work solve this?
Flags: needinfo?(dtownsend+bugmail)
Irakli, did you get a chance to test whether your windows/tabs refactoring solved this issue? This would be an excellent test case to add. Is there an open issue I could follow your work on?
i've run into this as well. any update? pretty bad bug to a very core api
Need-infoing Irakli, it looks like the logical thing to do here. 

Irakli, see comment 3.
Flags: needinfo?(rFobic)
This bug is really causing us lots of trouble and I still can't figure out any workaround. Is there anything we can do to have someone take a look at it?
+1 million
Ping Mossop, this bug seems popular.
Flags: needinfo?(dtownsend+bugmail)
There is pull request I have started for tabs / windows refactoring:
https://github.com/mozilla/addon-sdk/pull/1207

While parts of it has being landed with other work, pull still needs some
work (also it's likely out of date).

Ben if you want to submit a test case, that would really help, we can
fix failing test with new or existing implementation.
Flags: needinfo?(rFobic)
Given that it appears I'd only have to write JS, there's a chance I'd be able to create a test for this. First challenge would be just running the existing test suite. Do you recommend Windows? I usually use Ubuntu, but only see Windows mentioned in the README (https://github.com/mozilla/addon-sdk), so I could switch to that if it's more used by the Mozilla dev team and easier to get running. What command would I use to run the tests?

Should I put the new test in test-window-events.js? (Or maybe test-tab-events.js?) Also, will I be able to easily both open a window to a given webpage and then open a link within that webpage? If I can get references to all the correct objects then hopefully it shouldn't be too hard to test. It's very easy to reproduce manually.
Flags: needinfo?(rFobic)
Figured out how to run the tests against my install of FF26:
git checkout -b firefox26 origin/firefox26
./bin/cfx testpkgs --filter test-tab-events
The tab events test (https://github.com/mozilla/addon-sdk/blob/master/test/test-tab-events.js) is currently calling utils.openTab in order to open a new tab. I need to click a link within that tab for the unit test. Is there a way I can access the DOM from the object returned by utils.openTab in order to click the link?
This attachment demonstrates the issue. Use the add-on SDK to run the add-on, and navigate to http://www.benmccann.com/ff939496/popup.html and click the "click me" link. You should see the popup window appear and the console will show the following events:

console.log: test-ext: tab ready: http://www.benmccann.com/ff939496/index.html
console.log: test-ext: tab pageshow: http://www.benmccann.com/ff939496/index.html

Close the popup window, and the following events will appear:

console.log: test-ext: tab open: http://www.benmccann.com/ff939496/popup.html
console.log: test-ext: tab activate: http://www.benmccann.com/ff939496/popup.html
console.log: test-ext: tab close: http://www.benmccann.com/ff939496/popup.html
console.log: test-ext: tab activate: http://www.benmccann.com/ff939496/index.html#

The bug appears to be that none of the tab events for the popup fire until AFTER the tab is closed.
Thanks for the test case, we'll have someone to look into this!
Flags: needinfo?(rFobic)
Jordan, any chance you have time to take a look here?
Flags: needinfo?(dtownsend+bugmail) → needinfo?(jsantell)
Checking it out
Flags: needinfo?(jsantell)
Assignee: nobody → jsantell
To be specific, this is only for popup windows, same issue as bug 922956, and because using `window.open`
Irakli, weren't you working on a platform patch that allowed an event to bubble up for the opening of a first "tab" in a new window? Might be related.
Flags: needinfo?(rFobic)
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #19)
> Irakli, weren't you working on a platform patch that allowed an event to
> bubble up for the opening of a first "tab" in a new window? Might be related.

I don think that's related, patch I was working was just exposing first tab init event, which platform never triggered and it was assumed that new window contains single tab.
Flags: needinfo?(rFobic)
Right, and that's what `window.open` does -- creates a new window containing one tab except it's specified as whatever url is in window.open
Hey Jordan, any progress on this one?
No progress yet, although there are a few `window.open` related issues -- probably need to do something on the platform side
Jordan, maybe it would be nice to link all those bugs together so that we could see if there is any pattern and address them together?
Created bug 993015 for several `window.open` events
Unassigning myself from bugs I haven't gotten around to
Assignee: jsantell → nobody
Blocks: sdk/tabs
I just tested this again. Since https://bugzilla.mozilla.org/show_bug.cgi?id=922956 is fixed in Firefox 40 I was hoping this would be as well. No luck.

Firefox 39 - "opened a tab" is printed when popup is closed
Firefox 40 - "opened a tab" is never printed for popup
This bug also happens with right click -> open in new window in FF 42 (nightly). It doesn't look it was present in FF 39.
I think the original bug is gone but now we have two different issues:

* tabs.ready event is not emitted for the first tab in a new window. Fixed in the pull request above.
* tabs events in general are not emitted after window.open(). This is actually part of bug #989288. I'll take a look into that one.
Comment on attachment 8643661 [details] [review]
Pull request - Emit tabs open event for the first tab in a new window

Looks good to me, r+ if all tests are passing against the last master.
Attachment #8643661 - Flags: review+
The pull request has been updated, please review. Thanks!
Any chance to get the patch reviewed? If necessary, I can submit it through other means. Thanks in advance!
Hi, any chance to get the patch reviewed? Thanks!
Pinging Matteo
Flags: needinfo?(zer0)
Hi Jacobo! We'll try and find someone to help get this over the finish line. Have you confirmed that the unit tests pass with this patch? Have you updated the tests to make sure they fail without this patch? And if you don't know how to run the tests, also let us know and we'll point you in the right direction ;)
Flags: needinfo?(jaragunde)
Ping Matteo for r?
r+, as mentioned in the comment in github. Thanks for the patch!

Mossop, could you merge this patch on m-c?
Flags: needinfo?(zer0) → needinfo?(dtownsend)
Assignee: nobody → jaragunde
Flags: needinfo?(dtownsend)
https://hg.mozilla.org/mozilla-central/rev/f032711da68f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Thanks everybody!
You need to log in before you can comment on or make changes to this bug.