Last Comment Bug 371200 - Every nsXULTooltipListener listens for pref change to update a global variable.
: Every nsXULTooltipListener listens for pref change to update a global variable.
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4, mlk, perf
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-21 17:55 PST by Johnny Stenback (:jst, jst@mozilla.com)
Modified: 2008-07-31 03:23 PDT (History)
5 users (show)
dveditz: blocking1.8.1.4-
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix, only register our listener once. (2.50 KB, patch)
2007-02-21 18:16 PST, Johnny Stenback (:jst, jst@mozilla.com)
jonas: review+
jonas: superreview+
Details | Diff | Review
Don't pass |this| as closure as it's not needed and not consistent. (1.10 KB, patch)
2007-03-19 18:32 PDT, Johnny Stenback (:jst, jst@mozilla.com)
neil: review+
neil: superreview+
Details | Diff | Review
Above two patches merged together (diff against branch). (2.52 KB, patch)
2007-03-23 14:05 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jst: review+
jst: superreview+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Review

Description Johnny Stenback (:jst, jst@mozilla.com) 2007-02-21 17:55:59 PST
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, jst@mozilla.com) 2007-02-21 18:16:10 PST
Created attachment 255984 [details] [diff] [review]
Fix, only register our listener once.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2007-02-22 14:38:38 PST
Fixed.
Comment 3 neil@parkwaycc.co.uk 2007-02-24 13:59:53 PST
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) 2007-02-28 22:58:12 PST
So an easy fix would be to just pass in |nsnull| instead of |this|, no?
Comment 5 Daniel Veditz [:dveditz] 2007-03-19 11:40:38 PDT
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, jst@mozilla.com) 2007-03-19 18:32:04 PDT
(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, jst@mozilla.com) 2007-03-19 18:32:52 PDT
Created attachment 259054 [details] [diff] [review]
Don't pass |this| as closure as it's not needed and not consistent.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2007-03-19 18:35:56 PDT
Ok, that does actually leak pref code internals (a CallbackNode struct), but not the argument itself.
Comment 9 neil@parkwaycc.co.uk 2007-03-20 02:54:34 PDT
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, jst@mozilla.com) 2007-03-20 14:17:24 PDT
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, jst@mozilla.com) 2007-03-20 14:24:10 PDT
Fix landed on the trunk.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2007-03-23 14:05:26 PDT
Created attachment 259440 [details] [diff] [review]
Above two patches merged together (diff against branch).
Comment 13 Daniel Veditz [:dveditz] 2007-03-27 15:45:14 PDT
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, jst@mozilla.com) 2007-04-05 16:42:34 PDT
Fixed on both branches.

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