376802 - One Tooltip Listener to rule them all

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

Currently we create a tooltip listener for every XUL tree and every XUL element with either the tooltip or tooltiptext attribute. However the listener's source node is always the same as the event's current target and the listener only has to store state for the currently hovered element.

Comment 1

[[e-mail]]

Attachment: Proposed patch

The only bit I'm not sure about is the change to nsLayoutStatics.cpp

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I wonder if someone tries to get the review faster using Tolkien
quotations ;) That is ofc always the easy way get my attention.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 260910 [details] [diff] [review]
Proposed patch


>+++ layout/xul/base/src/nsRootBoxFrame.cpp	7 Apr 2007 12:34:01 -0000
>@@ -317,10 +317,8 @@
> {
>   nsXULTooltipListener* listener =
>     NS_STATIC_CAST(nsXULTooltipListener*, aPropertyValue);
>-  if (listener) {
>+  if (listener)
>     listener->RemoveTooltipSupport(NS_STATIC_CAST(nsIContent*, aObject));
>-    NS_RELEASE(listener);
>-  }
> }

Are you sure that this is always called before ReleaseInstance()?
Would it be safer to keep NS_RELEASE here and NS_ADDREF when adding tooltip.

> #ifdef MOZ_XUL
>   // titletips should just use the default tooltip
>-  if (mIsSourceTree && mNeedTitletip) {
>+  if (mNeedTitletip) {

Could you explain this change?

Comment 4

[[e-mail]]

(In reply to comment #3)
>Are you sure that this is always called before ReleaseInstance()?
>Would it be safer to keep NS_RELEASE here and NS_ADDREF when adding tooltip.
Actually I was wondering why you added it - I thought AddTooltipSupport addrefed the listener anyway.

>>-  if (mIsSourceTree && mNeedTitletip) {
>>+  if (mNeedTitletip) {
>Could you explain this change?
Sure. mNeedTitletip can only be set if the source is a tree (but even then it is cleared by a tooltip or tooltiptext attribute).

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Ok, http://lxr.mozilla.org/seamonkey/source/content/base/src/nsNodeUtils.cpp#202
::LastRelease deletes properties before releasing event listeners.

Comment 6

[[e-mail]]

Attachment: Revised patch

Well that was weird. I had to back out attachment 207282 [details] [diff] [review] to fix the regression that the proposed patch reintroduces...

I reverted that condition that Smaug asked about amongst other things to try to fix the above regression and I left it so as not to fiddle too much.

I also removed the tooltiplistener property, it was only there to avoid adding multiple tooltip listeners to the same element, but since there is only one...

I felt like making the callbacks use the global instance, so I did.

I also removed the static nsRefPtr, it bit me ;-)

Comment 7

[[e-mail]]

(In reply to comment #6)
>Well that was weird. I had to back out attachment 207282 [details] [diff] [review]
OK, so I overlooked the fact that the tooltip listener checks the mouse moves between events, and ignores all events that don't change the mouse position.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 261046 [details] [diff] [review]
Revised patch

Make the XULTooltipListener constructor private if possible.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 261046 [details] [diff] [review]
Revised patch

sr=jst

Comment 10

[[e-mail]]

Fix checked in.

Comment 11

[Peter Van der Beken [:peterv]]

nsLayoutStatics::Shutdown is only called after all windows and documents are released. nsXULTooltipListener holds nodes, which can easily keep windows and documents alive. This change seems to make it very easy for bad shutdown leaks to happen.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

nsXULTooltipListener could use nsWeakPtrs to nodes or
maybe even boxObjects.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Though, I guess shutdown leaks were possible even without the patch.
Peter, do you have a testcase?

Comment 14

[[e-mail]]

(In reply to comment #11)
>nsXULTooltipListener holds nodes
It's not supposed to. At least, I tried to make sure that it cleared out mSourceNode. Feel free to file bugs on me for any demonstratable "leaks".

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #14)
> Feel free to file bugs on me for any demonstratable "leaks".

Bug 417534 :)