When a session restore fails via "Show my windows and tabs from last time" (bug#1291521) while using containers, the "New Container Tab" context menu under the "File" menu completely breaks. You'll receive the following error messages: via the terminal when using a m-c debug build: ************************* JavaScript error: resource://gre/modules/ContextualIdentityService.jsm, line 238: TypeError: this._identities.filter is not a function ************************* via the console browser: ************************* TypeError: this._identities.filter is not a function ContextualIdentityService.jsm:238:25 getIdentitiesresource://gre/modules/ContextualIdentityService.jsm:238:25 createUserContextMenuchrome://browser/content/utilityOverlay.js:441:3 onpopupshowingchrome://browser/content/browser.xul:1:8 ************************* STR: (https://youtu.be/108HVVIsP5Y) * install/open the latest version of m-c * change "When Nightly starts" under about:preferences#general to "Show my windows and tabs from last time" * close the about:preferences#general tab * open a personal and work container via File -> New Container Tab * close/restart m-c (if it restores the tabs correctly the first time around, close/restart m-c a few more times) * one you notice that the tabs haven't been restored, try opening the "New Container Tab" context menu via the "File" menu

good 2016-06-14 bad 2016-06-15 change log: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ddb6b30149221f00eb5dd180530e9033093d4c2b&tochange=14c5bf11d37b9e92d27f7089d9392de2ac339bb3

Currently debugging this, I think I have the issue.

Review commit: https://reviewboard.mozilla.org/r/69596/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69596/

Comment on attachment 8778272 [details] Bug 1291524 - Load correct containers data in ensureDataReady method Hey Francois are you available to review this patch? Thanks!

Comment on attachment 8778272 [details] Bug 1291524 - Load correct containers data in ensureDataReady method https://reviewboard.mozilla.org/r/69596/#review66796

Comment on attachment 8778272 [details] Bug 1291524 - Load correct containers data in ensureDataReady method baku and Gijs are both on PTO. Paolo, would you be able to review this? It's a 3 line change. Thanks!

Comment on attachment 8778272 [details] Bug 1291524 - Load correct containers data in ensureDataReady method https://reviewboard.mozilla.org/r/69596/#review67996 I don't know this code, but the patch is straightforward enough that it's an obvious r+.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c9879bf9369e Load correct containers data in ensureDataReady method. r=paolo

Is Firefox 50 affected by this?

If this is the same as bug 1291521, that one was tracked to 50, so there is a chance this is in 50 as well?

(In reply to Tanvi Vyas [:tanvi] from comment #10) > Is Firefox 50 affected by this? FX50 is affected. We should uplift this patch into fx50. I reproduced the issue using the STR from comment#0. Used the following build: * fx50.0a2, buildId: 20160815004002, changeset: 96c1a1c7fbaa Platforms: * macOS 10.11.6 - Reproduced * Win 10 x64 VM - Reproduced * Ubuntu 14.04.5 LTS VM - Reproduced ************************* TypeError: this._identities.filter is not a function ContextualIdentityService.jsm:238:25 getIdentitiesresource://gre/modules/ContextualIdentityService.jsm:238:25 createUserContextMenuchrome://browser/content/utilityOverlay.js:441:3 onpopupshowingchrome://browser/content/browser.xul:1:8 *************************

Comment on attachment 8778272 [details] Bug 1291524 - Load correct containers data in ensureDataReady method Approval Request Comment [Feature/regressing bug #]: Containers feature https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ddb6b30149221f00eb5dd180530e9033093d4c2b&tochange=14c5bf11d37b9e92d27f7089d9392de2ac339bb3 [User impact if declined]: Users won't see restored container tabs [Describe test coverage new/current, TreeHerder]: [Risks and why]: [String/UUID change made/needed]:

Hi Jonathan, could you please provide more info on the uplift request around test coverage and risk associated with the patch? Without that information, it is difficult to decide whether we ought to uplift the patch to Aurora or not. Thanks!

As containers is defaulted on in Nightly the menu breaks when doing a session restore. There isn't a new test in this patch however it was tested by Kamil and approved to fix the issue. Basically the code in question was rarely called besides in a race condition upon session restore, when it was the menu would not populate due to it not unwrapping the data to a change in data shape. The risks are low due to the code only interacting with containers menu code which only breaks in this instance anyway. I guess there is a chance it could further break containers menus in different contexts however as mentioned Kamil took a look at it. The reason for no test was really that I struggled to make a replication that would be reproducible in a test. Please let me know if you need further information, thank you for looking at this.

Comment on attachment 8778272 [details] Bug 1291524 - Load correct containers data in ensureDataReady method Same reason as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1295160#c14. Please let me know if I missed something.

The fix in this bug also fixes bug https://bugzilla.mozilla.org/show_bug.cgi?id=1291521, which is a bigger issue, since session restore doesn't work. For Test Pilot users in Firefox 50, their previous tabs will be lost when they restart the browser. And they won't be able to open new container tabs. The code itself is a 3 line change. Making the request again, with some more information... Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: For Test Pilot users in Firefox 50, there previous tabs will be lost when they restart the browser. [Describe test coverage new/current, TreeHerder]: We have general Containers test, but not a test for this specific bug. [Risks and why]: Low risk, 3 line change that is in a file that is only used for Containers. [String/UUID change made/needed]: None

I couldn't reproduce the issue with mozregression for some reason, so I did it the old fashion way and found the regression range manually which took a while. I initially found the range using mozilla-central and narrowed it down using mozilla-inbound. It looks like bug#1287091 might have been the original culprit. This also fixes the issue that I reported in bug#1291521 which involves containers being restored. Tanvi, regarding our conversion on IRC, I'm pretty sure this wasn't broken the entire time as I remember testing session restores in bug#1274461. mozilla-central regression range: ================================= * https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-25-03-02-48-mozilla-central/ ** Changeset: 7c669d5d63ef [GOOD] * https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-26-03-02-13-mozilla-central/ ** Changeset: ff1ef8ec0fd8 [BAD] mozilla-inbound regression range: ================================= * https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-inbound.pushdate.2016.07.24.20160725024218.firefox/gecko.v2.mozilla-inbound.pushdate.2016.07.24.20160725024218.firefox.macosx64-opt ** Changeset: 2e6d8283458c [GOOD] * https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-inbound.pushdate.2016.07.25.20160725072639.firefox/gecko.v2.mozilla-inbound.pushdate.2016.07.25.20160725072639.firefox.macosx64-opt ** Changeset: f577fea91160 [BAD] Regression range: ================= * https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2e6d8283458c&tochange=f577fea91160

Comment on attachment 8778272 [details] Bug 1291524 - Load correct containers data in ensureDataReady method Now that I am more aware of the Containers Test Pilot planned for Fx50, this regression which affects session restore is definitely worth uplifting. Aurora50+