594644 - Return from Private Browsing Mode, Panorama forgets tab groups, until Panorama is manually re-launched

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[ecchinut]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5) Gecko/20100101 Firefox/4.0b5
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5) Gecko/20100101 Firefox/4.0b5

Create a bunch of Tab Groups in Panorama. Now enter Private Browsing Mode. Now Exit Private Browsing Mode. All the tabs will be constrained to one window, ignoring the groups that were created prior to going to into Private Browsing Mode. Clicking on Panorama will re-create the groups - but this step shouldn't be necessary - it should preserve the tab groups after coming back. Tested on: Mac OS X Snow Leopard 10.6.4 Firefox 4 Beta 5 AND Windows XP Firefox 4 Beta 5 - seems to happen on both.

Reproducible: Always

Steps to Reproduce:
1. Make a bunch of tab groups
2. Enter Private Browsing Mode
3. Exit Private Browsing Mode
Actual Results:  
All the tab groups created in step 1 are ignored, and instead are now in one window. Clicking the Panorama button causes the groups to be re-created automatically, but users shouldn't have to perform this step.

Expected Results:  
Exiting private browsing mode, the previous tab groups should still persist.

Comment 1

[juan becerra [:juanb]]

I can confirm this, but it might be a dupe of another bug.

Comment 2

[Daniel Veditz [:dveditz]]

I assume Juan hit the wrong blocking drop-down by mistake.

Comment 3

[Laxminarayan G Kamath A]

I confirm this too. I also noticed the tab groups other than current are forgotten, and each of all other tabs become individual groups when entering tabview.

Comment 4

[Laxminarayan G Kamath A]

also noticed page loading/parsing get paused till I switch to them after coming out of the tabview.

Comment 7

[Ian Gilman [:iangilman]]

Basically we're not doing the right thing with sessionrestore when transitioning to/from PB.

As part of fixing this, we should also make sure that we're clearing out all of our data when going into/out of PB. 

Notes from Ehsan on how to test this:

Also, the test needs to make sure that about:privatebrowsing is actually shown
when you enter the PB mode, and the correct session (with all the
windows/tabs/tabview data) is restored when you exit this mode.  In addition,
the test needs to make sure that when you enter the PB mode, all of the tabview
specific data is cleared (along with the rest of the sessionstore data).

Comment 8

[Ian Gilman [:iangilman]]

More from Ehsan: 

Also, PB uses sessionstore's getBrowserState and setBrowserState APIs behind
the scenes, so we need to make sure that what tabview does with sessionstore is
correctly reflected in those APIs as well.  I'm kind of certain that it's the
case, but we need to have tests to make sure of that.

Comment 9

[Ian Gilman [:iangilman]]

Attachment: patch v1 (requires 595371, 595374, 600741, 587031)

Needs test; otherwise good.

Comment 10

[(no longer active)]

Nothing in the patch seems crazy to me, but I don't think that I don't get the rationale, and comment 7 doesn't really help much.  Can you please explain in detail what exactly was wrong before this patch, how it lead to the symptoms in comment 0, and how your patch rectifies the problem?  Thanks!

Comment 11

[(no longer active)]

Comment on attachment 481672 [details] [diff] [review]
patch v1 (requires 595371, 595374, 600741, 587031)

Cleared the feedback request pending on clarifications on the patch.

Comment 12

[Ian Gilman [:iangilman]]

Comment on attachment 481672 [details] [diff] [review]
patch v1 (requires 595371, 595374, 600741, 587031)

Explanation for Ehsan. 

It all comes down to the fact that we weren't restoring groups on sessionrestore (which is part of PB). If you don't name your groups, they'll go away automatically when all their tabs are removed (which happens when going into PB). When we return from PB, since we weren't restoring the groups, they'd be gone; thus the symptoms in comment 0. Therefore, the fix is this one line: 

>     // session restore
>     function srObserver(aSubject, aTopic, aData) {
>       if (aTopic != "sessionstore-browser-state-restored")
>         return;
>         
>+      GroupItems.load();

The rest of the patch is refactoring to make that one line work (the load routine didn't exist yet, for instance). 

TabItem.missingGroupID is introduced to deal with the fact that we'll be loading the groups after the tabs, rather than before-hand like at startup. 

Does that clarify?

Comment 13

[(no longer active)]

Comment on attachment 481672 [details] [diff] [review]
patch v1 (requires 595371, 595374, 600741, 587031)

Makes more sense now.  Thanks!

Comment 14

[Raymond Lee [:raymondlee]]

There is another comment about this bug in bug 595893#22

Comment 15

[Ian Gilman [:iangilman]]

Attachment: patch v2 (requires 590268, 598600, 598795)

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 484471 [details] [diff] [review]
patch v2 (requires 590268, 598600, 598795)

>   // Function: add
>   // Adds an item to the groupItem.
>   // Parameters:
>   //
>   //   a - The item to add. Can be an <Item>, a DOM element or an iQ object.
>   //       The latter two must refer to the container of an <Item>.
>-  //   dropPos - An object with left and top properties referring to the location dropped at.  Optional.
>-  //   options - An object with optional settings for this call. Currently this includes dontArrange
>-  //       and immediately
>+  //   dropPos - An object with left and top properties referring to the 
>+  //             location dropped at.  Optional.
>+  //   options - An object with optional settings for this call. Currently
>+  //             this includes dontArrange and immediately.

document if it's optional or not, and point to where the options params are documented.

>   // Function: remove
>   // Removes an item from the groupItem.
>   // Parameters:
>   //
>   //   a - The item to remove. Can be an <Item>, a DOM element or an iQ object.
>   //       The latter two must refer to the container of an <Item>.
>-  //   options - An object with optional settings for this call. Currently this includes
>-  //             dontArrange and immediately
>+  //   options - An object with optional settings for this call. See below.
>+  //
>+  // Possible options: 
>+  //   dontArrange - don't rearrange the remaining items
>+  //   dontClose - don't close the group even if it normally would
>+  //   immediately - don't animate

document whether it's optional or not

>   // Function: removeAll
>   // Removes all of the groupItem's children.
>-  removeAll: function GroupItem_removeAll() {
>-    var self = this;
>-    var toRemove = this._children.concat();
>+  // The "options" param is passed to each remove call. 
>+  removeAll: function GroupItem_removeAll(options) {

ditto

>+    let groupItemsData = Storage.readGroupItemsData(gWindow);
>+    let groupItemData = Storage.readGroupItemData(gWindow);
>+    this.reconstitute(groupItemsData, groupItemData);
>+    this.killNewTabGroup(); // temporary?

can you explain that last line? i see it's moved from below, but what's it for?

r=me with these nits fixed

Comment 17

[Ian Gilman [:iangilman]]

> document if it's optional or not, and point to where the options params are
> documented.

Done in all three cases.

> >+    this.killNewTabGroup(); // temporary?
> 
> can you explain that last line? i see it's moved from below, but what's it for?

We used to have a "new tab" group, which is where all of your new tabs would go. This was stored along with all of the other groups in people's sessionstore, so when we got rid of it, we needed a method to clean it out of old sessionstores. It's been long enough now, we can get rid of it. I've added it as a rider to bug 609163.

Comment 18

[Ian Gilman [:iangilman]]

Landed on panorama-central:

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/804452c60d27

Comment 19

[ruiderson]

Hey Guys, I have similar issues about this. After making groups in panorama, and starting private browsing, I have APPCRASH issues in minefield, or when I exit private browsing, I have this issues too. Sometimes after exit private browsing, minefield don't get appcrash, but forgot all tabs and groups created before, only keeping the pined app tab, wich turns back to "normal/unpined tab" again.

Thanks!

Comment 21

[(no longer active)]

(In reply to comment #18)
> Landed on panorama-central:
> 
> http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/804452c60d27

Is there any ETA on merging panorama-central to mozilla-central?

Comment 22

[Ian Gilman [:iangilman]]

(In reply to comment #21)
> Is there any ETA on merging panorama-central to mozilla-central?

This patch has a gnarly test breakage, that I'm hoping bug 607016 will fix...

Comment 23

[(no longer active)]

(In reply to comment #22)
> (In reply to comment #21)
> > Is there any ETA on merging panorama-central to mozilla-central?
> 
> This patch has a gnarly test breakage, that I'm hoping bug 607016 will fix...

So, that bug has r+, which means that it can land on panorama-central to see if it fixes the test failures here, right?

Comment 24

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

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Is there any ETA on merging panorama-central to mozilla-central?
> > 
> > This patch has a gnarly test breakage, that I'm hoping bug 607016 will fix...
> 
> So, that bug has r+, which means that it can land on panorama-central to see if
> it fixes the test failures here, right?

Or run locally? The test in that patch currently fails. I'll have a new patch up later today though (just test changes, code won't be changing).

Comment 25

[Ian Gilman [:iangilman]]

(In reply to comment #23)
> So, that bug has r+, which means that it can land on panorama-central to see if
> it fixes the test failures here, right?

I've now tried it, and it doesn't fix it; I've posted more info to bug 607016. Paul's looking into it.

Comment 28

[Ian Gilman [:iangilman]]

Attachment: fix1

This additional patch fixes test failures associated with the original patch due to the fact that we were manipulating the tabbar during the private browsing transition.

Comment 29

[Ian Gilman [:iangilman]]

Attachment: fix2

This additional patch fixes another test failure with the original patch, having to do with our sessionstore access timing. Note that it also requires the patches from bug 615394 and bug 605935.

Comment 30

[Dietrich Ayala (:dietrich)]

Comment on attachment 496034 [details] [diff] [review]
fix1

this looks fine, r=me.

Comment 31

[Ian Gilman [:iangilman]]

Comment on attachment 496035 [details] [diff] [review]
fix2

This patch has now been moved to its own bug (bug 617511); marking it as obsolete here.

Comment 32

[Kevin Hanes]

Beta8 has 1 bug left on it, moving our blockers to b9

Comment 34

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/b06a94065ef0

http://hg.mozilla.org/mozilla-central/rev/ccfc9d214703

Comment 36

[George Carstoiu]

Issue present (Win7, Ubuntu, WinXP) with minor change in steps:

1. Make a bunch of tab groups
2. Enter Private Browsing Mode
3. Exit Firefox
4. Start Firefox

Actual results:
 - all tabs are presented as being part of same group

Expected results:
 - tabs are restored as being part of separate groups

Observation:
 - by going to Panorama the groups are restored to normal

Comment 38

[Tracy Walker [:tracy]]

verified with nightly build of minefield.

Comment 39

[Robert Augustin]

I am encountering this exact bug in FF 13.0.1 - unfortunately it doesn't seem to be resolved.

Comment 40

[(no longer active)]

(In reply to Robert Augustin from comment #39)
> I am encountering this exact bug in FF 13.0.1 - unfortunately it doesn't
> seem to be resolved.

Can you please file a new bug for this?

Comment 41

[Robert Augustin]

(In reply to Ehsan Akhgari [:ehsan] from comment #40)
> (In reply to Robert Augustin from comment #39)
> > I am encountering this exact bug in FF 13.0.1 - unfortunately it doesn't
> > seem to be resolved.
> 
> Can you please file a new bug for this?

Will do, thanks.