Closed Bug 802929 Opened 7 years ago Closed 7 years ago
A new port is created on every social
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
Whiteboard: [Fx17] → [Fx17][MemShrink]
Comment on attachment 672650 [details] [diff] [review] Patch Shane, it would still be good if you could take a look at this.
(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 on attachment 672650 [details] [diff] [review] Patch I don't see any issue here, seems more efficient for our implementation.
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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
> FWIW, the patch looks good and is covered by tests... Did the tests previously exist? I don't see them in the patch landings.
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.
Flags: in-testsuite? → in-testsuite-
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.
Whiteboard: [Fx17][MemShrink] → [Fx17][MemShrink][qa-]
You need to log in before you can comment on or make changes to this bug.