Closed Bug 1274461 Opened 8 years ago Closed 8 years ago

Session Restore not restoring containers

Categories

(Core :: DOM: Security, defect, P1)

49 Branch
x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: kjozwiak, Assigned: allstars.chh)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [userContextId][domsecurity-active])

Attachments

(3 files, 7 obsolete files)

3.91 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
6.34 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
2.93 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
It looks like either bug #1250033 or bug #1250063 has regressed when restoring containers via session restore. When fx crashes and the session restore is used, fx loads all the websites in a default container rather than the appropriate containers. Found the following regression range via mozregression:

37:11.23 INFO: Last good revision: f4f23a7dd3072268b18d9755e72ab2f27462455d
37:11.23 INFO: First bad revision: abf706cbf6a2012e4133bf6ca5b89d6159d995e1
37:11.23 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f4f23a7dd3072268b18d9755e72ab2f27462455d&tochange=abf706cbf6a2012e4133bf6ca5b89d6159d995e1

STR:

* launch the latest version of m-c
* change devtools.chrome.enabled;true & privacy.userContext.enabled;true
* open a regular tab (default container), personal container, work container and load a website in each tab
* open the browser console and crash fx using the following:

Cu.import("resource://gre/modules/ctypes.jsm");
let zero = new ctypes.intptr_t(8);
let badptr = ctypes.cast(zero, ctypes.PointerType(ctypes.int32_t));
badptr.contents;

* re-open fx and click on the restore session button

Yoshi, it looks like the two possible patches that might have caused this are yours. Mind taking a look?
Flags: needinfo?(allstars.chh)
Whiteboard: [userContextId]
Assignee: nobody → allstars.chh
Flags: needinfo?(allstars.chh)
I'm not sure if this is P1 or P2.  If the session is restored in the default container, it could cause the user to load something in default that they wouldn't have loaded otherwise.  For now setting it as P1, and we can reduce priority if we find that:
* it is less important than other P1s that come up
* it is quite difficult to fix.
Priority: -- → P1
Status: NEW → ASSIGNED
Whiteboard: [userContextId] → [userContextId][domsecurity-active]
This is also affecting restoring individual tabs via "Undo Close Tab", STR:

* load a website inside a container
* once the website has finished loading, close the container tab
* right click on another tab that's still currently opened and select "Undo Close Tab"

FX will restore the website correctly but will use the default container rather than using the correct contextid that was used before the tab was closed.
(In reply to Kamil Jozwiak [:kjozwiak] from comment #2)
Yeah, I also noticed this in bug 1273215, checking this now.

but Kamil do you have a way to crash a tab?
Comment 0 seems crashes the whole browser, and there are 'reviveCrashedTab' and 'reviveAllCrashedTabs' in SessionStore look likely have the same problem but I am not sure the correct way to trigger it.

If you don't know this I'll use the same tricks from Bug 1273215 to reproduce it.

Thanks
Flags: needinfo?(kjozwiak)
You could try using the tab crasher add-on [1] that was created by Mike Conley. When I was testing, I was just crashing the entire browser. Let me know if this works for you!

[1] https://addons.mozilla.org/en-US/firefox/addon/tab-crasher/
Flags: needinfo?(kjozwiak)
Blocks: 1276412
fixed my previous TODO, and working on tests now.
Attachment #8756270 - Attachment is obsolete: true
Comment on attachment 8756268 [details] [diff] [review]
Part 1: undoCloseTab should preserve userContextId.

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

Hi Tim
I found that undoCloseTab will not preserve userContextId, so I wrote a patch to fix it.

Could you review this for me ?

Thanks
Attachment #8756268 - Flags: review?(ttaubert)
Comment on attachment 8757818 [details] [diff] [review]
Part 2: restore tabs should preserve userContextId.

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

Hi Tim
I also found that restoreWindow doesn't preserve userContextId properly, 
and I've also fixed that case that when we restore into an existing tab should create a tab for this, see
https://bugzilla.mozilla.org/show_bug.cgi?id=1250063#c38
https://bugzilla.mozilla.org/show_bug.cgi?id=1250063#c42

Could you review this ?

Thanks
Attachment #8757818 - Flags: review?(ttaubert)
Comment on attachment 8757818 [details] [diff] [review]
Part 2: restore tabs should preserve userContextId.

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

Redirecting to Mike.
Attachment #8757818 - Flags: review?(ttaubert) → review?(mdeboer)
Comment on attachment 8756268 [details] [diff] [review]
Part 1: undoCloseTab should preserve userContextId.

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

Redirecting to Mike.
Attachment #8756268 - Flags: review?(ttaubert) → review?(mdeboer)
Comment on attachment 8757818 [details] [diff] [review]
Part 2: restore tabs should preserve userContextId.

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

::: browser/components/sessionstore/SessionStore.jsm
@@ +2926,5 @@
> +      // is matched, otherwise we create a new tab for restoring.
> +      let reuse = false;
> +      if (t < openTabCount) {
> +        let winUid = winData.tabs[t].userContextId || "";
> +        reuse = tabbrowser.tabs[t].getAttribute("usercontextid") == winUid;

I think you need to change this to:
```js
let tab = tabbrowser.tabs[t];
let userContextId = winData.tabs[t].userContextId;
let reuseOpenTab = t < openTabCount;
if (reuseOpenTab && userContextId)
  reuseOpenTab = tab.getAttribute("usercontextid") == userContextId;
// ...and change references below too.
```

@@ +2941,5 @@
> +      // If we didn't reuse the tab because of the mismatching userContextId,
> +      // remove it from gBrowser.
> +      if (t < openTabCount && !reuse) {
> +        tabbrowser.removeTab(tabbrowser.tabs[t]);
> +      }

I don't understand why you need to remove the tab here. If the context IDs don't match, then sure, we need to remove the tab, but why not earlier in the previous check? Additionally, what happens when `winData.tabs[t].userContextId` is undefined? I mean, in that case `t < openTabCount` will be TRUE and `reuse` will be FALSE...
Attachment #8757818 - Flags: review?(mdeboer) → review-
Comment on attachment 8756268 [details] [diff] [review]
Part 1: undoCloseTab should preserve userContextId.

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

::: browser/components/sessionstore/SessionStore.jsm
@@ +2162,5 @@
>  
>      // create a new tab
>      let tabbrowser = aWindow.gBrowser;
> +    let userContextId = state.userContextId;
> +    let tab = userContextId ? tabbrowser.addTab(null, {userContextId}) : tabbrowser.addTab();

I think I'd prefer:
```js
let tab = tabbrowser.selectedTab = tabbrowser.addTab(null, state);
```

::: browser/components/sessionstore/test/browser_sessionStoreContainer.js
@@ +47,5 @@
>    yield promiseRemoveTab(tab2);
>  });
>  
> +add_task(function* () {
> +  let tab = gBrowser.addTab("http://example.com/", {userContextId: 1});

nit: `{ userContextId: 1 }`, please correct it above too.

@@ +55,5 @@
> +
> +  gBrowser.removeTab(tab);
> +
> +  let tab2 = ss.undoCloseTab(window, 0);
> +  Assert.equal(tab2.getAttribute("usercontextid"), 1);

Why is `yield promiseTabRestored(tab2);` not needed here?

@@ +56,5 @@
> +  gBrowser.removeTab(tab);
> +
> +  let tab2 = ss.undoCloseTab(window, 0);
> +  Assert.equal(tab2.getAttribute("usercontextid"), 1);
> +  yield ContentTask.spawn(tab2.linkedBrowser, { expectedId: 1}, function* (args) {

nit: `{ expectedId: 1 }`, please correct it above too.
Attachment #8756268 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #14)
Imagine you have a window with 2 open tabs pointing to 'about:blank' and restore a window right there which starts looping over tabs to restore.
The first tab we try to restore has a `userContextId` that matches with the tab it may reuse, so it does. Note that this tab is at index 0.
The second tab we try to restore doesn’t have a userContextId, so we add a new tab instead. Note that this tab will be added at index tabbrowser.tabs.length - 1, or '2'.
The third tab we try to restore has a userContextId that matches with the tab it may reuse, because it t = '2' and the tab we just added is also at '2'. In other words, it will reuse and override the tab we just added.

The issue is that `addTab()` always _appends_ tabs, it doesn’t insert them at index `t`. So I think you’ll need to think of something smarter and more complicated that also does a `tabbrowser.moveTabTo(newlyAddedTab, t);`, because you're being more selective about which open tabs are usable for a restore.
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> ::: browser/components/sessionstore/SessionStore.jsm
> @@ +2926,5 @@
> > +      // is matched, otherwise we create a new tab for restoring.
> > +      let reuse = false;
> > +      if (t < openTabCount) {
> > +        let winUid = winData.tabs[t].userContextId || "";
> > +        reuse = tabbrowser.tabs[t].getAttribute("usercontextid") == winUid;
> 
> I think you need to change this to:
> ```js
> let tab = tabbrowser.tabs[t];
> let userContextId = winData.tabs[t].userContextId;
> let reuseOpenTab = t < openTabCount;
> if (reuseOpenTab && userContextId)
>   reuseOpenTab = tab.getAttribute("usercontextid") == userContextId;
> // ...and change references below too.
> ```
> 
I check this comment again and I think it will fail when 
restore data doesn't have userContextId but the existing tab has.

In that case we still need to open a new tab for it instead of reusing.
So I'll keep my original code as it is.
addressed the nits.
Attachment #8756268 - Attachment is obsolete: true
Attachment #8758632 - Flags: review?(mdeboer)
Hi Mike
I added moveTabto and refined my code with some comments,
would you review this again?

Thanks
Attachment #8757818 - Attachment is obsolete: true
Attachment #8758636 - Flags: review?(mdeboer)
Attachment #8758632 - Flags: review?(mdeboer) → review+
Comment on attachment 8758636 [details] [diff] [review]
Part 2: restore tabs should preserve userContextId. v2

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

Thanks for the tests!

I put an alternative way of writing in a comment down below, which I'd like you to consider. The logic is sound nonetheless, so r=me.

::: browser/components/sessionstore/SessionStore.jsm
@@ +2927,5 @@
> +        // If the tab (winData.tabs[t]) doesn't have userContextId, the value
> +        // will be undefined, but getAttribute("usercontextid") will be "", so
> +        // we use `|| ""` to let it has a default value "".
> +        let userContextId = winData.tabs[t].userContextId || "";
> +        reuse = tabbrowser.tabs[t].getAttribute("usercontextid") == userContextId;

You can drop the comment above and make the story part of the condition:
```js
reuse = !userContextId ||
  tabbrowser.tabs[t].getAttribute("usercontextid") == userContextId;
```

@@ +2957,5 @@
> +          tabbrowser.moveTabTo(tab, t);
> +        }
> +      }
> +
> +      tabs.push(tab);

I think you can write this down more succinct and merging the story into one comment, like so:

```js
// When trying to restore into existing tab, we also take the userContextId
// into account if present.
let userContextId = winData.tabs[t].userContextId;
let reuseExisting = t < openTabCount && (!userContextId ||
  tabbrowser.tabs[t].getAttribute("usercontextid") == userContextId);
let tab = reuseExisting ? tabbrowser.tabs[t] : tabbrowser.addTab("about:blank", {
  skipAnimation: true,
  forceNotRemote: true,
  userContextId
});

// If we inserted a new tab because the userContextId didn't match with the
// open tab, even though `t < openTabCount`, we need to remove that open tab
// and put the newly added tab in its place.
if (!reuseExisting && t < openTabCount) {
  tabbrowser.removeTab(tabbrowser.tabs[t]);
  tabbrowser.moveTabTo(tab, t);
}
```

::: browser/components/sessionstore/test/browser_sessionStoreContainer.js
@@ +106,5 @@
> +
> +  ss.setWindowState(win2, JSON.stringify(winState), true);
> +
> +  for (let i = 0; i < 4; i++) {
> +    yield ContentTask.spawn(win2.gBrowser.tabs[i].linkedBrowser, { expectedId: i + 1 }, function* (args) {

nit: can you do `let browser = win2.gBrowser.tabs[i].linkedBrowser;` so that this fits nicely on one line?

@@ +118,5 @@
> +    });
> +  }
> +
> +  // Test the last tab, which doesn't have userContextId.
> +  yield ContentTask.spawn(win2.gBrowser.tabs[4].linkedBrowser, { expectedId: 0 }, function* (args) {

nit: same here.
Attachment #8758636 - Flags: review?(mdeboer) → review+
Comment on attachment 8758636 [details] [diff] [review]
Part 2: restore tabs should preserve userContextId. v2

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

Hi Mike
Thanks for the review, however I have question for your previous comment, 
can you take a look again?

Thanks

::: browser/components/sessionstore/SessionStore.jsm
@@ +2927,5 @@
> +        // If the tab (winData.tabs[t]) doesn't have userContextId, the value
> +        // will be undefined, but getAttribute("usercontextid") will be "", so
> +        // we use `|| ""` to let it has a default value "".
> +        let userContextId = winData.tabs[t].userContextId || "";
> +        reuse = tabbrowser.tabs[t].getAttribute("usercontextid") == userContextId;

That looks incorrect to me.
For example, when the restored data doesn't have userContextId (userContextId is ""), however the open tab does. (tabbrowser.tabs[t] has usercontextid attribute).

reuse will become true even though ```tabbrowser.tabs[t].getAttribute("usercontextid") == userContextId;``` is false.

@@ +2957,5 @@
> +          tabbrowser.moveTabTo(tab, t);
> +        }
> +      }
> +
> +      tabs.push(tab);

let reuseExisting = t < openTabCount && (!userContextId ||
  tabbrowser.tabs[t].getAttribute("usercontextid") == userContextId);
will have the same problem I mentioned above.
Flags: needinfo?(mdeboer)
Hi Mike
This is the test as I mentioned earlier, restore data doesn't have userContextId, however open tab does have.

I've tried to address your comment, see 
https://gist.github.com/allstarschh/b90dec8cb9af30b3ddb47e8861f335a0

but it will fail in the test.

Or if I misunderstood your comments, can you point out where I am wrong?

thanks
Attachment #8759017 - Flags: review?(mdeboer)
OK, sorry for the misunderstanding... so I was talking about replacing all that code, including the first `if (t < openTabCount) {` part.
And it seems like it should be `let userContextId = winData.tabs[t].userContextId || "";`, like in your patch.
Flags: needinfo?(mdeboer)
Comment on attachment 8759017 [details] [diff] [review]
Part 3: another test for restoring.

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

::: browser/components/sessionstore/test/browser_sessionStoreContainer.js
@@ +138,5 @@
> +add_task(function* () {
> +  let win = window.openDialog(location, "_blank", "chrome,all,dialog=no");
> +  yield promiseWindowLoaded(win);
> +
> +  let tab = win.gBrowser.addTab("http://example.com/", {userContextId: 1});

nit: please add spaces, like `{ userContextId: 1 }`

@@ +155,5 @@
> +
> +  let win2 = window.openDialog(location, "_blank", "chrome,all,dialog=no");
> +  yield promiseWindowLoaded(win2);
> +
> +  let tab2 = win2.gBrowser.addTab("http://example.com/", {userContextId : 1});

nit: same.
Attachment #8759017 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #24)
> nit: please add spaces, like `{ userContextId: 1 }`
Sorry, I usually type without the spaces after/before { }, and I forgot it again. :P

Thanks for reminding.
rebase and addressed comments
Attachment #8758636 - Attachment is obsolete: true
Attachment #8759489 - Flags: review+
addressed nits,
Attachment #8759017 - Attachment is obsolete: true
Attachment #8759490 - Flags: review+
Comment on attachment 8759489 [details] [diff] [review]
Part 2: restore tabs should preserve userContextId. v3

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

::: browser/components/sessionstore/test/browser_sessionStoreContainer.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +requestLongerTimeout(2);

I still see some intermitten timeout on try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df07cea3e49f&selectedJob=21918831

I'll increase it to 3.
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba2c44ebb458
Part 1: undoCloseTab should preserve userContextId. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/898dd582cd36
Part 2: restore tabs should preserve userContextId.  r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e78d56f4c51
Part 3: another test for restoring. r=mikedeboer
Yoshi, can you file a follow-up to split up browser_sessionStoreContainer.js in two parts and to remove the requestLongerTimeout(3)?
Thanks!
Flags: needinfo?(allstars.chh)
(In reply to Mike de Boer [:mikedeboer] from comment #32)
> Yoshi, can you file a follow-up to split up browser_sessionStoreContainer.js
> in two parts and to remove the requestLongerTimeout(3)?
> Thanks!

Sure, Bug 1278149.
Flags: needinfo?(allstars.chh)
Depends on: 1279103
Depends on: 1475501
Has Regression Range: --- → yes
Keywords: regression
Summary: Regression: Session Restore not restoring containers → Session Restore not restoring containers
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: