notification panel documents leaked after disabling social functionality

VERIFIED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: Felipe)

Tracking

Trunk
Firefox 19
Points:
---

Firefox Tracking Flags

(firefox17+ verified, firefox18+ verified)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment, 2 obsolete attachments)

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?
Blocks: 805246
Assignee: nobody → felipc
(Assignee)

Comment 1

5 years ago
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
Attachment #677247 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 677250 [details] [diff] [review]
Set profile to undefined
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+
(Assignee)

Comment 6

5 years ago
Created attachment 677251 [details] [diff] [review]
Set profile to undefined
Attachment #677247 - Attachment is obsolete: true
Attachment #677250 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/668d22da982b
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
tracking-firefox17: ? → +
tracking-firefox18: ? → +
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
(Assignee)

Comment 12

5 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
Verified fixed using steps in comment 0.
Status: RESOLVED → VERIFIED
status-firefox17: fixed → verified
status-firefox18: fixed → verified
You need to log in before you can comment on or make changes to this bug.