598600 - New tabs should be added to the last active group after restart

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Michael Monreal [:micmon]]

Attachment: Screenshot

Set Firefox to remember and restore tabs on restart. Have at least one tab group. This group should be empty or only have a blank tab (app tabs are fine to have). Now close and restart the browser and start one or two new tabs. Go to panorama view: the tabs are NOT created inside the tab group you would assume them to be in, they are not associated to any group and show up on top of each other on the bottom right and may even be covered by a tab group.

=> after restart, tabs should always be created in the last active tab group

Comment 1

[Michael Monreal [:micmon]]

More information about my usage pattern: I currently have 3 or 4 tab groups. One is the "session group" e.g. normal browsing, the others are special topic groups ("university", "fedora"). I often close all tabs of the "session group" before quitting. The group is still there, though, and creating new tabs before restarting the browser creates them in there correctly. However, after the restart firefox does not seem to remember this tab group was active.

Comment 2

[Michael Monreal [:micmon]]

FWIW I should also mention that this works fine as long as the session group has at least one non-app tab in it when closing.

Comment 3

[Ian Gilman [:iangilman]]

Raymond, please verify that your work on bug 595893 and friends fixes this scenario as well. This is probably a dupe of one of those bugs.

Comment 4

[Raymond Lee [:raymondlee]]

I see that this bug is describing two  things.

1) Orphan tabs appear after restart. This would be handled by Bug 595893
2) If the last active group has no non-app tab and after restart, new tab might be added to another group when there are more than one group.

Changed the bug for point 2.

Comment 5

[Raymond Lee [:raymondlee]]

The patch in bug

Comment 6

[Raymond Lee [:raymondlee]]

(In reply to comment #5)
> The patch in bug
Please ignore this

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 9

[Raymond Lee [:raymondlee]]

Comment on attachment 482483 [details] [diff] [review]
v1

Pushed to try. 6edcdd3f1f11

Comment 10

[Ian Gilman [:iangilman]]

This seems to be happening more lately. The fact that it happens at all after bug 595893 means there's something wrong with the patch. Since bug 595893 was a b7 blocker, nominating this as a b7 blocker as well.

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 482483 [details] [diff] [review]
v1

Nice catch!

The test doesn't really seem like it tests the bug (at least not as described in comment 0). On the other hand I'm not sure exactly how you would test that... you'd need to create a new window (which therefore hasn't yet loaded Panorama), stuff some group/tab data into it as if it had run Panorama before in a previous session, create some new tabs, and switch to Panorama (loading it up for the first time this session) and see if any of the tabs are orphaned. Sounds hard. 

Also: 

>-    GroupItems.groupItems.forEach(function(group) {
>-      group.close();
>-    });
>-    
>     let options = {
>       bounds: box,
>       immediately: true
>     };
>+
>     let groupItem = new GroupItem([], options);
>     let items = TabItems.getItems();
>     items.forEach(function(item) {
>       if (item.parent)
>         item.parent.remove(item);
>       groupItem.add(item, null, {immediately: true});
>     });
>-    
>+
>+    // ensure items are moved before closing the groups
>+    GroupItems.groupItems.forEach(function(group) {
>+      if (groupItem != group) 
>+        group.close();
>+    });

I don't think this change is necessary; closing all the groups ahead of time removes all of their tabs anyway.

Comment 12

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

(In reply to comment #11)
> The test doesn't really seem like it tests the bug (at least not as described
> in comment 0). On the other hand I'm not sure exactly how you would test
> that... you'd need to create a new window (which therefore hasn't yet loaded
> Panorama), stuff some group/tab data into it as if it had run Panorama before
> in a previous session, create some new tabs, and switch to Panorama (loading it
> up for the first time this session) and see if any of the tabs are orphaned.
> Sounds hard. 

Nah, not too hard. A little bit of listening for the state to be restored and that's about it.

> let state = { windows[{ tabs: [{ ..., extData:{ that stuff panorama does} }], ..., extData: { more panorma... } }] };
> let win2 = openDialog(...);
> sessionstore.setWindowState(win2, JSON.stringify(state), true);

Now you have your window set up with a bunch of tabs & panorama data that's never been accessed.

> win2.gBrowser.addTab()

Now you *should* have an orphaned tab. I never tried to repro with a new window, just starting firefox, so this might not even be an accurate test.

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: v1

* Updated the test using setWindowState().  Thanks Paul
* Added code to ensure that we are not saving group id after the group item is deleted, this prevents empty group being created after a group is closed.

Comment 14

[Ian Gilman [:iangilman]]

Comment on attachment 482791 [details] [diff] [review]
v1

Looking good. Just a couple comment nits:

>-    if (!this._inited) // too soon to save now
>+    if (!this._inited || this._uninited) // too soon to save now

Update the comment ("too soon/late to save" or something). 

>+  // set the preference of first run to true

This comment confused me at first; I thought it meant we were setting it as if it was the first run (until I read the code below it). Perhaps the comment could read something like "make sure we don't trigger the 'first run' behavior".

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: v1

f+=ian

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: v1

Fixed a type

f+=ian

Comment 17

[Dietrich Ayala (:dietrich)]

This is a re-emergence of a b7 blocker, which wasn't all the way fixed, as this bug is testament to. so this should block b7 as well.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 483070 [details] [diff] [review]
v1

>   // ----------
>   // Function: save
>   // Saves this groupItem to persistent storage.
>   save: function GroupItem_save() {
>-    if (!this._inited) // too soon to save now
>+    if (!this._inited || this._uninited) // too soon/late to save
>       return;

when would this happen?

i don't like the idea of callers being able call this and think it did something when it actually did not.

Comment 19

[Raymond Lee [:raymondlee]]

(In reply to comment #18)
> Comment on attachment 483070 [details] [diff] [review]
> v1
> 
> >   // ----------
> >   // Function: save
> >   // Saves this groupItem to persistent storage.
> >   save: function GroupItem_save() {
> >-    if (!this._inited) // too soon to save now
> >+    if (!this._inited || this._uninited) // too soon/late to save
> >       return;
> 
> when would this happen?
> 
> i don't like the idea of callers being able call this and think it did
> something when it actually did not.

When a new tab item is restored/created, it would go into a group (based on the session storage data); if it doesn't belong to any group, a new group would be created by GroupItems.newTab().  

In the UI.init(), there are some code (UI.reset()) which removes all groups and creates a new group for all tab items if it's first run for Panorama or no group data exists

When a new browser window is opened with browser tab(s), and go into Panorama UI,  tab item(s) and a group item would be created.  Since there is no group data, the UI.reset() would remove the new group and move all tab Items into the a new group.  However, GroupItem.save() is called in multiple places, so we run into a situation which the old group item id is being saved to the session storage but the group item has already removed by UI.reset().  When user restarts the browser, Panorama would restore a blank group which should not exist.

Comment 22

[Ian Gilman [:iangilman]]

Comment on attachment 483070 [details] [diff] [review]
v1

Reassigning review to Dolske, as Dietrich's away on vacation.

Comment 23

[Rob Campbell [:rc] (:robcee)]

additional STR:

1. restart
2. add a new tab
3. switch tabs
4. right click on tab
5. ctrl-tab back to previous tab

Results: Previous tab is selected, other tabs disappear from tab-strip. 

Expected: Nothing changed, switches back to previous tab.

Comment 25

[Ian Gilman [:iangilman]]

Here's a script you can run from your error console to pull all of your tabs (whether they've been grouped or are orphaned) into a single group: 

http://gist.github.com/637583

I could probably whip one up that only does it for orphaned tabs if anyone wants.

Comment 26

[Thomas Stache]

(In reply to comment #25)
> Here's a script you can run from your error console to pull all of your tabs
> (whether they've been grouped or are orphaned) into a single group: 
> 
> http://gist.github.com/637583
> 
> I could probably whip one up that only does it for orphaned tabs if anyone
> wants.

Thanks!
I tried it, and it does its job, but without updating the tab bar (you have to enter and exit TabView). Is it possible to add that to the script?

Comment 27

[Rob Campbell [:rc] (:robcee)]

this type of functionality would be pretty useful to have included in TabView itself. Maybe as a button somewhere.

Alternatively, a jetpack to add this function would be a cool trick.

Comment 28

[Thomas Stache]

cc'ing the reviewer...

Comment 29

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

(In reply to comment #26)
> (In reply to comment #25)
> > Here's a script you can run from your error console to pull all of your tabs
> > (whether they've been grouped or are orphaned) into a single group: 
> > 
> > http://gist.github.com/637583
> > 
> > I could probably whip one up that only does it for orphaned tabs if anyone
> > wants.
> 
> Thanks!
> I tried it, and it does its job, but without updating the tab bar (you have to
> enter and exit TabView). Is it possible to add that to the script?

Forked & added that. Not sure if there's a way with a single tabview call (I feel like there might be), but this just calls showTab for each tab in the window. http://gist.github.com/638855

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Dolske: really need this review. Can we get an ETA or can you toss it to someone else?

Comment 31

[Raymond Lee [:raymondlee]]

Attachment: patch for check-in

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Ian: we can haz checkin to mozilla-central default and the b7relbranch?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

I landed this on trunk:
https://hg.mozilla.org/mozilla-central/rev/e2124a8f65ed

It doesn't apply cleanly to the b7 relbranch. I'd like to avoid us doing the work of figuring out why and merging it, which would be easy to do if we decided to kill the relbranch now :)

Comment 34

[Ian Gilman [:iangilman]]

Word is we're no longer using the b7 branch (shipping from tip instead), so this is resolved.

Comment 35

[Tracy Walker [:tracy]]

verified with latest build of minefield 20101029

Comment 37

[Maniac Vlad Florin (:vladmaniac)]

Test added in Litmus https://litmus.mozilla.org/show_test.cgi?id=15225