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

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kjozwiak, Assigned: jkt)

Tracking

(Blocks: 1 bug, {regression})

51 Branch
mozilla51
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Assignee)

Comment 2

2 years ago
Currently debugging this, I think I have the issue.
Assignee: nobody → jkt
(Assignee)

Updated

2 years ago
Blocks: 1291521
(Assignee)

Comment 3

2 years ago
Created attachment 8778272 [details]
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/
(Assignee)

Comment 4

2 years ago
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 6

2 years ago
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 7

2 years ago
mozreview-review
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9879bf9369e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Comment 10

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

Comment 12

2 years ago
(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
*************************
status-firefox50: --- → affected
Flags: needinfo?(kjozwiak)

Updated

2 years ago
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-backlog][uplift50+]
(Assignee)

Comment 13

2 years ago
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)
(Assignee)

Comment 15

2 years ago
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-

Updated

2 years ago
status-firefox50: affected → wontfix

Comment 17

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

Comment 18

2 years ago
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+

Updated

2 years ago
status-firefox50: wontfix → affected

Comment 20

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/9b637ce2fc3a
status-firefox50: affected → fixed

Updated

2 years ago
Depends on: 1287091
Flags: needinfo?(tanvi)
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1291521
You need to log in before you can comment on or make changes to this bug.