The default bug view has changed. See this FAQ.

Disable bug 818340 for Beta until we're ready

VERIFIED FIXED in Firefox 22

Status

()

Core
Networking: Cookies
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: akeybl, Assigned: geekboy)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ fixed, firefox23+ verified, firefox24+ verified, firefox25+ verified, firefox26+ verified)

Details

(Whiteboard: [no-nag])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Making sure we're tracking whether or not we should uplift bug 818340.

Updated

4 years ago
See Also: → bug 845353
(Reporter)

Comment 1

4 years ago
To clarify, this is around merges. Should this feature remain enabled at each merge point?
(Assignee)

Comment 2

4 years ago
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

4 years ago
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.
(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
(Assignee)

Comment 5

4 years ago
(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.
(Reporter)

Comment 6

4 years ago
Assigning to Dolske for final resolution, as he reviewed bug 818340.
Assignee: nobody → dolske
status-firefox21: --- → unaffected
status-firefox22: --- → affected
tracking-firefox22: ? → +
Summary: Should bug 818340 be uplifted to Aurora? Beta? Release? → Disable bug 818340 in FF22 Beta 4
Whiteboard: [no-nag]
(Bumping to Sid, since he's actually tracking this feature.)
Assignee: dolske → sstamm
(Assignee)

Comment 8

4 years ago
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.
Attachment #729195 - Flags: review?(dolske)
Attachment #729195 - Flags: review?(dolske) → review+
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.
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
(Assignee)

Comment 11

4 years ago
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.
Attachment #729195 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

4 years ago
status-firefox23: --- → affected
tracking-firefox23: --- → +
Summary: Disable bug 818340 in FF22 Beta 4 → Disable bug 818340 for Beta until we're ready
(Reporter)

Comment 12

4 years ago
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.
Attachment #729195 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Updated

4 years ago
Attachment #729195 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
status-firefox24: --- → ?
tracking-firefox24: --- → ?
(Assignee)

Comment 13

4 years ago
Ah yeah, whoops, I set the wrong flag.  We want this patch in beta, not in aurora.
https://hg.mozilla.org/releases/mozilla-beta/rev/dd4d3bc92ee8
status-firefox22: affected → fixed
(Assignee)

Comment 15

4 years ago
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
Attachment #729195 - Attachment is obsolete: true
Attachment #749420 - Flags: review?(dolske)
(Assignee)

Updated

4 years ago
Attachment #729195 - Attachment is obsolete: false
(Assignee)

Comment 16

4 years ago
Augh, user ID in the patchfile is wrong.  Should be updated to "Sid Stamm <sstamm@mozilla.com>" before landing.
Comment on attachment 749420 [details] [diff] [review]
fix tests and privacy.js

r=me
Attachment #749420 - Flags: review?(dolske) → review+
https://hg.mozilla.org/releases/mozilla-beta/rev/ab80e4d6ba88
(Reporter)

Updated

4 years ago
Attachment #749420 - Flags: approval-mozilla-beta+

Updated

4 years ago
status-firefox24: ? → affected
tracking-firefox24: ? → +
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?
(Assignee)

Comment 20

4 years ago
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.)
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.
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
status-firefox23: affected → fixed
Bogdan, since you are the QA Contact for Bug 818340 can you please verify this is disabled in the latest Beta?
Keywords: verifyme
QA Contact: bogdan.maris
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).
status-firefox23: fixed → verified

Updated

4 years ago
tracking-firefox25: --- → ?
(Assignee)

Comment 25

4 years ago
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).
status-firefox24: affected → fixed
Blocks: 902112
(Assignee)

Comment 26

4 years ago
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.
(Assignee)

Comment 27

4 years ago
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?
Attachment #786454 - Flags: review?(gavin.sharp)
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 .
Attachment #786454 - Flags: review?(gavin.sharp) → review-
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).
status-firefox24: fixed → verified
(Assignee)

Comment 30

4 years ago
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.
Attachment #786454 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
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.
Attachment #786982 - Attachment is obsolete: true
Attachment #787014 - Flags: review?(gavin.sharp)
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)
Attachment #787014 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 33

4 years ago
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
Attachment #787014 - Attachment is obsolete: true
Attachment #787695 - Flags: review+
(Assignee)

Comment 34

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/076b8758ecb0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Comment 36

4 years ago
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.
Attachment #787695 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

4 years ago
status-firefox25: --- → affected
status-firefox26: --- → affected
tracking-firefox25: ? → +
tracking-firefox26: --- → +
(Reporter)

Updated

4 years ago
Attachment #787695 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b2e185d15a4
status-firefox25: affected → fixed
status-firefox26: affected → fixed
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).
status-firefox25: fixed → verified
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).
Status: RESOLVED → VERIFIED
status-firefox26: fixed → verified
Keywords: verifyme

Updated

3 years ago
Depends on: 999170
You need to log in before you can comment on or make changes to this bug.