Closed Bug 1291524 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Security, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: kjozwiak, Assigned: jkt)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, Whiteboard: [userContextId][domsecurity-backlog][uplift50+])

Attachments

(1 file)

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
Currently debugging this, I think I have the issue.
Assignee: nobody → jkt
Blocks: 1291521
Comment on attachment 8778272 [details]
Bug 1291524 - Load correct containers data in ensureDataReady method

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

https://reviewboard.mozilla.org/r/69596/#review66796
Attachment #8778272 - Flags: review?(francois)
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!
Attachment #8778272 - Flags: review?(paolo.mozmail)
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+.
Attachment #8778272 - Flags: review?(paolo.mozmail) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9879bf9369e
Load correct containers data in ensureDataReady method. r=paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9879bf9369e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Is Firefox 50 affected by this?
Flags: needinfo?(kjozwiak)
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
*************************
Flags: needinfo?(kjozwiak)
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-backlog][uplift50+]
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]:
Attachment #8778272 - Flags: approval-mozilla-aurora?
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!
Flags: needinfo?(jkt)
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.
Flags: needinfo?(jkt) → needinfo?(rkothari)
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.
Flags: needinfo?(rkothari)
Attachment #8778272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Flags: needinfo?(rkothari)
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
Flags: needinfo?(tanvi)
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+
Flags: needinfo?(rkothari)
Attachment #8778272 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
Depends on: 1287091
Flags: needinfo?(tanvi)
Duplicate of this bug: 1291521
You need to log in before you can comment on or make changes to this bug.