Remove sync versions of document.cookie getter/setter

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: tjr, Assigned: dpino)

Tracking

(Blocks 1 bug, {good-first-bug})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

10 months ago
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)

Comment 1

10 months ago
I think so, but let's double check with Jason too.
Flags: needinfo?(ehsan) → needinfo?(jduell.mcbugs)

Comment 2

10 months ago
Yes, I assume that would be fine.
Flags: needinfo?(jduell.mcbugs)
Priority: -- → P3
Whiteboard: [necko-triaged]
Reporter

Updated

10 months ago
See Also: → 1484026
Assignee

Comment 3

9 months ago
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)
Reporter

Comment 4

9 months ago
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)
Assignee

Comment 5

9 months ago
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)

Comment 6

9 months ago
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...
Reporter

Comment 7

9 months ago
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
Reporter

Comment 8

9 months ago
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 9

9 months ago
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 10

9 months ago
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)
Assignee

Comment 11

9 months ago
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+

Comment 13

9 months ago
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

Comment 14

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d47f566e1399
Status: NEW → RESOLVED
Closed: 9 months 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.