1483986 - Remove sync versions of document.cookie getter/setter

Status: RESOLVED FIXED

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

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

In Bug 1331680 we refactored the document.cookie getter/setter. We still have the old version of the code left behind behind the network.cookie.ipc.sync pref.

Can/should we remove that pref and old code paths?

Comment 1

[(no longer active)]

I think so, but let's double check with Jason too.

Comment 2

[Jason Duell]

Yes, I assume that would be fine.

Comment 3

[Diego Pino]

Attachment: Bug-1483986-Remove-sync-versions-of-document.cookie-.patch

I gave an attempt to solving this bug. I removed the 'network.cookie.ipc.sync' preference and the member variable 'mIPCSync' it was guarding. I also changed two tests that referred to the ipc sync preference.

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9009857 [details] [diff] [review]
Bug-1483986-Remove-sync-versions-of-document.cookie-.patch

Hey that's great, thanks for tackling this!  Ultimately you should flag jduell (or whomever he redirects to) but I can give an initial review: go further!

You removed the only call to GetCookieStringSyncIPC, so you can remove that function too.

And that is the only thing that calls the ipdl method GetCookieString/RecvGetCookieString/SendGetCookieString

We'll also want to see if those tests still run successfully with the pref removed.  Given Comment 221 on Bug 1331680 - they may not.

Comment 5

[Diego Pino]

Attachment: Bug-1483986-Remove-sync-versions-of-document.cookie-.patch

I amended the patch to remove CookieServiceChild::GetCookieStringSyncIPC and GetCookieString/SendGetCookieString/RecvGetCookieString.

Regarding the tests, I run them manually and they both pass.

$ ./mach test netwerk/test/unit_ipc/test_bug528292_wrap.js
xpcshell
~~~~~~~~
Ran 3 checks (1 tests, 2 subtests)
Expected results: 3
OK


$ ./mach test netwerk/test/unit_ipc/test_multipart_streamconv_wrap.js
xpcshell
~~~~~~~~
Ran 10 checks (1 tests, 9 subtests)
Expected results: 10
OK

Comment 6

[(no longer active)]

FWIW I would be more than happy to help review the final version of the patch.  :-)

Also, this will be a pretty major change, and while the tests in netwerk/test would probably be the most impacted, it would be nice to test your patch against all of our tests.  Not sure if you already have try server access or not (it allows you to post a patch and run it against our full CI infrastructure) but if not, please read https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server and file a bug to get commit access.  I'd be happy to vouch if you needinfo me on the bug...

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I sent in a try run for your patch; as Ehsan said you can also get access to do this yourself. 

Patch looks good to me, but as I said, I can't do final review, I'll let Ehsan do that.

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

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9010647 [details] [diff] [review]
Bug-1483986-Remove-sync-versions-of-document.cookie-.patch

Passing to Ehsan for review

Comment 9

[(no longer active)]

Comment on attachment 9010647 [details] [diff] [review]
Bug-1483986-Remove-sync-versions-of-document.cookie-.patch

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

Very nice, thanks a lot for the patch!

Comment 10

[(no longer active)]

Comment on attachment 9010647 [details] [diff] [review]
Bug-1483986-Remove-sync-versions-of-document.cookie-.patch

The sync-messages.ini change needs a review from an IPC peer too.

Comment 11

[Diego Pino]

Thanks Ehsan and Tom for the review and pushing it to the try server. I've requested try server access now. Bug filed here: https://bugzilla.mozilla.org/show_bug.cgi?id=1493072

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 9010647 [details] [diff] [review]
Bug-1483986-Remove-sync-versions-of-document.cookie-.patch

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

Sync message removal is the best kind of review.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d47f566e1399
Remove sync versions of document.cookie getter/setter; r=ehsan,froydnj

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d47f566e1399