1310112 - Add pref for using private containers in thumbnail loads

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Jonathan Hao (inactive) [:jhao]]

This is the follow-up of bug 1309699.  We need a pref for
1) http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/toolkit/components/thumbnails/BackgroundPageThumbs.jsm#199-202
2) http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/toolkit/components/thumbnails/BackgroundPageThumbs.jsm#438-442

and turn this pref on in http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/toolkit/components/thumbnails/test/browser_thumbnails_bg_no_cookies_stored.js

I'll upload a WIP patch later.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Attachment: (WIP) Add pref for using private containers in about:newtab.

Comment 2

[Jonathan Hao (inactive) [:jhao]]

There's still a failure with this patch if you run `./mach mochitest toolkit/components/thumbnails/test`.

Comment 3

[Tanvi Vyas[:tanvi]]

We need to add this pref in Firefox 51 and 52, or we are going to have more crash issues.

Comment 4

[Tim Huang[:timhuang]]

Attachment: Add pref for using private containers in about:newtab.

Comment 5

[Jonathan Hao (inactive) [:jhao]]

Attachment: Rebased for aurora uplift.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Attachment: Rebased for aurora uplift.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8802821 [details] [diff] [review]
Add pref for using private containers in about:newtab.

Actually, I'm not sure what the intent of this is. The pref is already disabled on non-nightly builds, and all this patch does is to allow the pref to be flipped while the browser is running - and as written, it seems like there's a risk that it will be flipped during a capture, which will probably break.

Comment 8

[Tim Huang[:timhuang]]

Tanvi, do we allow a user to change this pref during the browser is running? If we do, we have to figure out how to resolve the problem that Mark has stated in the comment 7. Maybe we could lock the pref during a capture.

Comment 9

[Tanvi Vyas[:tanvi]]

I don't completely follow the comments here.  The pref privacy.usercontext.about_newtab_segregation.enabled doesn't exist yet.  In Firefox 51 and 52, we segregate thumbnails by default with no pref to disable that segregation.

What we'd like is to add an privacy.usercontext.about_newtab_segregation.enabled pref and enable it in Nightly only.  If a user tries to toggle the pref value in about:config, I am fine with requiring a restart.  Either we can prompt the user while they are on about:config, telling them they have to restart for the pref to take effect.  Or do nothing, and start segregating after a restart.

Are there other about:config prefs that require restart?  We can follow their pattern.

Again, I'm not sure I got the question right.  So please let me know if I haven't answered it.

Comment 10

[Tim Huang[:timhuang]]

Actually, the pref 'privacy.usercontext.about_newtab_segregation.enabled' already exists in the m-c [1]. But it doesn't exist in the Firefox 51. So, Nightly has this pref.

[1] http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js#1421

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to Tim Huang[:timhuang] from comment #10)
> Actually, the pref 'privacy.usercontext.about_newtab_segregation.enabled'
> already exists in the m-c [1]. But it doesn't exist in the Firefox 51. So,
> Nightly has this pref.
> 
> [1]
> http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.
> js#1421

That is odd; I thought this was backed out of Firefox 52, but it looks like only this test change was backed out:
https://bugzilla.mozilla.org/show_bug.cgi?id=1309699#c46

But the code stayed in Nightly - https://bugzilla.mozilla.org/show_bug.cgi?id=1309699#c7.

Does test toolkit/components/thumbnails/test/browser_thumbnails_bg_no_cookies_stored.js pass in mozilla-central?  Does that test still need to be updated in Firefox 52?  Tim, is your attached patch https://bugzilla.mozilla.org/attachment.cgi?id=8802821 for 51, 52, or both?

We need to add the pref in Firefox 51 and set it to false.

Comment 12

[Jonathan Hao (inactive) [:jhao]]

(In reply to Tanvi Vyas [:tanvi] from comment #9)
> I don't completely follow the comments here.  The pref
> privacy.usercontext.about_newtab_segregation.enabled doesn't exist yet.  In
> Firefox 51 and 52, we segregate thumbnails by default with no pref to
> disable that segregation.
> 
> What we'd like is to add an
> privacy.usercontext.about_newtab_segregation.enabled pref and enable it in
> Nightly only.  If a user tries to toggle the pref value in about:config, I
> am fine with requiring a restart.  Either we can prompt the user while they
> are on about:config, telling them they have to restart for the pref to take
> effect.  Or do nothing, and start segregating after a restart.

If we can accept "do nothing, and start segregating after a restart", the current code in central is already correct.  However, a testcase will fail if applied to aurora because we can't restart the browser after turning on the pref in that test.

One idea is to create an API to re-initialize the xul browser, and use that in the test.

> Are there other about:config prefs that require restart?  We can follow
> their pattern.
> 
> Again, I'm not sure I got the question right.  So please let me know if I
> haven't answered it.

Comment 13

[Ethan Tseng [:ethan]]

(In reply to Tanvi Vyas [:tanvi] from comment #9)
> ...  If a user tries to toggle the pref value in about:config, I
> am fine with requiring a restart.  Either we can prompt the user while they
> are on about:config, telling them they have to restart for the pref to take
> effect.  Or do nothing, and start segregating after a restart.
> 
> Are there other about:config prefs that require restart?  We can follow
> their pattern.

Yes, there are some prefs that require restart.
(Prefs that don't listen to pref change event or use pref cache variable)

And users won't be told that a restart is required.
Usually they can only obtain this information by documents.

For example, https://wiki.mozilla.org/Electrolysis#Firefox_Beta
"If you would like to opt-in to help us test open about:config and toggle browser.tabs.remote.autostart to true. On your next restart, e10s should be active."

Comment 14

[Tim Huang[:timhuang]]

Attachment: Making the pref for using private containers in about:newtab can be changed during the browser is running.

Mark, I think we still have to have the ability to change this Pref in the run time since the test case 'browser_thumbnails_bg_no_cookies_stored.js' needs this Pref to be on to pass. So, this test will pass for Nightly, but won't for Aurora. If we want to make it pass for Aurora, we need to change this Pref in the test case. However, if the Pref takes effect only after a restart, then the test will still fail because the thumbnail browser is already created by other test cases. So, we need to make this Pref can be changed during the browser is running.

This patch will not destroy the thumbnails browser during a capture. It makes the service renew the thumbnails browser at the beginning of the next capture.

Comment 15

[Tim Huang[:timhuang]]

Attachment: Making the pref for using private containers in about:newtab can be changed during the browser is running.

Comment 16

[Tim Huang[:timhuang]]

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

Comment 17

[Tim Huang[:timhuang]]

try looks good.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eee70f71e85
Make the pref for using private containers in about:newtab can be changed during the browser is running. r=markh

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3eee70f71e85

Comment 20

[Tim Huang[:timhuang]]

Attachment: Rebased for aurora uplift

Comment 21

[Tim Huang[:timhuang]]

Attachment: Rebased for aurora uplift

Comment 22

[Tim Huang[:timhuang]]

Attachment: Rebased for aurora uplift

Comment 23

[Tim Huang[:timhuang]]

Comment on attachment 8808102 [details] [diff] [review]
Rebased for aurora uplift

Approval Request Comment
[Feature/regressing bug #]: 1289001
[User impact if declined]: The users will encounter crash problem which describes in the bug 1289001
[Describe test coverage new/current, TreeHerder]: The test 'browser_thumbnails_bg_no_cookies_stored.js' will test this pref.
[Risks and why]: No risk.

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8808102 [details] [diff] [review]
Rebased for aurora uplift

Add a pref to enable the "private containers in thumbnail loads" in nightly only. It should not impact aurora/beta/release. Take it in 51 aurora.

Comment 25

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d9cfe58247e8