Closed Bug 594644 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P3, minor)

x86
All

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: ecchinut, Assigned: iangilman)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
OS: Mac OS X → All
I can confirm this, but it might be a dupe of another bug.
Status: UNCONFIRMED → NEW
blocking1.9.2: --- → ?
Ever confirmed: true
Priority: -- → P3
I assume Juan hit the wrong blocking drop-down by mistake.
blocking1.9.2: ? → ---
status2.0: --- → ?
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.
also noticed page loading/parsing get paused till I switch to them after coming out of the tabview.
Duplicate of this bug: 599034
Duplicate of this bug: 598571
blocking2.0: --- → betaN+
status2.0: ? → ---
Blocks: 598154
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).
Assignee: nobody → ian
Blocks: 597043
No longer blocks: 598154
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.
Needs test; otherwise good.
Attachment #481672 - Flags: feedback?(ehsan)
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 on attachment 481672 [details] [diff] [review]
patch v1 (requires 595371, 595374, 600741, 587031)

Cleared the feedback request pending on clarifications on the patch.
Attachment #481672 - Flags: feedback?(ehsan)
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?
Attachment #481672 - Flags: feedback?(ehsan)
Comment on attachment 481672 [details] [diff] [review]
patch v1 (requires 595371, 595374, 600741, 587031)

Makes more sense now.  Thanks!
Attachment #481672 - Flags: feedback?(ehsan) → feedback+
There is another comment about this bug in bug 595893#22
Status: NEW → ASSIGNED
Attachment #481672 - Attachment is obsolete: true
Attachment #484471 - Flags: review?(dietrich)
Depends on: 590268, 598600, 598795
Attachment #484471 - Attachment is patch: true
Attachment #484471 - Attachment mime type: application/octet-stream → text/plain
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
Attachment #484471 - Flags: review?(dietrich) → review+
> 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.
Landed on panorama-central:

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/804452c60d27
Whiteboard: [on-panorama-central]
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!
Blocks: 588597
No longer blocks: 588597
Blocks: 588597
Duplicate of this bug: 599044
Depends on: 611330
(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?
(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...
(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?
(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).
(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.
Duplicate of this bug: 616886
Depends on: 615394
Attached patch fix1Splinter Review
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.
Attachment #496034 - Flags: review?(dietrich)
Attached patch fix2 (obsolete) — Splinter Review
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.
Attachment #496035 - Flags: review?(dietrich)
Comment on attachment 496034 [details] [diff] [review]
fix1

this looks fine, r=me.
Attachment #496034 - Flags: review?(dietrich) → review+
Blocks: 605935
No longer depends on: 615394
Attachment #496035 - Flags: review?(dietrich)
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.
Attachment #496035 - Attachment is obsolete: true
Blocks: 598154
Beta8 has 1 bug left on it, moving our blockers to b9
No longer blocks: 597043
Duplicate of this bug: 618540
http://hg.mozilla.org/mozilla-central/rev/b06a94065ef0

http://hg.mozilla.org/mozilla-central/rev/ccfc9d214703
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [on-panorama-central]
Depends on: 623867
Duplicate of this bug: 621579
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
Duplicate of this bug: 626269
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
I am encountering this exact bug in FF 13.0.1 - unfortunately it doesn't seem to be resolved.
(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?
(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.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.