Closed Bug 807532 Opened 12 years ago Closed 12 years ago

notification panel documents leaked after disabling social functionality

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(firefox17+ verified, firefox18+ verified)

VERIFIED FIXED
Firefox 19
Tracking Status
firefox17 + verified
firefox18 + verified

People

(Reporter: Gavin, Assigned: Felipe)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 2 obsolete files)

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?
Assignee: nobody → felipc
Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #677247 - Flags: review?(mhammond)
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 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.
Attached patch Set profile to undefined (obsolete) — Splinter Review
Attachment #677250 - Flags: review+
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+
Attachment #677247 - Attachment is obsolete: true
Attachment #677250 - Attachment is obsolete: true
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+
Hmm, not really related to this bug, but we should probably also be clearing ambientNotificationIcons, right?
(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)
https://hg.mozilla.org/mozilla-central/rev/668d22da982b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Verified fixed using steps in comment 0.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: