Closed Bug 1255569 Opened 4 years ago Closed 4 years ago

Add coverage for cookie eviction events for expired cookies

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.2 - Apr 4
Tracking Status
firefox48 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

Splitting this out from bug 1236118 because everything else is simple and straighforward so I plan to land the rest of that bug separately from this.

Missing coverage can be seen at https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/components/extensions/ext-cookies.js.html
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
Flags: blocking-webextensions+
I've been looking into this, and wrote a test that I thought would work, but it seems that I need to do something to trigger the eviction based on `expirationDate`, and I'm not sure how to do that. Any suggestions?

Also, according to MDN [1]:

`If a cookie was removed due to being overwritten with an already-expired expiration date, "cause" will be set to "expired_overwrite".`

However a test that I wrote that does just that (overwrites an existing cookie with one with an already expired expiration date) does not generate an event with a cause of "expired_overwrite".

Finally, I also noticed in my testing that the following statement on MDN [2] seems to be untrue:

`As a special case, note that updating a cookie's properties is implemented as a two step process: the cookie to be updated is first removed entirely, generating a notification with "cause" of "overwrite" . Afterwards, a new cookie is written with the updated values, generating a second notification with "cause" "explicit".`

I do not see two events in my handler when overwriting a cookie. I see only one with `cause` of `overwrite` and `removed` of `false`. Is this a bug in the API? 

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/cookies/OnChangedCause
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/cookies/onChanged
baku, do you have any advice on the above?
Flags: needinfo?(amarchesini)
Kris, I know you're really busy, but if you have a chance to look into how we might test cookie eviction for expired cookies, that would be great.

I did a test to see if an expired-eviction would be triggered via an eviction for too many cookies (by manually setting `network.cookie.maxPerHost` and adding too many cookies), but it was not.
Flags: needinfo?(kmaglione+bmo)
I'm not really familiar with this cookie eviction, sorry.
Flags: needinfo?(amarchesini)
I think the easiest thing to do would be to set a cookie for a host, wait for it to expire, and then set a new cookie with the same name. That should give you a removal event with cause=expired rather than cause=overwrite.
Flags: needinfo?(kmaglione+bmo)
I added a test as suggested, but I know there's something wrong with it. It only passes intermittently and I'm sure it's a timing thing, but I'm not sure how to address it. I've tried a bunch of different values in the `setTimeout` call but none of them seem to be reliable. Either the test passes, or it fails because the listener is only called once, rather than twice, and with a value of `false:explicit`. I'm not sure what condition is causing that failing outcome. It seems like the original cookie isn't getting set first, or somehow disappears. Even with a value of 5000 for setTimeout the test is intermittent, so I don't think it's because I am not waiting long enough.
Attachment #8736066 - Flags: review?(kmaglione+bmo)
Comment on attachment 8736066 [details]
MozReview Request: Bug 1255569 - Add coverage for cookie eviction events for expired cookies, r?kmag

https://reviewboard.mozilla.org/r/43069/#review39671

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:191
(Diff revision 1)
> +    let expectedEvents = [];
> +
> +    browser.cookies.onChanged.addListener(event => {
> +      expectedEvents.push(`${event.removed}:${event.cause}`);
> +      if (expectedEvents.length === 1) {
> +        browser.test.assertEq("true:expired", expectedEvents[0], "expired cookie removed");

We should check the name, too.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:200
(Diff revision 1)
> +      }
> +    });
> +
> +    setTimeout(() => {
> +      browser.test.sendMessage("change-cookies");
> +    }, 2500);

This timeout is too long. A second should be sufficient. `Date.now() % 1000` would probably be better.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:213
(Diff revision 1)
> +    background: `(${background})()`,
> +  });
> +
> +  let cookieSvc = SpecialPowers.Services.cookies;
> +
> +  cookieSvc.add(domain, "/", "first", "one", false, false, false, Date.now() / 1000 + 0.5);

The expiry time is an integer. Adding .5 to it won't have any effect unless `time % 1 >= .5`.

We should probably do this in a loop, and keep retrying until the cookie has been successfully created. Otherwise, it's too easy for the time to be valid when we calculate it, but invalid by the time the cookie service checks it.
I tried rewriting the test to use the API to set the cookies, via `cookies.set` but what's odd is that no matter how I structured it, including waits in different places, I always got `true:overwrite` instead of `true:expired` when the cookie was re-written.
https://reviewboard.mozilla.org/r/43069/#review39671

> This timeout is too long. A second should be sufficient. `Date.now() % 1000` would probably be better.

Interestingly, when I change this to `Date.now() % 1000` or a similarly small number, I consistently get `true:evicted` as the first fired event, rather than `true:expired`. What is that eviction about? Is it because a new cookie is being set before the old one "takes"? It seems very odd to me.
https://reviewboard.mozilla.org/r/43069/#review39671

> Interestingly, when I change this to `Date.now() % 1000` or a similarly small number, I consistently get `true:evicted` as the first fired event, rather than `true:expired`. What is that eviction about? Is it because a new cookie is being set before the old one "takes"? It seems very odd to me.

No, that's just a bug in ext-cookies.js. It decides whether a cookie was evicted or expired based on its expiration time. It checks `(cookie.expiry + 1) * 1000 <= Date.now()`, when it should be something more like `cookie.expiry * 1000 <= Date.now()`.
Comment on attachment 8736066 [details]
MozReview Request: Bug 1255569 - Add coverage for cookie eviction events for expired cookies, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43069/diff/1-2/
Attachment #8736066 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/43069/#review39671

> No, that's just a bug in ext-cookies.js. It decides whether a cookie was evicted or expired based on its expiration time. It checks `(cookie.expiry + 1) * 1000 <= Date.now()`, when it should be something more like `cookie.expiry * 1000 <= Date.now()`.

Thanks. I fixed that bug in the next revision as it was messing with the test. Also, `Date.now() % 1000` doesn't seem to be quite enough, probably because the expiry date for the cookie is 1 second in the future. I found that `1000` seems to pass consistently though.
Attachment #8736066 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8736066 [details]
MozReview Request: Bug 1255569 - Add coverage for cookie eviction events for expired cookies, r?kmag

https://reviewboard.mozilla.org/r/43069/#review39687

Looks good. Thanks.
Comment on attachment 8736066 [details]
MozReview Request: Bug 1255569 - Add coverage for cookie eviction events for expired cookies, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43069/diff/2-3/
Iteration: 48.1 - Mar 21 → 48.2 - Apr 4
Keywords: checkin-needed
Whiteboard: triaged
Here's a link to the failure in the log: https://treeherder.mozilla.org/logviewer.html#?job_id=8345807&repo=fx-team. The message is "INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_cookies.html | (The exception cannot be converted to string.)".

I will investigate further.
Comment on attachment 8736066 [details]
MozReview Request: Bug 1255569 - Add coverage for cookie eviction events for expired cookies, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43069/diff/3-4/
It turns out we cannot use `SpecialPowers.Services.cookies` with e10s, so I indicated in the manifest not to run this test with e10s (as is currently being done for `test_ext_cookies_permissions.html` [1].

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/mochitest.ini#66
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e0b6fd97fd6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.