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

Status: RESOLVED FIXED

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

Description

[Ben McCann]

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

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Repro'd

Comment 2

[Ben McCann]

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.

Comment 3

[Dave Townsend [:mossop]]

Irakli, does your windows/tabs refactoring work solve this?

Comment 4

[Ben McCann]

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

[treyzilla48]

i've run into this as well. any update? pretty bad bug to a very core api

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Need-infoing Irakli, it looks like the logical thing to do here. 

Irakli, see comment 3.

Comment 7

[Ben McCann]

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

[treyzilla48]

+1 million

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Ping Mossop, this bug seems popular.

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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.

Comment 11

[Ben McCann]

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.

Comment 12

[Ben McCann]

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

Comment 13

[Ben McCann]

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

[Farooq Mela]

Attachment: Example add-on source that registers for and logs about tab events.

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.

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Thanks for the test case, we'll have someone to look into this!

Comment 16

[Dave Townsend [:mossop]]

Jordan, any chance you have time to take a look here?

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Checking it out

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

To be specific, this is only for popup windows, same issue as bug 922956, and because using `window.open`

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 20

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 22

[Ben McCann]

Hey Jordan, any progress on this one?

Comment 23

[Jordan Santell [:jsantell] [@jsantell]]

No progress yet, although there are a few `window.open` related issues -- probably need to do something on the platform side

Comment 24

[Ben McCann]

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?

Comment 25

[Jordan Santell [:jsantell] [@jsantell]]

Created bug 993015 for several `window.open` events

Comment 26

[Jordan Santell [:jsantell] [@jsantell]]

Unassigning myself from bugs I haven't gotten around to

Comment 27

[Ben McCann]

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

Comment 28

[Jacobo Aragunde Pérez]

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.

Comment 29

[Jacobo Aragunde Pérez]

Attachment: Pull request - Emit tabs open event for the first tab in a new window

Comment 30

[Jacobo Aragunde Pérez]

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 31

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 32

[Jacobo Aragunde Pérez]

The pull request has been updated, please review. Thanks!

Comment 33

[Jacobo Aragunde Pérez]

Any chance to get the patch reviewed? If necessary, I can submit it through other means. Thanks in advance!

Comment 34

[Jacobo Aragunde Pérez]

Hi, any chance to get the patch reviewed? Thanks!

Comment 35

[Jordan Santell [:jsantell] [@jsantell]]

Pinging Matteo

Comment 36

[Dietrich Ayala (:dietrich)]

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

Comment 37

[Jordan Santell [:jsantell] [@jsantell]]

Ping Matteo for r?

Comment 38

[Dietrich Ayala (:dietrich)]

https://twitter.com/dietrich/status/687195535841726464

Comment 39

[Matteo Ferretti [:zer0] [:matteo]]

r+, as mentioned in the comment in github. Thanks for the patch!

Mossop, could you merge this patch on m-c?

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f032711da68f

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Dietrich Ayala (:dietrich)]

Thanks everybody!