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?

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.

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

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/

fixed my previous TODO, and working on tests now.

added test

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

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

Comment on attachment 8757818 [details] [diff] [review] Part 2: restore tabs should preserve userContextId. Review of attachment 8757818 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting to Mike.

Comment on attachment 8756268 [details] [diff] [review] Part 1: undoCloseTab should preserve userContextId. Review of attachment 8756268 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting to Mike.

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...

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.

(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.

Hi Mike I added moveTabto and refined my code with some comments, would you review this again? Thanks

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.

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.

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

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.

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.

(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

rebase and addressed comments

addressed nits,

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!

(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.

https://hg.mozilla.org/mozilla-central/rev/ba2c44ebb458 https://hg.mozilla.org/mozilla-central/rev/898dd582cd36 https://hg.mozilla.org/mozilla-central/rev/9e78d56f4c51