Last Comment Bug 688695 - Deferred session restore doesn't behave correctly for a single tab group
: Deferred session restore doesn't behave correctly for a single tab group
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Tim Taubert [:ttaubert]
:
: Mike de Boer [:mikedeboer]
Mentors:
Depends on: 706430
Blocks: 627642 705964
  Show dependency treegraph
 
Reported: 2011-09-22 23:14 PDT by tabmix.onemen
Modified: 2012-04-20 14:17 PDT (History)
1 user (show)
ttaubert: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (2.54 KB, patch)
2011-10-10 15:21 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
ttaubert: review-
Details | Diff | Splinter Review
Patch v0.2 (3.39 KB, patch)
2011-10-11 02:52 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
proposed patch (3.44 KB, text/plain)
2011-10-11 06:17 PDT, tabmix.onemen
ttaubert: feedback-
Details
patch v3 (3.43 KB, patch)
2011-11-28 18:31 PST, Tim Taubert [:ttaubert]
paul: review+
Details | Diff | Splinter Review

Description tabmix.onemen 2011-09-22 23:14:39 PDT
look like a typo in _prepWindowToRestoreInto

we get tabview-group data change group id to zero and set the modified data into tabview-groups !!!

> let data = this.getWindowValue(aWindow, "tabview-group");
...
...
> this.setWindowValue(aWindow, "tabview-groups", JSON.stringify(data));
Comment 1 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-10 14:41:33 PDT
Even worse, just correcting the typo doesn't do what it's supposed to. Patch coming soon.
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-10 15:21:50 PDT
Created attachment 566054 [details] [diff] [review]
Patch v0.1

I changed what we look at entirely - we're now looking at tabview-groups instead of tabview-group. This seems like it should be a bit quicker & less tricky (no using Object.keys anymore). It seems to work just fine as well.
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-10 15:23:16 PDT
Comment on attachment 566054 [details] [diff] [review]
Patch v0.1

Tim, can you take a look at this too. I'm tricking Panorama in a way it wasn't before.
Comment 4 Tim Taubert [:ttaubert] 2011-10-11 02:49:39 PDT
Comment on attachment 566054 [details] [diff] [review]
Patch v0.1

Review of attachment 566054 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1630,4 @@
>          //XXXzpao This is a hack and the proper fix really belongs in Panorama.
> +        if (groupsData.activeGroupId != 0) {
> +          groupsData.activeGroupId = 0;
> +          this.setWindowValue(aWindow, "tabview-groups", JSON.stringify(groupsData));

That's unfortunately not working - we only use the activeGroupId to activate the right group after all groups were restored. Merging works only if both groupIds are the same. So you have to create two groups and use the second to store your tabs in (for the restored session) to make it fail. This behavior is now even worse than before because now we get an "orphan" tab instead of a second group.

We have an orphan tab because that has a non-existant groupID which we can't find a group for. So it's just not assigned to any group. I think that case should be handled by Panorama. I'll attach a patch with those changes (I needed to try that so I figured I could give you that patch right away ;).
Comment 5 Tim Taubert [:ttaubert] 2011-10-11 02:52:06 PDT
Created attachment 566157 [details] [diff] [review]
Patch v0.2

We should maybe write a test for this scenario, too? Shouldn't be that hard I think.
Comment 6 tabmix.onemen 2011-10-11 06:17:23 PDT
Created attachment 566185 [details]
proposed patch

There are 2 main problem with the function GroupItems_reconstitute, i'v attached a file with a patch to the function that i believe solves the problems

1st: we have to set tabItem._reconnected = false to All item that were exist before the restore, if not we can lose our data if the other action on groups trigger item save.

2nd. Removing items inside foreach groupItem.getChildren() is wrong, and lead to orphaned tabItems if the loop skip over some.
its have to be groupItem.getChildren().concat() or just remove the call to _reconnect() from this function

You have to take into consideration that any extension can also call setBrowserState setWindowState with or with out overwrite existing tabs.

my proposed patch is very simple:
1. mark tabs, that were overwrite by restore, that are about to move from one existing group to another as _reconnected = false.
2. mark tabs, that were overwrite by restore, taht are in group that is going to be close as _reconnected = false.
3. the rest of the work is done any how later by later by UI.reset if there are no groups at all or by TabItems.resumeReconnecting if there is groups. 

i don't see any reason to change any of the tabview-tab data that come from the sessionRestore.
Comment 7 Dietrich Ayala (:dietrich) 2011-10-18 14:56:38 PDT
Comment on attachment 566157 [details] [diff] [review]
Patch v0.2

Canceling review. Tim's going to evaluate the approach in the new patch.
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-06 19:42:33 PST
Comment on attachment 566185 [details]
proposed patch

Tim, I'm marking you for feedback? here so it doesn't fall off your radar. If this is on a better path than your patch, feel free to take the review.
Comment 9 Tim Taubert [:ttaubert] 2011-11-28 18:25:32 PST
Comment on attachment 566185 [details]
proposed patch

This approach works for tabs that get moved to a new group but only if the originating group will not be closed. Setting '_reconnected = false' does not prevent the tab from being closed when its parent group is. In a case like this we'd first need to find the new group's id, modify the tab data and then call reconnect() before the group is closed (see [1]).

Additionally I'd really like to get rid of the session store hack (see [1]) that solely exists to work around Panorama's strange (former) behavior.

[1] https://bug688695.bugzilla.mozilla.org/attachment.cgi?id=566157


> 1st: we have to set tabItem._reconnected = false to All item that were exist
> before the restore, if not we can lose our data if the other action on groups
> trigger item save.

True, thanks for finding that! At the moment even that can't occur because we didn't notice that tabs that changed groups weren't moved to their right groups on session restore... Filed bug 705964.

> 2nd. Removing items inside foreach groupItem.getChildren() is wrong, and lead to
> orphaned tabItems if the loop skip over some.

The usage here is correct. Please read the MDN documentation regarding Array.forEach().
Comment 10 Tim Taubert [:ttaubert] 2011-11-28 18:31:19 PST
Created attachment 577456 [details] [diff] [review]
patch v3

This patch removes the session store workaround for the old Panorama behavior wrt to group merging. It makes sure that tabs not assigned to a valid group are merged into the currently active group.

I'm not sure how to write a test for ss.restoreLastSession()... I think that's something for MozMill, right?
Comment 11 Tim Taubert [:ttaubert] 2011-11-28 18:32:57 PST
Please apply the patch from bug 705964 before this one (you might get conflicts when applying).
Comment 12 tabmix.onemen 2011-11-29 01:22:01 PST
(In reply to Tim Taubert [:ttaubert] from comment #9)
> The usage here is correct. Please read the MDN documentation regarding
> Array.forEach().

try this test code. it only show 1, 3 as an output.
let a = ["1", "2", "3", "4"];
a.forEach(function(item) {
  Services.console.logStringMessage(item);
  a.shift(item);
});
Comment 13 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-29 11:03:20 PST
Comment on attachment 577456 [details] [diff] [review]
patch v3

Review of attachment 577456 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks Tim! Testing ss.restoreLastSession() doesn't really happen, but a mozmill test would be the right way to go. I think there are litmus tests for this for whenever they get run.
Comment 14 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-29 11:11:07 PST
(In reply to onemen.one from comment #12)
> (In reply to Tim Taubert [:ttaubert] from comment #9)
> > The usage here is correct. Please read the MDN documentation regarding
> > Array.forEach().
> 
> try this test code. it only show 1, 3 as an output.
> let a = ["1", "2", "3", "4"];
> a.forEach(function(item) {
>   Services.console.logStringMessage(item);
>   a.shift(item);
> });

I don't think there's anything actually being removed from the array in Tim's code. So while your example shows gotchas with forEach, it's not applicable here.
Comment 15 Tim Taubert [:ttaubert] 2011-11-29 12:11:45 PST
(In reply to onemen.one from comment #12)
> try this test code. it only show 1, 3 as an output.

Mhh. I'm sure I tried the same with Array.splice() but got a different output. It's maybe safer to not rely on a special behavior and just clone the array...

(In reply to Paul O'Shannessy [:zpao] from comment #14)
> I don't think there's anything actually being removed from the array in
> Tim's code.

In fact, TabItem._reconnect() can remove items from the current children array when they change the group.
Comment 16 Tim Taubert [:ttaubert] 2011-11-29 22:53:59 PST
https://hg.mozilla.org/integration/fx-team/rev/7923eebf6c13
Comment 17 tabmix.onemen 2011-11-30 01:20:19 PST
> +            // correct the tab's groupID if necessary
> +            if (!parentGroup || -1 < toClose.indexOf(parentGroup)) {
> +              tabData.groupID = activeGroupId || Object.keys(groupItemData)[0];
> +              Storage.saveTab(tabItem.tab, tabData);
> +            }


this line
tabData.groupID = activeGroupId || Object.keys(groupItemData)[0];

not look good to me,
if we restore group-less session over existing session with group
in reconstitute both groupItemData and groupItemsData are empty objects ( {} }

activeGroupId is undefined
Object.keys(groupItemData)[0] is undefined when groupItemData = {}

setting undefined to tabData.groupID is not a good idea.

in TabItem__reconnect, for example, every tab with tabData.groupID = undefined
reconnect to new group !
Comment 18 Tim Taubert [:ttaubert] 2011-11-30 04:07:56 PST
(In reply to onemen.one from comment #17)
> if we restore group-less session over existing session with group
> in reconstitute both groupItemData and groupItemsData are empty objects ( {}

Uh, yeah. We should not try to sanitize the tab's groupID if its tabData is invalid. I filed a follow-up for this (bug 706430). Thanks again, you've been very helpful with this bug!
Comment 19 Tim Taubert [:ttaubert] 2011-11-30 22:21:06 PST
https://hg.mozilla.org/mozilla-central/rev/7923eebf6c13

Note You need to log in before you can comment on or make changes to this bug.