Last Comment Bug 807532 - notification panel documents leaked after disabling social functionality
: notification panel documents leaked after disabling social functionality
Status: VERIFIED FIXED
[MemShrink]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: :Felipe Gomes (needinfo me!)
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on:
Blocks: 805246
  Show dependency treegraph
 
Reported: 2012-10-31 17:19 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-12-04 15:09 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
Patch (838 bytes, patch)
2012-10-31 18:35 PDT, :Felipe Gomes (needinfo me!)
gavin.sharp: review+
markh: review+
Details | Diff | Splinter Review
Set profile to undefined (839 bytes, patch)
2012-10-31 19:04 PDT, :Felipe Gomes (needinfo me!)
felipc: review+
Details | Diff | Splinter Review
Set profile to undefined (844 bytes, patch)
2012-10-31 19:17 PDT, :Felipe Gomes (needinfo me!)
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-31 17:19:59 PDT
See bug 805246 comment 18.

STR:
1) Enable social functionality, let it load
2) Check about:memory, notice three contentpanel.php functions (one corresponding to each notification button)
3) Disable the social functionality from the Tools menu
4) wait some time, run "minimize memory usage" in about:memory

Expected: three contentpanel.php entries disappear
Actual: they seem to stay around indefinitely

SocialToolbar.updateButtonHiddenState removes the iframes from the DOM, so I'm not sure how this is happening. It's possible they're being leaked some other way?
Comment 1 :Felipe Gomes (needinfo me!) 2012-10-31 18:35:19 PDT
Created attachment 677247 [details] [diff] [review]
Patch

updateButtonHiddenState is the function that is meant to remove the panels, but SocialUI.haveLoggedInUser() keep returning true even after the feature is turned off, because the profile is not cleared

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js#704
Comment 2 Mark Hammond [:markh] 2012-10-31 18:49:05 PDT
Comment on attachment 677247 [details] [diff] [review]
Patch

I think this is fine, as if we have a code path which doesn't reach this on social being disabled we have bigger problems (eg, the worker is still running).

Longer term I think we should set .provider to null, but that is risker for 17/18.
Comment 3 Mark Hammond [:markh] 2012-10-31 18:53:47 PDT
Comment on attachment 677247 [details] [diff] [review]
Patch

Actually, it possibly should be set to undefined?  The difference between null and undefined will be if the "toolbar cache" is used on re-enabling the feature.
Comment 4 :Felipe Gomes (needinfo me!) 2012-10-31 19:04:34 PDT
Created attachment 677250 [details] [diff] [review]
Set profile to undefined
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-31 19:05:19 PDT
Comment on attachment 677247 [details] [diff] [review]
Patch

Ah, this makes sense. undefined as Mark suggests is the way to go, since that's the initial value before social is enabled.
Comment 6 :Felipe Gomes (needinfo me!) 2012-10-31 19:17:21 PDT
Created attachment 677251 [details] [diff] [review]
Set profile to undefined
Comment 7 :Felipe Gomes (needinfo me!) 2012-10-31 21:02:01 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/668d22da982b
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-31 21:29:39 PDT
Comment on attachment 677251 [details] [diff] [review]
Set profile to undefined

[Triage Comment]
very simple "leak" fix, let's get this on aurora/beta
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-31 21:30:29 PDT
Hmm, not really related to this bug, but we should probably also be clearing ambientNotificationIcons, right?
Comment 10 Mark Hammond [:markh] 2012-10-31 22:56:37 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Hmm, not really related to this bug, but we should probably also be clearing
> ambientNotificationIcons, right?

yes, we should.  I don't think the toolbars will be impacted (as it wants a profile before it uses them) but SocialMenu looks like it might still show them (it only looks at Social.active and doesn't take the lack of a profile into account)
Comment 11 Ed Morley [:emorley] 2012-11-01 06:48:56 PDT
https://hg.mozilla.org/mozilla-central/rev/668d22da982b
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 15:09:35 PST
Verified fixed using steps in comment 0.

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