1255569 - Add coverage for cookie eviction events for expired cookies

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

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

Comment 1

[Bob Silverberg [:bsilverberg]]

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

Comment 2

[Bob Silverberg [:bsilverberg]]

baku, do you have any advice on the above?

Comment 3

[Bob Silverberg [:bsilverberg]]

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.

Comment 4

[Andrea Marchesini [:baku]]

I'm not really familiar with this cookie eviction, sorry.

Comment 5

[Kris Maglione [:kmag]]

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.

Comment 6

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1255569 - Add coverage for cookie eviction events for expired cookies, r?kmag

Review commit: https://reviewboard.mozilla.org/r/43069/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43069/

Comment 7

[Bob Silverberg [:bsilverberg]]

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.

Comment 8

[Kris Maglione [:kmag]]

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.

Comment 9

[Bob Silverberg [:bsilverberg]]

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.

Comment 10

[Bob Silverberg [:bsilverberg]]

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.

Comment 11

[Kris Maglione [:kmag]]

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 12

[Bob Silverberg [:bsilverberg]]

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/

Comment 13

[Bob Silverberg [:bsilverberg]]

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.

Comment 14

[Kris Maglione [:kmag]]

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 15

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b8e1db8eb80

Comment 16

[Bob Silverberg [:bsilverberg]]

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/

Comment 17

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3035bb02e65

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0228e00caf59

Comment 19

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/1ffc15e90603

Comment 20

[Bob Silverberg [:bsilverberg]]

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 21

[Bob Silverberg [:bsilverberg]]

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/

Comment 22

[Bob Silverberg [:bsilverberg]]

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

Comment 23

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6891d840d6ed

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0b6fd97fd6

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e0b6fd97fd6