Closed
Bug 807532
Opened 12 years ago
Closed 12 years ago
notification panel documents leaked after disabling social functionality
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(firefox17+ verified, firefox18+ verified)
VERIFIED
FIXED
Firefox 19
People
(Reporter: Gavin, Assigned: Felipe)
References
Details
(Whiteboard: [MemShrink])
Attachments
(1 file, 2 obsolete files)
844 bytes,
patch
|
Gavin
:
approval-mozilla-aurora+
Gavin
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → felipc
Assignee | ||
Comment 1•12 years ago
|
||
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
Attachment #677247 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•12 years ago
|
Attachment #677247 -
Flags: review?(mhammond)
Comment 2•12 years ago
|
||
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.
Attachment #677247 -
Flags: review?(mhammond) → review+
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #677250 -
Flags: review+
Reporter | ||
Comment 5•12 years ago
|
||
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.
Attachment #677247 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #677247 -
Attachment is obsolete: true
Attachment #677250 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/668d22da982b
tracking-firefox17:
--- → ?
tracking-firefox18:
--- → ?
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Comment 8•12 years ago
|
||
Comment on attachment 677251 [details] [diff] [review] Set profile to undefined [Triage Comment] very simple "leak" fix, let's get this on aurora/beta
Attachment #677251 -
Flags: approval-mozilla-beta+
Attachment #677251 -
Flags: approval-mozilla-aurora+
Reporter | ||
Comment 9•12 years ago
|
||
Hmm, not really related to this bug, but we should probably also be clearing ambientNotificationIcons, right?
Comment 10•12 years ago
|
||
(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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/668d22da982b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/6534c274eef5 https://hg.mozilla.org/releases/mozilla-beta/rev/47a455864ed7
status-firefox17:
--- → fixed
status-firefox18:
--- → fixed
Comment 13•12 years ago
|
||
Verified fixed using steps in comment 0.
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•