tests for restore into existing tab with correct userContextId

NEW
Unassigned

Status

()

P3
normal
3 years ago
20 days ago

People

(Reporter: allstars.chh, Unassigned)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox57 wontfix, firefox58 affected)

Details

(Whiteboard: [OA-testing])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Comment 1

3 years ago
Created attachment 8735351 [details] [diff] [review]
WIP - Part 1: test for setTabState
(Reporter)

Comment 2

3 years ago
Created attachment 8735353 [details] [diff] [review]
WIP - Part 2: test for getWindowState

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

Comment 3

3 years ago
Created attachment 8735793 [details] [diff] [review]
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
Attachment #8735353 - Attachment is obsolete: true
Attachment #8735793 - Flags: feedback?(ttaubert)
(Reporter)

Comment 4

3 years ago
Created attachment 8736254 [details] [diff] [review]
Part 1: test for set/getTabState
Attachment #8735351 - Attachment is obsolete: true
(Reporter)

Comment 5

3 years ago
Created attachment 8736255 [details] [diff] [review]
Part 2: test for set/getWindowState
Attachment #8735793 - Attachment is obsolete: true
Attachment #8735793 - Flags: feedback?(ttaubert)
(Reporter)

Comment 6

3 years ago
Created attachment 8736256 [details] [diff] [review]
Part 3: test for set/getBrowserState
(Reporter)

Comment 7

3 years ago
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
Attachment #8736254 - Flags: review?(wmccloskey)
(Reporter)

Comment 8

3 years ago
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
Attachment #8736255 - Flags: review?(wmccloskey)
(Reporter)

Updated

3 years ago
Attachment #8736256 - Flags: review?(wmccloskey)
(Reporter)

Updated

3 years ago
Attachment #8735353 - Attachment is obsolete: false
(Reporter)

Updated

3 years ago
Attachment #8735353 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8735793 - Attachment is obsolete: false
(Reporter)

Comment 9

3 years ago
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
Attachment #8735793 - Flags: feedback?(wmccloskey)
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1260687
(Reporter)

Comment 11

3 years ago
Created attachment 8736562 [details] [diff] [review]
Part 4: test for duplicateTab
Attachment #8736562 - Flags: review?(wmccloskey)
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.
(Reporter)

Comment 13

3 years ago
(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.
(Reporter)

Updated

3 years ago
Attachment #8736254 - Flags: review?(wmccloskey) → review?(ttaubert)
(Reporter)

Updated

3 years ago
Attachment #8736255 - Flags: review?(wmccloskey) → review?(ttaubert)
(Reporter)

Updated

3 years ago
Attachment #8736256 - Flags: review?(wmccloskey) → review?(ttaubert)
(Reporter)

Updated

3 years ago
Attachment #8736562 - Flags: review?(wmccloskey) → review?(ttaubert)
(Reporter)

Comment 14

3 years ago
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.
Attachment #8735793 - Flags: feedback?(wmccloskey) → feedback?(ttaubert)
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?
Attachment #8735793 - Flags: feedback?(ttaubert)
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?
Attachment #8736254 - Flags: review?(ttaubert)
(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 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?
Attachment #8736562 - Flags: review?(ttaubert)
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.
Attachment #8736256 - Flags: review?(ttaubert)
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.
Attachment #8736255 - Flags: review?(ttaubert)

Updated

3 years ago
Whiteboard: [OA]

Updated

3 years ago
Status: NEW → ASSIGNED

Updated

3 years ago
Whiteboard: [OA] → [OA-testing]
(Reporter)

Updated

3 years ago
No longer blocks: 1250063
(Reporter)

Comment 22

2 years ago
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.
Assignee: allstars.chh → nobody
Status: ASSIGNED → NEW
Priority: -- → P2
(Reporter)

Comment 23

2 years ago
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
Flags: needinfo?(mdeboer)
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?
Flags: needinfo?(mdeboer) → needinfo?(allstars.chh)

Updated

2 years ago
Priority: P2 → P1

Updated

2 years ago
Assignee: nobody → huseby
(Reporter)

Comment 25

2 years ago
(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
Flags: needinfo?(allstars.chh)
(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.
Flags: needinfo?(huseby)
I'm planning on re-using/change existing code as much as possible.
Flags: needinfo?(huseby)
Assignee: huseby → nobody
Blocks: 1191418
Moving to p3 because no activity for at least 24 weeks.
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.