1291524 - Regression: "New Container Tab" context menu under the "File" breaks when session restore fails

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Kamil Jozwiak [:kjozwiak]]

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

Comment 1

[Justin [:JW_SoftvisionQA]]

good 2016-06-14
bad 2016-06-15

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

Comment 2

[Jonathan Kingston [:jkt] he/him]

Currently debugging this, I think I have the issue.

Comment 3

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1291524 - Load correct containers data in ensureDataReady method

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

Comment 4

[Jonathan Kingston [:jkt] he/him]

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 5

[François Marier [:francois]]

Comment on attachment 8778272 [details]
Bug 1291524 - Load correct containers data in ensureDataReady method

https://reviewboard.mozilla.org/r/69596/#review66796

Comment 6

[Tanvi Vyas[:tanvi]]

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 7

[:Paolo Amadini]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9879bf9369e
Load correct containers data in ensureDataReady method. r=paolo

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9879bf9369e

Comment 10

[Tanvi Vyas[:tanvi]]

Is Firefox 50 affected by this?

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 12

[Kamil Jozwiak [:kjozwiak]]

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

[Jonathan Kingston [:jkt] he/him]

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]:

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 15

[Jonathan Kingston [:jkt] he/him]

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 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 17

[Tanvi Vyas[:tanvi]]

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

Comment 18

[Kamil Jozwiak [:kjozwiak]]

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 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b637ce2fc3a