1416468 - [Regression] Firefox Nightly overrides the Containers every time it is started

Status: RESOLVED FIXED

(Firefox :: Untriaged, defect)

Description

[renatogamici+firefox]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171110100139

Steps to reproduce:

1. Create new containers (and delete the default ones (Personal; Working; Banking and Shopping);
2. Restart firefox.



Actual results:

The new containers are gone, and the default that were deleted are back.
The pages that were open on a non-default container are open as with no container.
The ones on default containers are still opened correctly.


Expected results:

It should preserve the container settings.
Previously, it was working correctly, the problem appeared probably this week. I cannot check exactly which one was the first one broken, but 58.0a1 (2017-11-10) is not working.

Comment 1

[Will Levine]

I can confirm this. I see the exact same thing. Previously I had some custom containers, and now they aren't visible (staring sometime this week). The container list got reset to the default.

I think that the custom containers may still exist, but are just hidden. Because I had gmail open in my "google" container, and when I restarted and updated, the "google" container had disappeared, and the gmail tab was no longer marked as a container tab, but I was still logged in to google in that tab, and I wasn't logged in to google on any of the non-container tabs. However, once I close that tab, I have no way to open another "google" container tab.

Currently using:
Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
20171111100349

but the problem started a few days ago.

Comment 2

[Kohei Yoshino [:kohei]]

Please report any containers issue to https://github.com/mozilla/multi-account-containers/issues

Note: it's already reported in https://github.com/mozilla/multi-account-containers/issues/949

Comment 3

[bart.zalewski]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171111100349

I have experienced the same behavior with the last 2 nightly updates I have run.

Further Information:
The containers actually appear to be intact however the Manage Containers page is reset to the 4 defaults. If I add the 3 additional containers I utilize, then all of the state within them appear to still exist and their functionality is normal at that point. Also the labels don't matter at all, I renamed the 4 defaults however their state is carried over despite them reverting back to the original default names; it would seem that the containers are simply referenced by their array index. 

Moreover, I use Cookie AutoDelete https://addons.mozilla.org/en-US/firefox/addon/cookie-autodelete/ which has container support and the results are the same here. The array position of the containers simply maps over despite whether they have been renamed (reset to defaults).

The underlying bug seems to simply be that the container label/icon settings are cleared out, however the underlying containers themselves still exist and make it through the nightly update.

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

Kohei I'm reopening this because of the dependency, and because it's reproducible without the extension.
And now also, because I've figured out the actual bug.

This seems, based on a mozregression run, to be a regression from bug 1347515.
And indeed https://hg.mozilla.org/integration/mozilla-inbound/rev/08f21a0e7ebe touches toolkit/components/contextualidentity/ContextualIdentityService.jsm

Interestingly, it only occurs if "Show your windows and tabs from last time" is selected in options.  If one instead has "Show your home page" but then manually hits "Restore Previous Session" in the appmenu on startup, containers are preserved.  The containers.json file in the profile folder has the custom containers after shutdown, they only get deleted from it on load and auto-restore session.

Console wise, in bad builds I get this:
Argument 1 of TextDecoder.decode could not be converted to any of: ArrayBufferView, ArrayBuffer.  ContextualIdentityService.jsm:321
	ensureDataReady resource://gre/modules/ContextualIdentityService.jsm:321:31
	getPublicIdentityFromId resource://gre/modules/ContextualIdentityService.jsm:345:5
	setTabStyle resource://gre/modules/ContextualIdentityService.jsm:370:20
	addTab chrome://browser/content/tabbrowser.xml:2718:15
	ssi_restoreWindow resource:///modules/sessionstore/SessionStore.jsm:3414:15
	ssi_restoreWindows resource:///modules/sessionstore/SessionStore.jsm:3617:5
	initializeWindow resource:///modules/sessionstore/SessionStore.jsm:1179:11
	onBeforeBrowserWindowShown/< resource:///modules/sessionstore/SessionStore.jsm:1330:9

So it looks like the use of gTextDecoder.decode() in https://hg.mozilla.org/integration/mozilla-inbound/rev/08f21a0e7ebe is the problem.

Baku, I noticed you have a related PR for the extension, but won't ContextualIdentityService.jsm need a patch too?

In fact, after some more debugging, I think I know exactly what the problem is, and will have a patch shortly.
Basically the patch in https://hg.mozilla.org/integration/mozilla-inbound/diff/08f21a0e7ebe/toolkit/components/contextualidentity/ContextualIdentityService.jsm
tries to get the bytes of the file from NetUtil.readInputStreamToString(), and then decode them into a string with gTextDecoder.decode(). But readInputStreamToString() already returns a string, so the decode is unnecessary, and throws, (it would be needed with the sibling NetUtil function readInputStream()).

Comment 5

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1416468 - Share container data parsing instead of duplicating it.

NetUtil.readInputStreamToString() already returns a string, so trying to decode
it from bytes to a string throws.

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

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8927906 [details]
Bug 1416468 - Share container data parsing instead of duplicating it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199162/diff/1-2/

Comment 7

[Ian Moody [:Kwan] (UTC+1)]

Baku, would you be willing to review this please?  Or able to suggest someone else?

After some IRC discussion with jkt we decided to make the patch a bit bigger by unifying the parsing of the file from load() and ensureDataReady() into a separate function, since it seemed weird to repeat the code (and ensureDataReady() missed out on the version checking, unless that was deliberate?).

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8927906 [details]
Bug 1416468 - Share container data parsing instead of duplicating it.

https://reviewboard.mozilla.org/r/199162/#review204280

Comment 9

[Tanvi Vyas[:tanvi]]

It looks like https://bugzilla.mozilla.org/show_bug.cgi?id=1347515 made it into 58.  This would need to make it into the first distributed build of Firefox 58 too, otherwise all our beta users will also lose their Containers.

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

Thanks baku!

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c683ea6edbc6db37ab1532766ccbcfb4ad89aa5

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8927906 [details]
Bug 1416468 - Share container data parsing instead of duplicating it.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1347515
[User impact if declined]: Upon Fx restart and automatic session restore users will lose their custom containers, and any open tabs in said containers will be opened in the default container
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]:
0) go to preferences and select "Show your windows and tabs from last time"
1) go to about:preferences#containers and create a new container
2) long-press the new tab button to get a menu
3) click on the name of the created container in said menu to open a new tab in said container
4) navigate to some page in the container tab
5) restart the browser
6a) in bad builds the previously opened page will reopen in the default container, and the created container will no longer exist
6b) in good build the previously opened page will reopen in the created container
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: small code change, de-duplicates file parsing code, and instead of a function that returns the containers.json file as a string uses one that returns it as bytes (which is what the code expected, and was the source of the bug).
[String changes made/needed]: none

Comment 12

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

Tracked as a 58 blocker.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

jcristau

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Oops, meant to n-i to julien. We may want to rebuild 58.0b3 as described in bug 1347515.

Comment 15

[Tanvi Vyas[:tanvi]]

To elaborate on the impact, in addition to the data loss, the following data leakage scenario could happen without this bug fix:
I create Container "social", it has usercontextId=5.  I create container "adult-content" it has usercontextid=6.  I update Nightly.  My container names and icons are lost.  I recreate container "adult-content" first - it gets usercontextid=5 now instead of 6.  Then I create container "social" with usercontextid=6.  Now the cookies that were in social are accessible to adult-content and vice versa.  When I browse adult-content in my new "adult-content" container, social.com cookies exist and are now tracking me through adult-content websites I visit.

Comment 16

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Ian Moody [:Kwan] from comment #11)
> [User impact if declined]: Upon Fx restart and automatic session restore
> users will lose their custom containers, and any open tabs in said
> containers will be opened in the default container
Correction: they only _appear_ to be in the default context because the tabs lack the styling (because they can't get a definition of it when the context doesn't exist anymore), they are still in their context, and if one creates a new container (that'll reuse the ids) they will get some styling back (that in the address bar, but not the tab)
Something that will break is the removing individual cookies dialog, it won't list any cookies in containers that aren't defined anymore (until you create a new container definition for that contextid).
Also as tanvi pointed out on IRC (and now the above comment #15), if you don't re-create the containers in the same order you originally did, stuff will "change" container.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/e1d7427787f7
Share container data parsing instead of duplicating it. r=baku a=baku

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8927906 [details]
Bug 1416468 - Share container data parsing instead of duplicating it.

This is 58 beta blocker. We need this. Beta58+. Should be in 58.0b4.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8927906 [details]
Bug 1416468 - Share container data parsing instead of duplicating it.

Remove the beta approval for now as we plan to back out bug 1347515 from m-b.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Fixed for 58 by backing out bug 1347515.

Comment 22

[Szmozsánszky István [:flaki]]

(In reply to Ian Moody [:Kwan] from comment #16)
> (In reply to Ian Moody [:Kwan] from comment #11)
> > [User impact if declined]: Upon Fx restart and automatic session restore
> > users will lose their custom containers, and any open tabs in said
> > containers will be opened in the default container
> Correction: they only _appear_ to be in the default context because the tabs
> lack the styling (because they can't get a definition of it when the context
> doesn't exist anymore), they are still in their context [...]

Can confirm, I've switched to the extension when all Nightly controls disappeared the other day, and the most peculiar part of this issue emerging is that my (pinned) tabs are restored with their original contexts intact (but there is no way to control those contexts, or open new tabs in any of those specific contexts).

Relevant Twitter convo: https://twitter.com/slsoftworks/status/930106998946979840