Last Comment Bug 802929 - A new port is created on every social.cookies-get message
: A new port is created on every social.cookies-get message
Status: RESOLVED FIXED
[Fx17][MemShrink][qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: :Felipe Gomes (needinfo me!) [offline until Jun 24]
:
Mentors:
Depends on:
Blocks: 802671
  Show dependency treegraph
 
Reported: 2012-10-17 20:01 PDT by :Felipe Gomes (needinfo me!) [offline until Jun 24]
Modified: 2012-12-04 15:54 PST (History)
5 users (show)
markh: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
Patch (1.12 KB, patch)
2012-10-17 20:01 PDT, :Felipe Gomes (needinfo me!) [offline until Jun 24]
jaws: review+
mixedpuppy: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Review

Description :Felipe Gomes (needinfo me!) [offline until Jun 24] 2012-10-17 20:01:57 PDT
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
Comment 1 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-17 20:31:32 PDT
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] 2012-10-17 21:23:19 PDT
(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) [on leave 5/16-7/16] 2012-10-17 22:25:47 PDT
Comment on attachment 672650 [details] [diff] [review]
Patch

I don't see any issue here, seems more efficient for our implementation.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-18 08:19:18 PDT
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] 2012-10-18 10:34:45 PDT
https://hg.mozilla.org/mozilla-central/rev/fdd5c75c14f2
Comment 8 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 15:10:56 PST
> 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] 2012-12-04 15:46:28 PST
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 15:54:22 PST
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.

Note You need to log in before you can comment on or make changes to this bug.