1193854 - Restoring browser sessions should preserve containers

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

When you restart or launch the browser and restore your previous session, we should make sure to restore the tabs in the correct context.

Example:
* Browser open with 4 tabs - 2 in "personal" context, 1 in default context, 1 in "work" context.
* User closes the browser
* User relaunches the browser and either has their tabs set to restore in preferences or clicks "restore previous session".
* Browser should open with 4 tabs all with the correct contexts.

This might happen automatically; I'm not sure.  Once we are further along with the implementation, we can tests.  Keeping this bug to make sure we don't forget about this scenario.

This could be extended to bookmarked tabs, but I have a feeling that will be a little more tricky to pull off.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: WIP

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - store the userContextId in sessionStore

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - restore the UI

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8703604 [details] [diff] [review]
part 1 - store the userContextId in sessionStore

r+ for the docshell part, but some browser peer should look at the session store stuff. (oddly enough SessionHistory.jsm is about something else than actual session history.)

Comment 5

[Tanvi Vyas[:tanvi]]

Comment on attachment 8703618 [details] [diff] [review]
part 2 - restore the UI

You need a browser peer or someone more familiar with these files for this review.

Comment 6

[Tanvi Vyas[:tanvi]]

(In reply to Andrea Marchesini (:baku) from comment #2)
> Created attachment 8703604 [details] [diff] [review]
> part 1 - store the userContextId in sessionStore

Kate, can you look over this?  I seem to recall Jonas asking us not to add an nsDocShell::GetUserContextId.  Baku, do you need one for this patch to work?

Comment 7

[Andrea Marchesini [:baku]]

> Kate, can you look over this?  I seem to recall Jonas asking us not to add
> an nsDocShell::GetUserContextId.  Baku, do you need one for this patch to
> work?

Yes, I need to get the userContextId in used by the docShell in order to save it in the sessionStore with the entries.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8703604 [details] [diff] [review]
part 1 - store the userContextId in sessionStore

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

The basic changes are okay, giving r- just so that the patch doesn't land before addressing the comments.

::: browser/components/sessionstore/TabState.jsm
@@ +158,2 @@
>        tabData.pinned = true;
> +    }

Why all these changes here? Please revert them, they don't seem necessary.

@@ +238,5 @@
>          }
> +
> +        if (value.hasOwnProperty("userContextId")) {
> +          tabData.userContextId = value.userContextId;
> +        }

Can't we just set the property right after tabData.entries? SessionHistory.collect() always sets this property so we don't need to check, right?

Comment 9

[Andrea Marchesini [:baku]]

> Can't we just set the property right after tabData.entries?
> SessionHistory.collect() always sets this property so we don't need to
> check, right?

What about if we update FF and we restore from a previous session without userContextId ?

Comment 10

[Andrea Marchesini [:baku]]

> Why all these changes here? Please revert them, they don't seem necessary.

Right, but we do something that is not needed like, deleting non-existing properties.
I'll file a separate bug for this.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - store the userContextId in sessionStore

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8703618 [details] [diff] [review]
part 2 - restore the UI

ttaubert, can you review this patch too? Thanks.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8704015 [details] [diff] [review]
part 1 - store the userContextId in sessionStore

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

r=me for the two JSMs.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8703618 [details] [diff] [review]
part 2 - restore the UI

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

LGTM. I think we should have tests for this feature, it can be hard to detect manually when something breaks it.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 3 - test

Yes, you are right. Here a mochitest.

Comment 16

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Andrea Marchesini (:baku) from comment #7)
> > Kate, can you look over this?  I seem to recall Jonas asking us not to add
> > an nsDocShell::GetUserContextId.  Baku, do you need one for this patch to
> > work?
> 
> Yes, I need to get the userContextId in used by the docShell in order to
> save it in the sessionStore with the entries.

You should be able to get the userContextId from the OriginAttributes. :bholley requested that it be gettable only via the OriginAttributes in https://bugzilla.mozilla.org/show_bug.cgi?id=1191460#c22

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8704244 [details] [diff] [review]
part 3 - test

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

Thanks for starting with a test! Please take a look at newer tests that use add_task() and similar test suite features. I would also prefer if we don't overwrite the whole browser's state, but instead call .setTabState() on a tab we added just for that purpose. Lines 24-28 in [1] should have what you're looking for.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_purge_shistory.js

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 3 - test

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8704254 [details] [diff] [review]
part 3 - test

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

::: browser/components/sessionstore/test/browser_sessionStoreContainer.js
@@ +10,5 @@
> +    yield promiseBrowserLoaded(browser);
> +    yield promiseTabState(tab, { userContextId: i, entries: [{ url: "http://example.com/" }] });
> +
> +    let docShell = browser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                        .getInterface(Ci.nsIDocShell);

So the only problem we have here is that in e10s we can't access the contentWindow directly. Please use ContentTask.spawn() to read the userContextId and run the mochitest with --e10s to check that it works there as well.

@@ +15,5 @@
> +
> +    ok(docShell, "We have a DocShell");
> +    is(docShell.userContextId, i, "The docShell has the correct userContextId");
> +
> +    gBrowser.removeTab(tab);

Nit: yield promiseRemoveTab(tab);

@@ +18,5 @@
> +
> +    gBrowser.removeTab(tab);
> +  }
> +
> +  executeSoon(finish);

Nit: we don't need this

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 3 - test

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8704300 [details] [diff] [review]
part 3 - test

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

Nice, thanks!

::: browser/components/sessionstore/test/browser_sessionStoreContainer.js
@@ +18,5 @@
> +
> +    let userContextId = yield retrieveUserContextId(browser);
> +    is(userContextId, i, "The docShell has the correct userContextId");
> +
> +    gBrowser.removeTab(tab);

yield promiseRemoveTab(tab);

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/177acd4efc6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/294b15e9bcc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e27b120d5cf

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/177acd4efc6d
https://hg.mozilla.org/mozilla-central/rev/294b15e9bcc4
https://hg.mozilla.org/mozilla-central/rev/7e27b120d5cf

Comment 24

[Kate McKinley [:kmckinley, :Kate]]

baku, can you back this out so we can get the e10s contextual identity patch in?

Comment 25

[Andrea Marchesini [:baku]]

I don't think it's a good idea to back out those patches.
Actually I don't think the bug is in here at all (but I don't want to be too optimistic about it :)

I can reproduce the crash without having this code in my tree. Here what I do:

0. Open a new fresh FF
1. have 1 open tab A in userContextId = 0
2. open a tab B in any userContext != 0
3. open a content in the new tab B
4. close the tab A
5. load a new content in tab B. Often you can see the crash. In particular is the website is complex and it has iframes.

The reason why we crash is that we compare the SerializedLoadContext with all the possible 'configurations' of the ContentParent (NeckoParent::GetValidatedAppInfo). But if we have only 1 tab, in theory we should have just userContextId != 0.
But in my log I can often see:

Many: SERIALIZED 4 - TABCONTENT: 4
Then: SERIALIZED 0 - TABCONTENT: 4 and we crash.

my code is:

printf("SERIALIZED %d - TABCONTENT: %d\n", (int)aSerialized.mOriginAttributes.mUserContextId, (int) tabContext.OriginAttributesRef().mUserContextId);

I suspect we don't propagate correctly the userContextId to nested docShells but I'm not sure and I have to debug this issue more.

Comment 26

[Tanvi Vyas[:tanvi]]

(In reply to Andrea Marchesini (:baku) from comment #25)
> I don't think it's a good idea to back out those patches.
> Actually I don't think the bug is in here at all (but I don't want to be too
> optimistic about it :)
> 
> I can reproduce the crash without having this code in my tree. Here what I
> do:
> 
> 0. Open a new fresh FF
> 1. have 1 open tab A in userContextId = 0
> 2. open a tab B in any userContext != 0
> 3. open a content in the new tab B
> 4. close the tab A
> 5. load a new content in tab B. Often you can see the crash. In particular
> is the website is complex and it has iframes.
> 
> The reason why we crash is that we compare the SerializedLoadContext with
> all the possible 'configurations' of the ContentParent
> (NeckoParent::GetValidatedAppInfo). But if we have only 1 tab, in theory we
> should have just userContextId != 0.
> But in my log I can often see:
> 
> Many: SERIALIZED 4 - TABCONTENT: 4
> Then: SERIALIZED 0 - TABCONTENT: 4 and we crash.
> 
> my code is:
> 
> printf("SERIALIZED %d - TABCONTENT: %d\n",
> (int)aSerialized.mOriginAttributes.mUserContextId, (int)
> tabContext.OriginAttributesRef().mUserContextId);
> 
> I suspect we don't propagate correctly the userContextId to nested docShells
> but I'm not sure and I have to debug this issue more.

baku, it looks like you fixed this in https://bugzilla.mozilla.org/show_bug.cgi?id=1239420, correct?

Comment 27

[Andrea Marchesini [:baku]]

Yes, but we need to land also the additional patch I proposed for e10s and container (bug 1195881)