1514853 - Change network.cookie.cookieBehavior default pref value in Firefox 65 back to 0 in release

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/browser/app/profile/firefox.js#1511

We need to add back an #ifdef NIGHTLY_OR_EARLY_BETA for network.cookie.cookieBehavior.  Something like:

// Enable blocking access to storage from tracking resources by default
#ifdef NIGHTLY_OR_EARLY_BETA
pref("network.cookie.cookieBehavior", 4 /* BEHAVIOR_REJECT_TRACKER */);
#endif
pref("network.cookie.cookieBehavior", 0 /* BEHAVIOR_ACCEPT */);

Comment 1

[Andrea Marchesini [:baku]]

Attachment: pref.patch

Comment 2

[Tanvi Vyas[:tanvi]]

Comment on attachment 9032480 [details] [diff] [review]
pref.patch

baku, where is the default value of 0 for network.cookie.cookieBehavior set?

Comment 3

[(no longer active)]

https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/modules/libpref/init/StaticPrefList.h#1605

Comment 4

[(no longer active)]

Have we thought about how downgrading the pref from 4 to 0 during the beta cycle would work with the new Preferences UI by the way?  As in, which mode would users end up in on the beta channel, etc.

Comment 5

[Tanvi Vyas[:tanvi]]

No; are we already past early beta 65?  I will investigate.

Comment 6

[Tanvi Vyas[:tanvi]]

I guess it doesn't matter much whether or not we are still in early beta, it is still a problem.

The default value will switch from 4 to 0, so users who had it on will then end up with it turned off.  I think this is what happened in the beta 64 cycle.  The difference is the UI.  The UI will change with the default value.  A user would have been in Standard in until this pref change lands, where the default value was 4.  When we switch the default to 0, the users will still be in Standard but see the fallback UI. Users who have selected any other settings (Strict or Custom) won't change their category either.  So, unless I'm missing something, I don't think we have to worry about anything additional here.

Comment 7

[(no longer active)]

(In reply to Tanvi Vyas[:tanvi] from comment #6)
> I guess it doesn't matter much whether or not we are still in early beta, it
> is still a problem.
> 
> The default value will switch from 4 to 0, so users who had it on will then
> end up with it turned off.  I think this is what happened in the beta 64
> cycle.  The difference is the UI.  The UI will change with the default
> value.  A user would have been in Standard in until this pref change lands,
> where the default value was 4.  When we switch the default to 0, the users
> will still be in Standard but see the fallback UI. Users who have selected
> any other settings (Strict or Custom) won't change their category either. 
> So, unless I'm missing something, I don't think we have to worry about
> anything additional here.

Yes, I went through it in my head also, and I think you're right.  Now that I think about it, we did in fact think ahead about this problem and our solution was supposed to work in this case.  :-)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f70c0d3d1e
network.cookie.cookieBehavior default to 0 in release, r=tanvi

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/12f70c0d3d1e

Comment 10

[Tanvi Vyas[:tanvi]]

Please request uplift.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 9032480 [details] [diff] [review]
pref.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1492563

User impact if declined: This patch has been written to disable cookeBehavior 4 in release.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just disabling a feature.

String changes made/needed: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9032480 [details] [diff] [review]
pref.patch

[Triage Comment]
Reverts our cookie handling back to our previous behavior after the early betas. Approved for 65.0b7. Note that I'm planning to flip the early beta ifdef after shipping b7 prior to building b8.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7af67b7f5f79

Comment 14

[(no longer active)]

(In reply to Tanvi Vyas[:tanvi] from comment #0)
> https://searchfox.org/mozilla-central/rev/
> 47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/browser/app/profile/firefox.js#1511
> 
> We need to add back an #ifdef NIGHTLY_OR_EARLY_BETA for
> network.cookie.cookieBehavior.  Something like:
> 
> // Enable blocking access to storage from tracking resources by default
> #ifdef NIGHTLY_OR_EARLY_BETA

Such a macro does not exist.  It's EARLY_BETA_OR_EARLIER.

It seems like baku just copies and pasted this into the patch.  As a result, cookie restrictions have been disabled on Nightly since Friday.  :-(

> pref("network.cookie.cookieBehavior", 4 /* BEHAVIOR_REJECT_TRACKER */);
> #endif
> pref("network.cookie.cookieBehavior", 0 /* BEHAVIOR_ACCEPT */);

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (Bug 1514853, Bug 1516185) for browser_contentblocking.js failures a=backout

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=c37650fcf69ca288b874f4bb780831af68dce283&selectedJob=218619396

Backout link: https://hg.mozilla.org/mozilla-central/rev/f87b08093cd56ae66434ab8a42759b37e3e4198a

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218619396&repo=mozilla-central&lineNumber=8964

10:41:12     INFO - Entering test bound testContentBlockingCustomCategory
10:41:12     INFO - Buffered messages logged at 10:41:03
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | the pref urlclassifier.trackingTable remains as default value - 
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | the pref privacy.trackingprotection.enabled remains as default value - 
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | the pref privacy.trackingprotection.pbmode.enabled remains as default value - 
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | the pref network.cookie.cookieBehavior remains as default value - 
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | browser.contentblocking.category has been set to custom - 
10:41:12     INFO - Buffered messages logged at 10:41:04
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | browser.contentblocking.category has been set to custom - 
10:41:12     INFO - Buffered messages finished
10:41:12     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_contentblocking.js | Uncaught exception - undefined - timed out after 50 tries.
10:41:12     INFO - Leaving test bound testContentBlockingCustomCategory
...
10:41:36     INFO - GECKO(2099) | [Parent 2099, Main Thread] WARNING: NS_FAILED internal_GetScalarByEnum for CHILD: file /builds/worker/workspace/build/src/toolkit/components/telemetry/core/TelemetryScalar.cpp, line 2161
10:41:40     INFO - GECKO(2099) | --DOMWINDOW == 17 (0x118a36400) [pid = 2099] [serial = 388] [outer = 0x0] [url = about:preferences#privacy]
10:41:40     INFO - GECKO(2099) | --DOMWINDOW == 16 (0x11a367800) [pid = 2099] [serial = 396] [outer = 0x0] [url = about:preferences#privacy]
10:42:23     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
10:43:53     INFO - Not taking screenshot here: see the one that was previously logged
10:43:53     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_contentblocking.js | Test timed out - 
10:43:54     INFO - GECKO(2099) | MEMORY STAT | vsize 4552MB | residentFast 514MB | heapAllocated 109MB
10:43:54     INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_contentblocking.js | took 180361ms
10:43:54     INFO - Not taking screenshot here: see the one that was previously logged
10:43:54     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_contentblocking.js | Found a tab after previous test timed out: about:blank - 
10:43:54     INFO - Not taking screenshot here: see the one that was previously logged
10:43:54     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_contentblocking.js | Found a tab after previous test timed out: about:preferences#privacy - 
10:43:54     INFO - GECKO(2099) | ++DOCSHELL 0x114941800 == 1 [pid = 2104] [id = {1f7ee5b0-39e3-e641-b63a-6371c31e36c1}]
10:43:54     INFO - GECKO(2099) | ++DOMWINDOW == 1 (0x11e505c00) [pid = 2104] [serial = 22] [outer = 0x0]
10:43:54     INFO - GECKO(2099) | ++DOMWINDOW == 2 (0x11e5cd400) [pid = 2104] [serial = 23] [outer = 0x11e505c00]
10:43:54     INFO - GECKO(2099) | ++DOMWINDOW == 3 (0x11e5d2000) [pid = 2104] [serial = 24] [outer = 0x11e505c00]
10:43:54     INFO - checking window state

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

Also backed out from beta:

Backed out changeset 7af67b7f5f79 (Bug 1514853) as the followup 1516185 was backed out a=backout

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/750811248cbc40b3cc086d1be799dfd858bad4ad

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218619396&repo=mozilla-central&lineNumber=8964

10:41:12     INFO - Entering test bound testContentBlockingCustomCategory
10:41:12     INFO - Buffered messages logged at 10:41:03
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | the pref urlclassifier.trackingTable remains as default value - 
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | the pref privacy.trackingprotection.enabled remains as default value - 
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | the pref privacy.trackingprotection.pbmode.enabled remains as default value - 
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | the pref network.cookie.cookieBehavior remains as default value - 
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | browser.contentblocking.category has been set to custom - 
10:41:12     INFO - Buffered messages logged at 10:41:04
10:41:12     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_contentblocking.js | browser.contentblocking.category has been set to custom - 
10:41:12     INFO - Buffered messages finished
10:41:12     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_contentblocking.js | Uncaught exception - undefined - timed out after 50 tries.
10:41:12     INFO - Leaving test bound testContentBlockingCustomCategory
...
10:41:36     INFO - GECKO(2099) | [Parent 2099, Main Thread] WARNING: NS_FAILED internal_GetScalarByEnum for CHILD: file /builds/worker/workspace/build/src/toolkit/components/telemetry/core/TelemetryScalar.cpp, line 2161
10:41:40     INFO - GECKO(2099) | --DOMWINDOW == 17 (0x118a36400) [pid = 2099] [serial = 388] [outer = 0x0] [url = about:preferences#privacy]
10:41:40     INFO - GECKO(2099) | --DOMWINDOW == 16 (0x11a367800) [pid = 2099] [serial = 396] [outer = 0x0] [url = about:preferences#privacy]
10:42:23     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
10:43:53     INFO - Not taking screenshot here: see the one that was previously logged
10:43:53     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_contentblocking.js | Test timed out - 
10:43:54     INFO - GECKO(2099) | MEMORY STAT | vsize 4552MB | residentFast 514MB | heapAllocated 109MB
10:43:54     INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_contentblocking.js | took 180361ms
10:43:54     INFO - Not taking screenshot here: see the one that was previously logged
10:43:54     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_contentblocking.js | Found a tab after previous test timed out: about:blank - 
10:43:54     INFO - Not taking screenshot here: see the one that was previously logged
10:43:54     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_contentblocking.js | Found a tab after previous test timed out: about:preferences#privacy - 
10:43:54     INFO - GECKO(2099) | ++DOCSHELL 0x114941800 == 1 [pid = 2104] [id = {1f7ee5b0-39e3-e641-b63a-6371c31e36c1}]
10:43:54     INFO - GECKO(2099) | ++DOMWINDOW == 1 (0x11e505c00) [pid = 2104] [serial = 22] [outer = 0x0]
10:43:54     INFO - GECKO(2099) | ++DOMWINDOW == 2 (0x11e5cd400) [pid = 2104] [serial = 23] [outer = 0x11e505c00]
10:43:54     INFO - GECKO(2099) | ++DOMWINDOW == 3 (0x11e5d2000) [pid = 2104] [serial = 24] [outer = 0x11e505c00]
10:43:54     INFO - checking window state

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/724652219657
network.cookie.cookieBehavior default to 0 in release, r=tanvi

Comment 19

[(no longer active)]

The reason why this broke tests was this change that Andrea had added to the patch after review which was breaking the test: https://hg.mozilla.org/mozilla-central/rev/12f70c0d3d1e#l2.1

That change was effectively making the test only work when the default value of the cookieBehavior is 0.  The test needs to be rewritten properly to be able to handle both cases.

Comment 20

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-beta/rev/17b0cf07401c

Comment 21

[Razvan Maries]

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

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

improvements from landing:
== Change summary for alert #18406 (as of Fri, 21 Dec 2018 03:46:59 GMT) ==

Improvements:

 27%  raptor-tp6-yandex-firefox windows10-64 pgo           155.06 -> 112.90
 25%  raptor-tp6-yandex-firefox windows7-32 pgo            150.87 -> 113.54
 25%  raptor-tp6-yandex-firefox windows10-64 opt           161.43 -> 121.53
 25%  raptor-tp6-yandex-firefox windows7-32 opt            162.41 -> 122.49
 24%  raptor-tp6-yandex-firefox osx-10-10 opt              307.22 -> 233.68
 20%  raptor-tp6-yandex-firefox windows10-64-qr opt        141.85 -> 113.03
 18%  raptor-tp6-yandex-firefox linux64 pgo                143.57 -> 117.93
 18%  raptor-tp6-yandex-firefox linux64-qr opt             147.22 -> 121.20
 17%  raptor-tp6-yandex-firefox linux64 opt                151.57 -> 125.17
 17%  raptor-tp6-wikia-firefox windows7-32 opt             179.14 -> 148.62
 11%  raptor-tp6-amazon-firefox windows10-64 opt           493.46 -> 437.83
 10%  raptor-tp6-amazon-firefox linux64-qr opt             524.60 -> 470.30
  9%  raptor-tp6-amazon-firefox linux64 opt                485.78 -> 443.42
  7%  raptor-tp6-amazon-firefox windows10-64-qr opt        486.10 -> 450.56
  7%  raptor-tp6-amazon-firefox osx-10-10 opt              1,248.22 -> 1,158.77
  7%  raptor-tp6-amazon-firefox linux64 pgo                429.34 -> 400.56
  5%  raptor-tp6-amazon-firefox windows7-32 opt            455.54 -> 432.88
  5%  raptor-tp6-amazon-firefox windows7-32 pgo            411.96 -> 391.55
  5%  raptor-tp6-amazon-firefox windows10-64 pgo           431.05 -> 410.50
  4%  raptor-tp6-facebook-firefox linux64 pgo              369.34 -> 354.88
  4%  raptor-tp6-facebook-firefox windows10-64 opt         401.36 -> 386.06
  4%  raptor-tp6-facebook-firefox linux64-qr opt           416.77 -> 401.58
  3%  raptor-tp6-facebook-firefox windows7-32 pgo          372.93 -> 360.84
  3%  raptor-tp6-facebook-firefox linux64 opt              394.29 -> 381.67
  3%  raptor-tp6-facebook-firefox windows10-64-qr opt      412.12 -> 399.20
  3%  raptor-tp6-facebook-firefox windows10-64 pgo         377.01 -> 366.28

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=18406

regression for backing out is in bug 1516540

Comment 23

[(no longer active)]

As noted on that bug, this patch was relanded so there is no regression here per se, but the real regression is of course from cookieBehavior=0 -> cookieBehavior=4 which is what bug 1516540 is really tracking.