Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

Trunk
Firefox 19
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ fixed)

Details

(Whiteboard: [Fx17][MemShrink][qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 672650 [details] [diff] [review]
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
Attachment #672650 - Flags: review?(mixedpuppy)
Attachment #672650 - Flags: review?(mixedpuppy) → review+
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.
Attachment #672650 - Flags: review?(mixedpuppy)
(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.
Attachment #672650 - Flags: review?(mixedpuppy) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd5c75c14f2
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
Blocks: 802671
tracking-firefox17: ? → +
tracking-firefox18: ? → +
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.
Attachment #672650 - Flags: approval-mozilla-beta+
Attachment #672650 - Flags: approval-mozilla-aurora+

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/fdd5c75c14f2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6278eb415795
https://hg.mozilla.org/releases/mozilla-beta/rev/f465f395240b
status-firefox17: --- → fixed
status-firefox18: --- → fixed
> FWIW, the patch looks good and is covered by tests...

Did the tests previously exist? I don't see them in the patch landings.
Flags: in-testsuite?
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.