Policy: "block" under "cookies" should clear already-stored cookies

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: yuki, Assigned: yuki)

Tracking

Trunk
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

Last year
"cookies" allows to specify policy for newly stored cookies, but already stored cookies are out of control. When the policy is updated to block cookies, already stored cookies should be cleared to avoid problems caused by old values.
Assignee

Comment 1

Last year
I'm trying to implement this behavior but the automated test still fails. Is any missing part for the test?
Comment on attachment 8956029 [details] [diff] [review]
Clear stored cookies for cookie blocked hosts (WIP)

I didn't run it myself, so I don't know why the test is failing. How is JSON.stringify(cookies) and JSON.stringify(expected) differing?

Can you use the function removeCookiesWithOriginAttributes to avoid the manual iteration?

Also, you should wrap the newly added block to Policies.jsm with the runOnce helper function because we don't want to be doing that on every startup because that would be a performance hit
(In reply to :Felipe Gomes (needinfo me!) from comment #2)

> Can you use the function removeCookiesWithOriginAttributes to avoid the
> manual iteration?

Looks like you can, by calling Services.cookies.removeCookiesWithOriginAttributes("{}", host);

note: each element of the param.Block array is already an nsIURI object, so you don't need to do a newURI call on it
Assignee

Comment 4

Last year
(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> Looks like you can, by calling
> Services.cookies.removeCookiesWithOriginAttributes("{}", host);
> 
> note: each element of the param.Block array is already an nsIURI object, so
> you don't need to do a newURI call on it

Thank you for reviewing and advises! I misunderstood the spec of param.Block. After some changes the test succeeded as expected.
Assignee

Comment 5

Last year
Finalized patch.
Attachment #8956029 - Attachment is obsolete: true
Attachment #8956318 - Flags: review?(felipc)
Comment on attachment 8956318 [details] [diff] [review]
Clear stored cookies for cookie blocked hosts

Review of attachment 8956318 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

I'll just double check with jdm that this is the correct usage for this removeCookiesWithOriginAttributes function.
Attachment #8956318 - Flags: review?(josh)
Attachment #8956318 - Flags: review?(felipc)
Attachment #8956318 - Flags: review+
Attachment #8956318 - Flags: review?(josh) → review+
Assignee

Updated

Last year
Keywords: checkin-needed
Assignee: nobody → yuki

Comment 7

Last year
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5521f4a9012d
Policy: "block" under "cookies" should clear already-stored cookies. r=felipc, r=jdm
Keywords: checkin-needed
Assignee

Comment 10

Last year
(In reply to Cosmin Sabou [:cosmin_s] from comment #9)
> Also please take a look at these failures:
> https://treeherder.mozilla.org/logviewer.html#?job_id=166673261&repo=mozilla-
> inbound&lineNumber=2003
> 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=5521f4a9012d88d6aa399bc16b27ec0013976526&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&selectedJob=166673261&filter-searchStr=tv

Except eslint errors, I couldn't reproduce the failure with a command line:

----
$ ./mach test browser/components/enterprisepolicies/tests/br
owser/browser_policy_blocked_cookies.js
----

I still cannot find out the reason why it succeeds on my environment but fails on CI...
Flags: needinfo?(yuki)
Assignee

Comment 11

Last year
This just fixes eslint erros.
Attachment #8956318 - Attachment is obsolete: true
to reproduce TV failure locally, you need to specify --verify after "test".

then, the issue is that runOncePerModification function early-returns because the value is same as previous one, 
that is stored in browser.policies.runOncePerModification..clearCookiesForBlockedHosts.
So, you need to clear the pref in teardown.
(In reply to Tooru Fujisawa [:arai] from comment #12)
> So, you need to clear the pref in teardown.

sorry, this wasn't proper fix.
iiuc the same issue can happen outside of test.
So, the pref should be cleared when "Cookies" or "Block" isn't present for newly set policy.
so that it will be cleared by setupPolicyEngineWithJson("") in testcase
Assignee

Comment 14

Last year
(In reply to Tooru Fujisawa [:arai] from comment #12)
> to reproduce TV failure locally, you need to specify --verify after "test".
> 
> then, the issue is that runOncePerModification function early-returns
> because the value is same as previous one, 
> that is stored in
> browser.policies.runOncePerModification..clearCookiesForBlockedHosts.
> So, you need to clear the pref in teardown.

Thanks a lot! Now I'm updating my patch with clearing of "runOnce" state.
Assignee

Comment 15

Last year
Update patch. Changes from v1:

 * fix eslint errors.
 * clear "runOnce" state for every test.
Attachment #8957115 - Attachment is obsolete: true
Attachment #8957217 - Flags: review?(felipc)
Comment on attachment 8957217 [details] [diff] [review]
Clear stored cookies for cookie blocked hosts v2.1

>diff --git a/browser/components/enterprisepolicies/Policies.jsm b/browser/components/enterprisepolicies/Policies.jsm
>+
>+  resetRunOnceState: function resetRunOnceState() {
>+    const runOnceBaseKeys = [
>+      "browser.policies.runonce.",
>+      "browser.policies.runOncePerModification."
>+    ];
>+    for (let base of runOnceBaseKeys) {
>+      for (let key of Services.prefs.getChildList(base, {})) {
>+        if (Services.prefs.prefHasUserValue(key))
>+          Services.prefs.clearUserPref(key);
>+      }
>+    }
>+  },

Thanks for doing this!!
Instead of calling it directly from your test, can you call resetRunOnceState from the cleanup function in head.js?
https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/tests/browser/head.js#24
Assignee

Comment 17

Last year
(In reply to :Felipe Gomes (needinfo me!) from comment #16)
> Instead of calling it directly from your test, can you call
> resetRunOnceState from the cleanup function in head.js?
> https://searchfox.org/mozilla-central/source/browser/components/
> enterprisepolicies/tests/browser/head.js#24

Yes, I tried that, but it seems to break the test for SetHomepage policy, so I couldn't decide that it should be done in head.js. I need more research for it...
Comment on attachment 8957217 [details] [diff] [review]
Clear stored cookies for cookie blocked hosts v2.1

Review of attachment 8957217 [details] [diff] [review]:
-----------------------------------------------------------------

Ok! So let's use this version for now. But if you find the reason for that breakage of the other test, let me know
Attachment #8957217 - Flags: review?(felipc) → review+
Assignee

Updated

Last year
Keywords: checkin-needed

Comment 19

Last year
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eae9d89985b
Policy: "block" under "cookies" should clear already-stored cookies r=felipc
Keywords: checkin-needed
Assignee

Comment 20

Last year
(In reply to :Felipe Gomes (needinfo me!) from comment #18)
> Ok! So let's use this version for now. But if you find the reason for that
> breakage of the other test, let me know

Finally I found the cause why the test is broken. This is a patch to fix the breakage. I think it should be applied after the current v2.1 patch is land.

Comment 21

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/7eae9d89985b
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment on attachment 8957601 [details] [diff] [review]
clear "runOnce" state for every test

(marking myself for review so I don't forget about it)
Attachment #8957601 - Flags: review?(felipc)
Comment on attachment 8957601 [details] [diff] [review]
clear "runOnce" state for every test

Review of attachment 8957601 [details] [diff] [review]:
-----------------------------------------------------------------

(I moved this patch to bug 1446508 and gave r+ there)
Attachment #8957601 - Flags: review?(felipc)
You need to log in before you can comment on or make changes to this bug.