1457929 - Enable privacy.usercontext.about_newtab_segregation.enabled

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Tom Ritter [:tjr]]

In Bug 1279568 we noticed that the new tab page was setting cookies.  We decided to load thumbnails in a special dedicated container.  But that caused crashes debugged painfully and fixed in Bug 1289001 - during that process we put this feature behind the pref privacy.usercontext.about_newtab_segregation.enabled 

That pref has been enabled in Nightly since ~51. So there's probably (?) not any lingering crashes.

But there are some things we should investigate before flipping this pref.

1) Do we want to?  It seems like it would be beneficial to isolate the Thumbnail context from other stuff.

2) Do we (still) need to? Can thumbnails still set cookies or did something change in the past year or so to fix that.

3) Is there anything that prevents the chosen userContextId (seems like 5) from being used for a user container?

4) Is there any mechanism to clear this container's data so we don't just have a persistent tab thumbnail tracking storage?

Comment 1

[Tom Ritter [:tjr]]

Per :jkt - Besides being on by default in Nightly, if you install the Containers extension, this pref will get enabled automatically. So whoever is running that Containers Extension on Beta/Release is also running with this enabled. 

https://searchfox.org/mozilla-central/rev/08df4e6e11284186d477d7e5b0ae48483ecc979c/toolkit/components/extensions/parent/ext-contextualIdentities.js#18

Comment 2

[Jonathan Kingston [:jkt] he/him]

Yeah anyone with a contextual identity addon which is actually MAC+FB containers and any third party addons. It's safe to say there isn't likely to cause crashes.

I think at this stage it's ok to enable for everyone.

Comment 3

[(no longer active)]

(In reply to Tom Ritter [:tjr] from comment #0)
> 1) Do we want to?  It seems like it would be beneficial to isolate the
> Thumbnail context from other stuff.

This means that the thumbnail for sites like Gmail and Facebook would be captured from the logged out homepage, which is a bit surprising.  Is that a limitation that we're OK with?

> 2) Do we (still) need to? Can thumbnails still set cookies or did something
> change in the past year or so to fix that.

I'm pretty sure they will be able to set cookies, yes.

> 3) Is there anything that prevents the chosen userContextId (seems like 5)
> from being used for a user container?
> 
> 4) Is there any mechanism to clear this container's data so we don't just
> have a persistent tab thumbnail tracking storage?

These two questions are better answered by tanvi or baku.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Tom Ritter [:tjr] from comment #0)
> 1) Do we want to?  It seems like it would be beneficial to isolate the
> Thumbnail context from other stuff.
> 
> 2) Do we (still) need to? Can thumbnails still set cookies or did something
> change in the past year or so to fix that.
> 
Needinfo Ed

> 3) Is there anything that prevents the chosen userContextId (seems like 5)
> from being used for a user container?
> 
I believe it is reserved and can't be reused.  In fact, no usercontextid can be reused, they continuously increment, so if you delete a container with usercontextid=8, then next newly created container won't reuse 8, but will instead use usercontextid=9. Baku can confirm.

> 4) Is there any mechanism to clear this container's data so we don't just
> have a persistent tab thumbnail tracking storage?
You can clear a containers storage by deleting the container, but in this case there is no way to delete this container.  SO if we want this to be less persistent, we would have to create rules around this and build them into the platform.

Comment 5

[Andrea Marchesini [:baku]]

> I believe it is reserved and can't be reused.

This is right. userContextIds are not reused. Plus, userContextId 5 is marked as private and it cannot be deleted using ContextualIdentityService component. Basically it's completely invisible to web-extensions and the rest of the codebase.

> > 4) Is there any mechanism to clear this container's data so we don't just
> > have a persistent tab thumbnail tracking storage?

> You can clear a containers storage by deleting the container, but in this
> case there is no way to delete this container.  SO if we want this to be
> less persistent, we would have to create rules around this and build them
> into the platform.

If I remember correctly, the idea of using a private userContextId was a 'temporary' solution. We wanted to use a private browsing sessions with a private id: remember that originAttributes dictionary supports private browsing as integer allowing multiple sessions, but this is not supported by many components which still have an internal boolean for private browsing/normal navigation.

What we could do is to cleanup private containers on shutdown (and on startup in case of crashes).
Any feedback about this approach?

Comment 6

[Ed Lee :Mardak]

Oh… I was quite confused about this bug because I'm pretty sure Activity Stream people have been working with the premise that thumbnails are captured without cookies, etc. We have various bugs to deal with pages that are not that useful with a logged-out thumbnail where the thumbnail captures a log in screen. So to be clear, this cookie-less behavior is currently only on Nightly and this bug is to make it the default for all channels. ?

tspurway, I would guess most of our telemetry / experiment results are from non-Nightly users, so does turning on cookieless thumbnails for all warrant more discussion?

Activity Stream has added features like custom screenshots where the user can replace a thumbnail / favicon of a top site to be some other image, so lower quality page thumbnails is probably not a huge issue… ?

Comment 7

[Tim Spurway [:tspurway]]

This is surprising.  If we feel confident that turning this on won't affect the crash rate, let's set this pref to true for all channels.  

Since we have (wrongly) insisted that the thumbnailer does not issue cookies on some privacy oriented bugs, I believe we should also pursue uplifting this to beta.

Comment 8

[Tom Ritter [:tjr]]

I can write a patch that enables the pref, that's easy.  An important followup bug will need to clear the container on shutdown/startup.

Comment 9

[Tom Ritter [:tjr]]

Attachment: Bug 1457929 Enable ontainer-based segregation of newtab thumbnails on all channels

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

Comment 10

[Tom Ritter [:tjr]]

I've attached a patch that will enable it outside nightly.  

One wrinkle. There are other container-related prefs we make Nightly only that we aren't enabling with this. From my read of the code, everything should be fine. But we should test to be sure. I've made a Nightly build that will emulate Release (that is to say, it enables privacy.usercontext.about_newtab_segregation.enabled but not the other prefs that are Nightly only.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3a07cfbbe44edbd80052b36a2153341db4014b9

I'm also filing a followup bug about clearing out this container.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Tom Ritter [:tjr] from comment #10)
> I've attached a patch that will enable it outside nightly.  
> 
> One wrinkle. There are other container-related prefs we make Nightly only
> that we aren't enabling with this. From my read of the code, everything
> should be fine. But we should test to be sure. I've made a Nightly build
> that will emulate Release (that is to say, it enables
> privacy.usercontext.about_newtab_segregation.enabled but not the other prefs
> that are Nightly only.)
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e3a07cfbbe44edbd80052b36a2153341db4014b9
> 
> I'm also filing a followup bug about clearing out this container.

What kind of testing help will you need here, if any?

Comment 12

[Tom Ritter [:tjr]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> (In reply to Tom Ritter [:tjr] from comment #10)
> > I've attached a patch that will enable it outside nightly.  
> > 
> > One wrinkle. There are other container-related prefs we make Nightly only
> > that we aren't enabling with this. From my read of the code, everything
> > should be fine. But we should test to be sure. I've made a Nightly build
> > that will emulate Release (that is to say, it enables
> > privacy.usercontext.about_newtab_segregation.enabled but not the other prefs
> > that are Nightly only.)
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=e3a07cfbbe44edbd80052b36a2153341db4014b9
> > 
> > I'm also filing a followup bug about clearing out this container.
> 
> What kind of testing help will you need here, if any?

From my reading of the code, it's primarily focused on "Does the Preferences and some other specific UI behave the same way it does now on release or does it behave the way it behaves after you install Containers or does it behave in a mix of both?"

The authoritative people for this are probably jkt and Tanvi though.

Comment 13

[Jonathan Kingston [:jkt] he/him]

My understanding is the private container for thumbnailing clears itself anyway so no patch should be required when this pref is enabled.

The other prefs:
privacy.usercontext.enabled = Controls the menus of the browser, long press, context menus etc.
privacy.usercontext.ui.enabled = Controls the about:preferences ui
privacy.usercontext.longPressBehaviour = Controls if the new tab button shop allow a long press or a one click to get the container menu
privacy.usercontext.extension = Kind of a hack used by the web extension API to decide if the prefs are being controlled.

None of these should impact if origin attributes can be used, the feature is always enabled just not accessible to uses.

Testing should likely be done to check that these assertions are true when checking on a stable build. However at this point I'm pretty confident this is completely fine.

Also I would prefer we removed the thumbnailing all-together to be honest.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

If we want this to ship in 61, we've realistically got about two weeks left in the cycle to make that happen. Will you be able to look at this review soon, Tanvi?

Comment 15

[Tanvi Vyas[:tanvi]]

Comment on attachment 8976984 [details]
Bug 1457929 Enable ontainer-based segregation of newtab thumbnails on all channels

r+ for the patch, but does need some testing.  The pref has only ever been set in combination with the other container prefs.  So what happens if it is set independently?  Hopefully there are no unknown dependencies here.  Jonathan seems to think we will be okay, but should be tested regardless.

I'm very confused on the cookie discussions regarding the thumbnail service.  If the thumbnail service had cookies, this would make sure those cookies are segregated into their own cookie jar.  Ed, you believed the thumbnail service wouldn't cause any cookies to be created in the browser in the first place, regardless of whether or not such cookies would have been segregated.  What changed your mind?  Has anyone seen cookies being created from the use of the thumbnail service?

Comment 16

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #15)
> I'm very confused on the cookie discussions regarding the thumbnail service.
> If the thumbnail service had cookies, this would make sure those cookies are
> segregated into their own cookie jar.  Ed, you believed the thumbnail
> service wouldn't cause any cookies to be created in the browser in the first
> place, regardless of whether or not such cookies would have been segregated.
> What changed your mind?  Has anyone seen cookies being created from the use
> of the thumbnail service?

Comment 17

[Tom Ritter [:tjr]]

Info for QE Verification:

Without the containers extension:

Confirm that in [Preferences -> General -> Tabs Section] there isn't anything about Container Tabs.  (You can compare to a browser that has the Containers Extension installed)

Confirm that the File menu doesn't have 'New Container Tab'

That right clicking on a link doesn't show the container tab menu

That long pressing the new tab button doesn't show the container menu

That any other container-related UI thing I couldn't find doesn't show up (but I think I found them all?)

Confirm that adding the container extension works fine.

Comment 18

[Tom Ritter [:tjr]]

Oh, something important for QE Verification: It needs to be done on Beta; it can't be done on Nightly.

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 8976984 [details]
Bug 1457929 Enable ontainer-based segregation of newtab thumbnails on all channels

Approval Request Comment
[Feature/Bug causing the regression]: We'd like to enable this privacy feature (it's baked on Nightly for several releases.)

[User impact if declined]: None? 

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]: Yes (See comment 17 & 18)

[List of other uplifts needed for the feature/fix]: We should probably take Bug 1462662 too

[Is the change risky?]: Not really.

[Why is the change risky/not risky?]: We quite confident there aren't crashes, since it's been in Nightly for so long. However this pref has only ever been set in conjunction with other prefs and this new combination of them hasn't been tested. That what Manual QE is for.

Comment 20

[Tanvi Vyas[:tanvi]]

Comment on attachment 8976984 [details]
Bug 1457929 Enable ontainer-based segregation of newtab thumbnails on all channels

https://reviewboard.mozilla.org/r/245120/#review254520

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8976984 [details]
Bug 1457929 Enable ontainer-based segregation of newtab thumbnails on all channels

Sounds like a nice privacy win. Approved for 61.0b11.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8976984 [details]
Bug 1457929 Enable ontainer-based segregation of newtab thumbnails on all channels

Oh bah, this hasn't landed yet.

Comment 23

[Ed Lee :Mardak]

(In reply to Tanvi Vyas [:tanvi] from comment #15)
> you believed the thumbnail service wouldn't cause any cookies to be created
> in the browser in the first place
From our testing and triaging bugs, it appeared that thumbnails did not create cookies. Only after looking at this bug was it clear that this behavior was only on Nightly.

> Has anyone seen cookies being created from the use of the thumbnail service?
I would think any non-Nightly user is currently seeing thumbnails that make use of cookies, and maybe that's what users expected so didn't file bugs? The bugs we have seen have related to how cookies aren't used with thumbnails, but I guess those were Nightly users?

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0d9fa700814
Enable ontainer-based segregation of newtab thumbnails on all channels r=tanvi

Comment 25

[Natalia Csoregi [:nataliaCs]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8976984 [details]
Bug 1457929 Enable ontainer-based segregation of newtab thumbnails on all channels

Approved for real this time :)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/119d817c5c50

Comment 28

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

I have verified this issue on Mac Os X 10.13, Windows 10 x64 and Ubuntu 16.04 x64 with Firefox Beta (61.0b11-20180603002454) following the steps from comment 17 and I can confirm the fix.

I will mark this as verified.

Comment 29

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

I have verified this issue on Mac Os X 10.13, Windows 10 x64 and Ubuntu 16.04 x64 with the latest Nightly (62.0a1-20180605100102) and I can confirm the fix.