Last Comment Bug 851606 - Disable bug 818340 for Beta until we're ready
: Disable bug 818340 for Beta until we're ready
Status: VERIFIED FIXED
[no-nag]
:
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla26
Assigned To: Sid Stamm [:geekboy or :sstamm]
: Bogdan Maris, QA [:bogdan_maris]
Mentors:
Depends on: 999170
Blocks: 818340 902112
  Show dependency treegraph
 
Reported: 2013-03-15 11:58 PDT by Alex Keybl [:akeybl]
Modified: 2014-04-21 14:45 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
verified
+
verified
+
verified
+
verified


Attachments
revert cookie defaults to "allow all" (1.51 KB, patch)
2013-03-25 14:08 PDT, Sid Stamm [:geekboy or :sstamm]
dolske: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
fix tests and privacy.js (7.06 KB, patch)
2013-05-14 12:22 PDT, Sid Stamm [:geekboy or :sstamm]
ehsan: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
3pc-autopref.diff (8.71 KB, patch)
2013-08-06 12:50 PDT, Sid Stamm [:geekboy or :sstamm]
gavin.sharp: review-
Details | Diff | Splinter Review
3pc-autopref (12.76 KB, patch)
2013-08-07 09:43 PDT, Sid Stamm [:geekboy or :sstamm]
no flags Details | Diff | Splinter Review
auto-revert patch for central and aurora (12.86 KB, patch)
2013-08-07 11:01 PDT, Sid Stamm [:geekboy or :sstamm]
gavin.sharp: review+
Details | Diff | Splinter Review
auto-revert patch for central and aurora (13.21 KB, patch)
2013-08-08 11:31 PDT, Sid Stamm [:geekboy or :sstamm]
mozbugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Alex Keybl [:akeybl] 2013-03-15 11:58:08 PDT
Making sure we're tracking whether or not we should uplift bug 818340.
Comment 1 Alex Keybl [:akeybl] 2013-03-15 12:38:56 PDT
To clarify, this is around merges. Should this feature remain enabled at each merge point?
Comment 2 Sid Stamm [:geekboy or :sstamm] 2013-03-15 13:11:33 PDT
I think we should keep the code in there, but maybe during the next merge to aurora, we revert the pref to the old one.  Should be a one-liner.  This way users of aurora can test it, but it's not on by default.  (We can revisit uplifting the default as we get more data about the feature.)
Comment 3 rsx11m 2013-03-15 14:24:18 PDT
Since SeaMonkey has added a corresponding UI as well on trunk already (bug 845353), possibly other Gecko browsers too, just flipping the pref to its old default would be preferable if it's decided not to let this move to aurora or beta. Backing out the Core code would also require dependent changes for those applications, whereas just changing the pref's default for those channels shouldn't pose a problem.
Comment 4 Brendan Eich [:brendan] 2013-03-18 09:29:25 PDT
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> I think we should keep the code in there, but maybe during the next merge to
> aurora, we revert the pref to the old one.  Should be a one-liner.  This way
> users of aurora can test it, but it's not on by default.  (We can revisit
> uplifting the default as we get more data about the feature.)

At this point I do not see enough smoke to declare "fire". We need to dig into reports of problems that have trickled in, but on balance I think it's better to get a larger channel: beta. We can hold the patch there for as long as we need to be sure it is not an unacceptable breaking change.

Want bugs on Firefox OS and Firefox for Android UX, if they aren't on file already.

/be
Comment 5 Sid Stamm [:geekboy or :sstamm] 2013-03-18 09:45:54 PDT
(In reply to Brendan Eich [:brendan] from comment #4)
> Want bugs on Firefox OS and Firefox for Android UX, if they aren't on file
> already.

Android: bug 837331

I will file a bug for Firefox OS.

Your plan sounds good, Brendan.  So no work to deviate from the usual until beta at the earliest.
Comment 6 Alex Keybl [:akeybl] 2013-03-18 15:46:35 PDT
Assigning to Dolske for final resolution, as he reviewed bug 818340.
Comment 7 Justin Dolske [:Dolske] 2013-03-19 16:02:34 PDT
(Bumping to Sid, since he's actually tracking this feature.)
Comment 8 Sid Stamm [:geekboy or :sstamm] 2013-03-25 14:08:24 PDT
Created attachment 729195 [details] [diff] [review]
revert cookie defaults to "allow all"

This patch reverts the global default cookie setting to allow all cookies by default.  Flagging dolske for review since it's probably good to double-check.

If things go well on the 22 train, we won't need this patch, but in case we want to have it trigger-ready, here it is.
Comment 9 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2013-04-19 06:46:45 PDT
I agree to keep the "accept all" preference on Firefox 22.

We haven't landed bug 837326 yet. Thus I seems we have not collect the analytic "real" data about how may 3rd parties cookies are blocked by new action. This means we can't predict correctly with real data. The new default behavior may break the web. So we should be careful.
Comment 10 Brendan Eich [:brendan] 2013-05-14 08:41:08 PDT
See bug 818340 comment 88. Without telemetry or equivalent monitoring (tracked by bug 837326), we should not go to Beta. jmayer agrees.

Sid will request approval for the one-line patch here ASAP.

/be
Comment 11 Sid Stamm [:geekboy or :sstamm] 2013-05-14 08:47:44 PDT
Comment on attachment 729195 [details] [diff] [review]
revert cookie defaults to "allow all"

akeybl: can you approve?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): need telemetry for bug 818340
User impact if declined: unknown for sure, but potentially some sites could break.
Testing completed (on m-c, etc.): this reverts the default cookie behavior, testing is in all prior versions of Firefox.
Risk to taking this patch (and alternatives if risky): Not risky.
String or IDL/UUID changes made by this patch: None.
Comment 12 Alex Keybl [:akeybl] 2013-05-14 09:03:36 PDT
Comment on attachment 729195 [details] [diff] [review]
revert cookie defaults to "allow all"

Approving for Beta 22 and tracking for 23 so that we can make a call there next cycle as well.
Comment 13 Sid Stamm [:geekboy or :sstamm] 2013-05-14 09:11:09 PDT
Ah yeah, whoops, I set the wrong flag.  We want this patch in beta, not in aurora.
Comment 15 Sid Stamm [:geekboy or :sstamm] 2013-05-14 12:22:55 PDT
Created attachment 749420 [details] [diff] [review]
fix tests and privacy.js

The last landed (trivial) patch caused oranges in m-b.  

https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=dd4d3bc92ee8

There were some issues with the tests and UI (making assumptions about default) so changing the default wasn't enough.  This patch is hopefully a fix, and it's on try right now.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b9271786b7c3
Comment 16 Sid Stamm [:geekboy or :sstamm] 2013-05-14 12:30:07 PDT
Augh, user ID in the patchfile is wrong.  Should be updated to "Sid Stamm <sstamm@mozilla.com>" before landing.
Comment 17 :Ehsan Akhgari (away Aug 1-5) 2013-05-14 12:31:25 PDT
Comment on attachment 749420 [details] [diff] [review]
fix tests and privacy.js

r=me
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-05-14 12:37:25 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/ab80e4d6ba88
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-19 17:08:59 PDT
Seems like we should just put this behind an #ifndef RELEASE_BUILD (see https://wiki.mozilla.org/Platform/Channel-specific_build_defines) so we don't need to keep landing this patch every 6 weeks?
Comment 20 Sid Stamm [:geekboy or :sstamm] 2013-06-19 17:15:02 PDT
We could, but it's a bunch of one-line changes so tests don't go haywire when we change the pref.  See attachment 749420 [details] [diff] [review].  (Both attachments on this bug have to be landed together to change the default without ill effect.   Might be easier to just reland the patches.)
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-19 18:35:26 PDT
By "this" I meant "the pref flip and associated test changes". There's no reason for us to manually back this out of beta every 6 weeks if the build system can do that for us automatically.
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-06-24 13:21:29 PDT
Not sure if "fixed" is really the right status here, but that's what we did for 22, so why not. I took the liberty of rolling this up into one changeset to make life easier in the event that the last comments don't come to fruition.

https://hg.mozilla.org/releases/mozilla-beta/rev/99622e19b353
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-07-18 15:05:22 PDT
Bogdan, since you are the QA Contact for Bug 818340 can you please verify this is disabled in the latest Beta?
Comment 24 Bogdan Maris, QA [:bogdan_maris] 2013-07-19 06:50:19 PDT
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

Verified that is disabled in Firefox 23 beta 7 (buildID: 20130718163513).
Comment 25 Sid Stamm [:geekboy or :sstamm] 2013-08-06 12:07:55 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/57b72587c11f

I'll work on a patch that lets us stop landing this over and over (you convinced me, gavin).
Comment 26 Sid Stamm [:geekboy or :sstamm] 2013-08-06 12:44:50 PDT
FWIW, this backout (and the original patch) do not affect Android or B2G since there's no UI for it on those platforms.  This is Desktop-only.
Comment 27 Sid Stamm [:geekboy or :sstamm] 2013-08-06 12:50:03 PDT
Created attachment 786454 [details] [diff] [review]
3pc-autopref.diff

This patch makes the backout conditional: apply on top of the other patches (or on top of RyanVM's folded patch in comment 22).

Gavin: is this what you had in mind?
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-08-06 12:58:11 PDT
Comment on attachment 786454 [details] [diff] [review]
3pc-autopref.diff

Right idea, but a couple of issues:

- you'll need to add a "*" to the jar.mn entries for both privacy.js files (to indicate that they should be preprocessed) for this to work. 
- the test files also aren't preprocessed, so to make the ifdefs work there you'll need to adjust their Makefile to do something like http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/Makefile.in#31 .
Comment 29 Bogdan Maris, QA [:bogdan_maris] 2013-08-07 05:15:08 PDT
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0

Verified as disabled on Firefox 24 beta 1 (buildID: 20130806170643).
Comment 30 Sid Stamm [:geekboy or :sstamm] 2013-08-07 09:43:27 PDT
Created attachment 786982 [details] [diff] [review]
3pc-autopref

Updated makefiles to preprocess the test files.  Tested locally on beta (modified tests pass, cookieBehavior is indeed set to default/0 in about:config).  Also tested on central (applied backout patch plus this one, modified tests pass, cookieBehavior is indeed set to default/3 in about:config).

There's no sense in landing this in beta since it's already preffed off there, so I'm going to rebase this patch for aurora/central before flagging for review again.
Comment 31 Sid Stamm [:geekboy or :sstamm] 2013-08-07 11:01:48 PDT
Created attachment 787014 [details] [diff] [review]
auto-revert patch for central and aurora

Applies clean to central and aurora without the backout patch.  Modified tests pass locally, and manually verified the default was "limit" in central/aurora.
Comment 32 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-08-08 11:22:21 PDT
Comment on attachment 787014 [details] [diff] [review]
auto-revert patch for central and aurora

>diff -r ad0ae007aa9e browser/components/preferences/in-content/tests/Makefile.in

>@@ -19,15 +19,23 @@ MOCHITEST_BROWSER_FILES := \

>     browser_privacypane_4.js \

You should remove this file from this list if you're adding it to pp_mochitest_browser_files. (same comment applies to browser/components/preferences/tests/Makefile.in)
Comment 33 Sid Stamm [:geekboy or :sstamm] 2013-08-08 11:31:57 PDT
Created attachment 787695 [details] [diff] [review]
auto-revert patch for central and aurora

Thanks Gavin, didn't notice that that target is used for more than preprocess selection.  Updated, sent to try on linux for a gut check.

https://tbpl.mozilla.org/?tree=Try&rev=9f01a0fb17b2
Comment 34 Sid Stamm [:geekboy or :sstamm] 2013-08-09 11:03:36 PDT
Well, there's one orange xpcshell test on that try, but try was having some issues during that run so I think it's a packaging failure.  Test passes locally (on linux).

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/076b8758ecb0

If it settles well on inbound/central I'll flag it for uplift to Aurora then we can be done with the backing out.
Comment 35 Ryan VanderMeulen [:RyanVM] 2013-08-09 16:41:46 PDT
https://hg.mozilla.org/mozilla-central/rev/076b8758ecb0
Comment 36 Sid Stamm [:geekboy or :sstamm] 2013-08-12 10:36:55 PDT
Comment on attachment 787695 [details] [diff] [review]
auto-revert patch for central and aurora

Looks like it's doing fine on central.  Nominating for uplift so we don't have to deal with manually landing this in beta at the next merge.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): have to manually land patch for this bug after each beta merge
User impact if declined: Potentially we forget and start aggressively blocking cookies 
Testing completed (on m-c, etc.): landed on mozilla-central, conditional includes work there.  Logic has been in the last three betas.
Risk to taking this patch (and alternatives if risky): Not risky. 
String or IDL/UUID changes made by this patch: None.  Just a pref and some tests behind an #ifdef.
Comment 37 Ryan VanderMeulen [:RyanVM] 2013-08-13 05:17:24 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b2e185d15a4
Comment 38 Bogdan Maris, QA [:bogdan_maris] 2013-09-09 07:40:08 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:25.0) Gecko/20100101 Firefox/25.0

Verified as fixed with latest Aurora 25.0a2 (Build ID: 20130909004001).
Comment 39 Bogdan Maris, QA [:bogdan_maris] 2013-10-30 01:18:28 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed with latest Firefox 26 beta 1 (buildID: 20131028225529).

Note You need to log in before you can comment on or make changes to this bug.