Closed Bug 1483986 Opened 2 years ago Closed 2 years ago

Remove sync versions of document.cookie getter/setter

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: tjr, Assigned: dpino)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(ehsan)
I think so, but let's double check with Jason too.
Flags: needinfo?(ehsan) → needinfo?(jduell.mcbugs)
Yes, I assume that would be fine.
Flags: needinfo?(jduell.mcbugs)
Priority: -- → P3
Whiteboard: [necko-triaged]
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.
Attachment #9009857 - Flags: review?(tom)
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.
Attachment #9009857 - Flags: review?(tom)
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
Attachment #9009857 - Attachment is obsolete: true
Attachment #9010647 - Flags: feedback?(tom)
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...
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 on attachment 9010647 [details] [diff] [review]
Bug-1483986-Remove-sync-versions-of-document.cookie-.patch

Passing to Ehsan for review
Attachment #9010647 - Flags: review?(ehsan)
Attachment #9010647 - Flags: feedback?(tom)
Attachment #9010647 - Flags: feedback+
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!
Attachment #9010647 - Flags: review?(ehsan) → review+
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.
Attachment #9010647 - Flags: review?(nfroyd)
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 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.
Attachment #9010647 - Flags: review?(nfroyd) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d47f566e1399
Remove sync versions of document.cookie getter/setter; r=ehsan,froydnj
https://hg.mozilla.org/mozilla-central/rev/d47f566e1399
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: nobody → dpino
You need to log in before you can comment on or make changes to this bug.