628188 - "tabs" Module conflicts with Panorama: groups getting mangled for the last few nightly updates

Status: RESOLVED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

over the last few days, when i update to the nightly build, my tabcandy groups get messed up.

once, almost all tabs were put into one group. another time, the set of tabs in group B were all in group A, and an equivalent number from A were moved into B.

i made an add-on that acts on TabOpen events, so i need to confirm for sure that the addon isn't interacting badly with Panorama.

Comment 1

[Vykintas Vyšniauskas]

I was going to confirm this, since I've been seeing this for a couple of days too. But firstly I decided to check if it's not an extension's fault and it turned out it was happening because of the "Hard Blockers Counter 0.8" extension.

In case it matters, the behavior was similar to that described your comment:
After startup everything is fine, but after entering panorama all or most tabs ended up being in the first group. After disabling the addon, TWO browser restarts are needed to fix the problem.

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110124 Firefox/4.0b10pre

Comment 2

[Vykintas Vyšniauskas]

P.S. the up to date "Hard Blockers Counter 1.0.1" does not cause this issue.

Comment 3

[Vykintas Vyšniauskas]

Sorry for bugspam, but actually even the latest version of the addon causes the issue...

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

can you post steps to reproduce?

Comment 5

[Vykintas Vyšniauskas]

There probably is shorter way to achieve this but that's what I did:

1. Start Minefield with a clean profile
2. Set Minefield to load previous session on startup
3. Install Hard Blockers Counter 1.0.1 from AMO
4. Have 4 tabs with something loaded
5. Restart Minefield (not sure if it's necessary)
6. Open Panorama
7. Create a new group and move one tab from the first group
8. Create one more group and move a tab from the first group
9. Select a tab from the first group
10. Restart Minefield
11. Notice that 2 tabs are shown in the tab bar
12. Open Panorama
13. Notice that 3 (sometimes 4) tabs are shown in the first group
14. Choose a tab from the first group
15. Notice that 3 or 4 tabs are shown on the tab bar

Seems to be reproducible always on my machine. Works OK without the addon.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

reproduced.

Simplified steps:
1. Minefield with 4 tabs open and HBC extension
2. Create two new groups in panorama and move one tab to each
3. Restart browser
4. Enter Panorama mode

Current behavior:
One tab from one of the new groups is moved to the default group and becomes visible after step 4 when viewing default group.

What's interesting is that after I disable HBC and restart the browser I still can reproduce it until I restart the browser yet again. It seems like if the tabs were pinned to the wrong groups and I need to manually pin a tab to the right one with HBC disabled for it to stick.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

I tested if the Widget itself causes it but it seems that it is not Widget module.

We should try to minimize it - I suspect that maybe widget+panel, timer or simpleStorage may be triggering it.

Comment 8

[Kevin Hanes]

Should this bug be closed? Repurposed with a more specific summary?

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I've been hitting this too, but glad that I can blame something now.

Gandalf: you used the addon-sdk for your extension, right? You have some suspicions here about what might be causing it, so I'm CC'ing Myk in case he has any ideas.

(In reply to comment #8)
> Should this bug be closed? Repurposed with a more specific summary?

Closed, no. Renamed/moved, maybe. It could be something from the addon-sdk misbehaving or an interaction that is somehow mishandled by panorama.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

wondering if this may be related to bug 625988 ...

Comment 11

[Dietrich Ayala (:dietrich)]

(In reply to comment #10)
> wondering if this may be related to bug 625988 ...

I never use PB, and my groups are remixed almost every time I restart the browser.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Dietrich: I didn't mean that PB causes it, but that there may be a common factor involved.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

I wanted to minimize the testcase, but after an update to latest nightly on mac (4.0b11pre 2011-01-26) I cannot reproduce it anymore.

Can you confirm it still exists?

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

I was wrong in my previous comment - the bug still exists but it's sometimes hard to trigger it.

I was able to minimize my testcase in order to reproduce it. It was as simple as: 

main.js:
"let tabs = require("tabs")"

This line triggers the bug. The extension I used to test is stored here:

https://builder.addons.mozilla.org/addon/1000097/latest/

And I'll attach an .xpi with updated maxVersion to test against latest nightlies.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: testcase .xpi

minimized extension that triggers this bug.

Note: When you're reproducing the bug it is sometimes required to remove all additional panorama groups, restart browser, add new groups, drag&drop tabs to reinitialize the bug.

Also, when you disable the extension it takes two cycles (restart, enter panorama, drag&drop back to the second group, restart ...) to stop triggering this bug.

That causes me to suspect that some code responsible for attaching tab to the given group doesn't work due to the Jetpack module "tabs" being initialized.

Comment 19

[Justin Dolske [:Dolske]]

I tried disabling the "Hard Blockers Counter" addon, which was the only recent addon I remember adding, but this is still killing me every time I restart the browser.

Comment 20

[Dietrich Ayala (:dietrich)]

(In reply to comment #19)
> I tried disabling the "Hard Blockers Counter" addon, which was the only recent
> addon I remember adding, but this is still killing me every time I restart the
> browser.

What other add-ons do you have?

Comment 21

[Les Orchard [:lorchard]]

(In reply to comment #19)
> I tried disabling the "Hard Blockers Counter" addon, which was the only recent
> addon I remember adding, but this is still killing me every time I restart the
> browser.

FWIW: This happened for me with *any* Jetpack / Add-on SDK based addon. It took disabling all addons and re-enabling one-by-one after restarts to narrow that down, though. So, not knowing what other add-ons you might have running, I'd say try that. It's a pain, but less pain than having the tab groups shuffle

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Pull request:
https://github.com/mozilla/addon-sdk/pull/103

Comment 23

[Scream]

1

Comment 24

[Dietrich Ayala (:dietrich)]

(In reply to comment #22)
> Pull request:
> https://github.com/mozilla/addon-sdk/pull/103

thanks for tracking this down Felipe. can you attach the patch here and ask adw for review?

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Small change from the pull request, fixed an existing strict mode warning (missing "let" in for loop) and updated doc to clarify the close event behavior.

Grabbing text from the the pull request:
----
The tabs module conflicts with panorama because it manually calls tab.close() when a window is closed, which interferes with the browser itself closing them. With this change we'll just emit the onclose event instead, which was the original purpose of calling tab.close().

There was another problem in which only half of the tabs were receiving the onclose event due to elements being removed from the tabs list during an iteration on the list.
Tests included for both changes

Comment 26

[Drew Willcoxon :adw]

Comment on attachment 509548 [details] [diff] [review]
Patch

I don't understand the tabs and windows code at all and am having a hard time trying to follow it.  r+'s for everyone!

This comment on line 116 of addon-kit/lib/windows.js isn't accurate anymore, so could you update or remove it?

  // Need to remove all the tabs before window listener are notified.

Comment 27

[:Felipe Gomes (needinfo for replies!)]

pull request for checkin, with the outdated comment removed.

https://github.com/mozilla/addon-sdk/pull/106

Comment 28

[Drew Willcoxon :adw]

Thanks Felipe!

https://github.com/mozilla/addon-sdk/commit/2496d77232d02dd130c815b615eea9c8c8f2fb49

https://github.com/mozilla/addon-sdk/commit/7beedfce2986784887456c076ca0b8fe7b72d1d8