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

RESOLVED FIXED in Firefox 46

Status

P1
normal
RESOLVED FIXED
5 years ago
3 years ago

People

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

Tracking

unspecified
mozilla46
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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
Priority: -- → P1
Repro'd
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

5 years ago
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)
Flags: needinfo?(jgriffiths) → needinfo?(dtownsend+bugmail)
Irakli, does your windows/tabs refactoring work solve this?
Flags: needinfo?(dtownsend+bugmail)
(Reporter)

Comment 4

5 years ago
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?

Comment 5

5 years ago
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)
(Reporter)

Comment 7

5 years ago
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?

Comment 8

5 years ago
+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)
(Reporter)

Comment 11

5 years ago
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.
(Reporter)

Updated

5 years ago
Flags: needinfo?(rFobic)
(Reporter)

Comment 12

5 years ago
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
(Reporter)

Comment 13

5 years ago
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?

Comment 14

5 years ago
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
(Reporter)

Comment 22

5 years ago
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
(Reporter)

Comment 24

5 years ago
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
(Reporter)

Updated

4 years ago
Blocks: 821779
(Reporter)

Comment 27

4 years ago
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
(Assignee)

Comment 28

4 years ago
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.
(Assignee)

Comment 30

4 years ago
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+
(Assignee)

Comment 32

3 years ago
The pull request has been updated, please review. Thanks!
(Assignee)

Comment 33

3 years ago
Any chance to get the patch reviewed? If necessary, I can submit it through other means. Thanks in advance!
(Assignee)

Comment 34

3 years ago
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)

Comment 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f032711da68f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Thanks everybody!
You need to log in before you can comment on or make changes to this bug.