802929 - A new port is created on every social.cookies-get message

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

social.cookies-get calls getFrameWorkerHandle, which unconditionally creates a new port stores it forever on the worker. There is no need to create a new port since WorkerAPI already has its own _port, and all it needs is to access window.document.cookie

getFrameWorkerHandle probably should not need to create a port every time, but that's a different question for another bug.

This problem is bad because social.cookies-get gets called about once a sec, so a simple fix is to just not call getFrameWorkerHandle

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 672650 [details] [diff] [review]
Patch

Shane, it would still be good if you could take a look at this.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Felipe Gomes from comment #0)
> getFrameWorkerHandle probably should not need to create a port every time,
> but that's a different question for another bug.

Actually, I think that is fine.  Ports are supposed to be fairly light-weight in a real worker, and having multiple ports makes some things much simpler to rationalize about - eg, the user-recommend-prompt handling in browser-social.js creates a short-lived port just to do its thing and that would end up more complicated if it had to share a port with anything.

FWIW, the patch looks good and is covered by tests...

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 672650 [details] [diff] [review]
Patch

I don't see any issue here, seems more efficient for our implementation.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd5c75c14f2

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 672650 [details] [diff] [review]
Patch

[Triage Comment]
very low-risk, obvious social-only fix for a potentially serious performance/memory problem. approved for beta/aurora.

Comment 6

[Ed Morley [:emorley]]

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

Comment 7

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6278eb415795
https://hg.mozilla.org/releases/mozilla-beta/rev/f465f395240b

Comment 8

[u279076]

> FWIW, the patch looks good and is covered by tests...

Did the tests previously exist? I don't see them in the patch landings.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

There are no tests specifically around the fact we leaked a port, but there are existing tests that the functionality touched by the tests continues to work.  So I guess that means in-testsuite- for this one, but I don't think that's an issue.

Comment 10

[u279076]

Okay, thanks Mark. I'll flag this [qa-] for now as I don't see anything here to verify. If there's something you want me to check, please add the verifyme keyword.