1257456 - tests for restore into existing tab with correct userContextId

Status: NEW

(Core :: Security, defect, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

See ttaubert's comment https://bugzilla.mozilla.org/show_bug.cgi?id=1250063#c38
We should have some tests for verifying userContextId is matched after ss.setTabState, ss.setWindowState and ss.setBrowserState

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP - Part 1: test for setTabState

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP - Part 2: test for getWindowState

I found getWindowState() will fail to get the correct userContextId from the tabs, will look into this.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP - Part 2: test for getWindowState

Hi Tim
I found some problems when I wrote a test for getWindowState, for example, I got two windows, (win, win2),
then I try to get the winState2 from win2, it doesn't have correct userContextId set.

//... create a tab with userContextId:1 in win
let winState = JSON.parse(ss.getWindowState(win));
Assert.equal(winState.windows[0].tabs[1].userContextId, 1); // okay

ss.setWindowState(win2, winState);
let winState2 = JSON.parse(ss.getWindowState(win2));
Assert.equal(winState2.windows[0].tabs[1].userContextId, 1); // fail

I suspect that's because TabState doesn't propagate userContextId.

When we call ss.getWindowState(win2) it will call SessionStore.jsm#_collectWindowData() and in turn call TabState.collect() to get the tabData.

So I modify TabState.jsm to grab the userContextId attribute from tabbrowser.
Would like to get feedback from you first.

Thanks

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: test for set/getTabState

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: test for set/getWindowState

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: test for set/getBrowserState

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8736254 [details] [diff] [review]
Part 1: test for set/getTabState

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

Hi billm
As :ttaubert is still in PTO, could you review this for me?

Thanks

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8736255 [details] [diff] [review]
Part 2: test for set/getWindowState

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

Hi billm
I updated SessionStore.jsm to update TabStateCache, without doing this the test 
Assert.equal(winState2.windows[0].tabs[1].userContextId, 1, "2nd Window: tabs[1].userContextId should exist.");
will fail,

because previous call setWindowState() won't update the userContextId in TabStaeCache,
and when getWindowState() is called, it will call TabState.collect, but we cannot get the userContextId as it isn't in the cache.

Thanks

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8735793 [details] [diff] [review]
WIP - Part 2: test for getWindowState

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

::: browser/components/sessionstore/TabState.jsm
@@ +139,5 @@
>  
> +    if (tab.hasAttribute("usercontextid")) {
> +      tabData.userContextId = tab.getAttribute("usercontextid");
> +    }
> +

Hi billm
This is my previous Part 2 patch.
However I am not sure is this neccesary?

In my latest Part 2 patch I updated TabStateCache, so this change could be ignored.

But I am worried that if in some cases, TabStateCache won't have userContextId, so when TabState.collect/TabState.clone is called, we might lose userContextId here, could you give me some suggestions here?

Thanks

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: test for duplicateTab

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, I'm afraid I don't understand the context for these tests. It seems like they mostly test collecting the userContextId but not restoring it. Is the work to save/restore this data actually done?

I think it would probably be best to wait until Tim gets back.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Bill McCloskey (:billm) from comment #12)
> Sorry, I'm afraid I don't understand the context for these tests. It seems
> like they mostly test collecting the userContextId but not restoring it. Is
> the work to save/restore this data actually done?
By 'restore', do you mean a tab is closed or crashed, and then we try to restore it back, right?
That will my next plan but will be a seperate bug, as in the beginning I try to verify ttaubert's comments from https://bugzilla.mozilla.org/show_bug.cgi?id=1250063#c38 or Comment 0.

> I think it would probably be best to wait until Tim gets back.
I'll forward to r? to him.
Thanks for the comments.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8735793 [details] [diff] [review]
WIP - Part 2: test for getWindowState

As explained in Comment 9,
I am not sure the patch in TabState.jsm is neccesary.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1885ffa77c86

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8735793 [details] [diff] [review]
WIP - Part 2: test for getWindowState

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

::: browser/components/sessionstore/TabState.jsm
@@ +138,5 @@
>      }
>  
> +    if (tab.hasAttribute("usercontextid")) {
> +      tabData.userContextId = tab.getAttribute("usercontextid");
> +    }

We collect this value from loadContext.originAttributes in SessionHistory.jsm already, why would we do this twice?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8736254 [details] [diff] [review]
Part 1: test for set/getTabState

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

All this test does is check the tab attribute, shouldn't we rather kick off a load and see what the effective userContextId really is?

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #16)
> We collect this value from loadContext.originAttributes in
> SessionHistory.jsm already, why would we do this twice?

Also, we restore this attribute in SessionStore.restoreTab() and in SessionHistory.restore(). This doesn't seem well thought out and probably needs cleaning up.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8736562 [details] [diff] [review]
Part 4: test for duplicateTab

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

Should probably check the effective userContextId here as well by loading something?

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8736256 [details] [diff] [review]
Part 3: test for set/getBrowserState

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

It doesn't seem to make sense to write tests that depend on the location or presence of the userContextId property somewhere in the browser state. Checking that we properly restore/overwrite tabs should do.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8736255 [details] [diff] [review]
Part 2: test for set/getWindowState

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

Same comment as for the previous patch.

::: browser/components/sessionstore/SessionStore.jsm
@@ +3244,5 @@
>      // Update the persistent tab state cache with |tabData| information.
>      TabStateCache.update(browser, {
> +      history: {entries: tabData.entries,
> +                index: tabData.index,
> +                userContextId: tabData.userContextId},

This fix probably should have its own bug with a separate test, if we can write one for it. It's also good to think about the tab.setUserContextId() call in the same function above this line, I don't think we need both.

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I've fixed the main problem when restoring into tabs with differnt userContextId in bug 1274461.
So far session restore works well in container tab, I'll lower the priority of this bug and deassign myself on this.

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Mike
I'd like to request for your suggestions for this bug.
As I have written some tests related to SessionRestore in container tab in Bug 1274461, do you think we still need this bug?
This bug is filed long time ago even before bug 1274461, but now I am not sure that do we still need some tests for those APIs in SessionStore, like set/getTabState, BrowserState, WindowState, as bug 1274461 should cover most of them already.

Can you give me some suggestions here? 

Thanks

Comment 24

[Mike de Boer [:mikedeboer]]

Hi Yoshi! I indeed think that you've covered many cases in bug 1274461 and friends, but I don't know by heart if you've covered the case as stated in the summary:
Is there a test for restoring into an _existing_ tab with the correct userContextID? If so, can you point me to it?

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Mike de Boer [:mikedeboer] from comment #24)
> Is there a test for restoring into an _existing_ tab with the correct
> userContextID? If so, can you point me to it?
Hi Mike
This one, https://hg.mozilla.org/mozilla-central/rev/898dd582cd36#l2.28

Or do you think we need to have similar tests for get/setTabState, and get/setBrowserState()?

I am not 100% sure when set/getTabState() and get/setBrowserState() are used during SessionRestore.
And wonder if bug 1274461 covers enough test, then I'll close this one.

Thanks

Comment 26

[Mike de Boer [:mikedeboer]]

(In reply to Yoshi Huang[:allstars.chh] from comment #25)
> (In reply to Mike de Boer [:mikedeboer] from comment #24)
> > Is there a test for restoring into an _existing_ tab with the correct
> > userContextID? If so, can you point me to it?
> Hi Mike
> This one, https://hg.mozilla.org/mozilla-central/rev/898dd582cd36#l2.28

Agreed.

> Or do you think we need to have similar tests for get/setTabState, and
> get/setBrowserState()?

It won't hurts to augment the current test with tests for `getTabState` and `getBrowserState`, no?

It also seems like Dave Huseby has taken an interest in this bug, so I'm curious what he intends to do here.

Comment 27

[Dave Huseby [:huseby]]

I'm planning on re-using/change existing code as much as possible.

Comment 28

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 29

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bugmasters/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.