371200 - Every nsXULTooltipListener listens for pref change to update a global variable.

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Johnny Stenback (:jst)]

This is just silly, we've got a *lot* of nsXULTooltipListener objects around at any given time, and every single one of them listens to changes to the pref browser.chrome.toolbar_tips only to update a static boolean member. There should only be one observer for that pref, period.

I found this when I was tracking down a severe performance problem when closing a window in a Firefox instance that's been running for a long time (weeks), and it turns out that there's *thousands* of these listeners around, and the slowness came primarily due to these listeners unregistering themselves as pref observers. Obviously there's another problem here as well, the fact that we've got thousands of tooltip listeners, but that's another problem for another bug.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Fix, only register our listener once.

Comment 2

[Johnny Stenback (:jst)]

Fixed.

Comment 3

[[e-mail]]

Comment on attachment 255984 [details] [diff] [review]
Fix, only register our listener once.

>+  if (--sTooltipListenerCount == 0) {
>+    // Unregister our pref observer
>+    nsContentUtils::UnregisterPrefCallback("browser.chrome.toolbar_tips",
>+                                           ToolbarTipsPrefChanged, this);
>+  }
Unfortunately the last destroyed tooltip is unlikely the same to be the first created tooltip, so this leaks.

That there are thousands of tooltip listeners is because every XUL element that is either a tree or has a tooltip or tooltiptext attribute currently has one. However, I'm not sure that's strictly necessary, for instance mSourceNode is always equal to aEvent's currentTarget, and the tooltip listener would simply reset itself to whichever node you last moused over.

Comment 4

[jag (Peter Annema)]

So an easy fix would be to just pass in |nsnull| instead of |this|, no?

Comment 5

[Daniel Veditz [:dveditz]]

This is a footprint or perf win? Not a blocker, but sounds nice to have. But sounds like there are unresolved issues we'd need fixed (comment 3) before approving a branch patch.

Comment 6

[Johnny Stenback (:jst)]

(In reply to comment #3)
> (From update of attachment 255984 [details] [diff] [review])
> >+  if (--sTooltipListenerCount == 0) {
> >+    // Unregister our pref observer
> >+    nsContentUtils::UnregisterPrefCallback("browser.chrome.toolbar_tips",
> >+                                           ToolbarTipsPrefChanged, this);
> >+  }
> Unfortunately the last destroyed tooltip is unlikely the same to be the first
> created tooltip, so this leaks.

Good point, passing this in here makes no sense. I do not however believe this is a leak. The pref service doesn't AFAICT hold a strong reference to the closure passed in to a pref callback. Passing in null in these cases seems like the right thing to do, the callback doesn't need the closure at all in either case... Patch coming up.

Comment 7

[Johnny Stenback (:jst)]

Attachment: Don't pass |this| as closure as it's not needed and not consistent.

Comment 8

[Johnny Stenback (:jst)]

Ok, that does actually leak pref code internals (a CallbackNode struct), but not the argument itself.

Comment 9

[[e-mail]]

Comment on attachment 259054 [details] [diff] [review]
Don't pass |this| as closure as it's not needed and not consistent.

Even if it's a weak pointer it's still morally wrong because it will dangle ;-)

Why is nsContentUtils still using nsIPref (deprecated)?

Comment 10

[Johnny Stenback (:jst)]

Because it's old, and noone has bothered to update it :)(In reply to comment #9)
> (From update of attachment 259054 [details] [diff] [review])
> Even if it's a weak pointer it's still morally wrong because it will dangle ;-)

Agreed :)

> Why is nsContentUtils still using nsIPref (deprecated)?

Because it's old, and nobody has bothered to update it.

Comment 11

[Johnny Stenback (:jst)]

Fix landed on the trunk.

Comment 12

[Johnny Stenback (:jst)]

Attachment: Above two patches merged together (diff against branch).

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 259440 [details] [diff] [review]
Above two patches merged together (diff against branch).

approved for 1.8.0.12/1.8.1.4, a=dveditz for release-drivers

Comment 14

[Johnny Stenback (:jst)]

Fixed on both branches.