463127 - nsICookieManager/CookieService should allow creation of arbitrary cookies for testing

Status: NEW

(Core :: Networking: Cookies, enhancement, P3)

Description

[Johnathan Nightingale [:johnath]]

Writing tests for bug 453440, I needed to create cookies with arbitrary creation times, so that I could test clear private data's time-based functionality.  For downloads I can access the database to do this, with places I can do something similar, but the add/setCookieString functions don't support alternate creation time.

If we resolve this bug to provide that mechanism, we should also update the tests added in bug 453440.

Comment 1

[[e-mail]]

easy enough to add a creationtime arg to the add() method, though i'm not sure exactly how we'd do it neatly so consumers who want to use the current time don't have to jump through hoops... (or perhaps a single hoop, since getting the time in ms and multiplying by 1000 isn't that hard in js)

Comment 2

[Johnathan Nightingale [:johnath]]

Yeah, and given that it's only really needed for testing, it might make more sense to do something more primitive - expose the dbconnection like nsIDownloadManager does, maybe?

Comment 3

[[e-mail]]

nope, since cookies has its own in-memory store, the db is just the on-disk part of it.

so we need the API to provide this specifically. having two methods that do almost exactly the same thing would be a bit weird, but if there are other reasons to do it, then sure...

Comment 4

[Emma Zühlcke [:emz]]

This still seems worth having! Particularly for the data sanitization tests.

Comment 5

[edgul]

What format/type should this API take?

Comment 6

[Emma Zühlcke [:emz]]

(In reply to edgul from comment #5)

What format/type should this API take?

Could be an extension of https://searchfox.org/mozilla-central/rev/345ec3c55ddda5f0ce37168f0644dcdcc4834409/netwerk/cookie/nsICookieManager.idl#128 with an optional argument to set the creation time. Or we add a separate test-only method. That might be cleaner.
Hannah tried that a while ago, but we rolled the change back due to some regression in release.

Comment 7

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

I think the addForTest method added in https://phabricator.services.mozilla.com/D170377#change-HC1mDTCLgPWk was about right.
The problem with the implementation is that it ended up calling AddNative, and passing an invalid timestamp - see regression in bug 1827669.