Closed Bug 1416468 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Untriaged, defect)

58 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 blocking fixed
firefox59 + fixed

People

(Reporter: renatogamici+firefox, Assigned: Kwan)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Summary: Firefox Nightly overrides the Containers every time it is started → [Regression] Firefox Nightly overrides the Containers every time it is started
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.
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
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
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.
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()).
Assignee: nobody → moz-ian
Blocks: 1347515
Status: RESOLVED → REOPENED
Ever confirmed: true
Keywords: regression
Resolution: INCOMPLETE → ---
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?).
Flags: needinfo?(amarchesini)
Comment on attachment 8927906 [details]
Bug 1416468 - Share container data parsing instead of duplicating it.

https://reviewboard.mozilla.org/r/199162/#review204280
Attachment #8927906 - Flags: review+
Flags: needinfo?(amarchesini)
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 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
Attachment #8927906 - Flags: approval-mozilla-beta?
Tracked as a 58 blocker.
Oops, meant to n-i to julien. We may want to rebuild 58.0b3 as described in bug 1347515.
Flags: needinfo?(jcristau)
Flags: needinfo?(jcristau) → needinfo?(gchang)
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.
(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.
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/e1d7427787f7
Share container data parsing instead of duplicating it. r=baku a=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e1d7427787f7
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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.
Flags: needinfo?(gchang)
Attachment #8927906 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8927906 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Fixed for 58 by backing out bug 1347515.
(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
Attachment #8927906 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1416958
Duplicate of this bug: 1417001
See Also: → 1419589
See Also: → 1419591
You need to log in before you can comment on or make changes to this bug.