383837 - Crash [@ nsXULTooltipListener::LaunchTooltip] when deleting bookmark item in Bookmarks Manager

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

To reproduce the crash:
- Open Bookmarks Manager.
- Select a bookmark to delete.
- Make the title column small, so the title of the bookmark is overflowing (the tooltip will then appear when hovering over that treeitem).
- Position the mouse over the bookmark and then press the "Del" key to delete.

Result: crash
It might be a bit difficult to reproduce the crash, because it depends on the tooltiplistener timer, I guess.

Talkback ID: TB32972650Z
nsXULTooltipListener::LaunchTooltip  [mozilla/layout/xul/base/src/nsxultooltiplistener.cpp, line 513]
nsXULTooltipListener::ShowTooltip  [mozilla/layout/xul/base/src/nsxultooltiplistener.cpp, line 411]
PR_Lock  [mozilla/nsprpub/pr/src/threads/combined/prulock.c, line 240]
0x016a5050

Before the crash, I get an alert with this cryptic message (Places related?):
ASSERT: null node
Stack trace:
0:PU_NodeIsSeparator(undefined)
1:PTV_getCellText(2, [object TreeColumn]

It doesn't crash in branch builds, so it's a regression, I can look for a regression range, if wanted.

Comment 1

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

Haven't yet able to reproduce.
Any chance to a better stack trace?

Should the "del" be pressed when tooltip is visible or just before that?
(I guess latter)

Comment 2

[Martijn Wargers (dead)]

Just before that.

Comment 3

[Martijn Wargers (dead)]

Ok, one additional note to get the crash:
- You need to select the last bookmark you have in the bookmarks manager and delete that one, before the tooltip is visible to see the crash.

Comment 4

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

I can reproduce this now.

Comment 5

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

Attachment: fixes crash, contains also other clean-ups

- The null check is needed to fix the crash (for example mutation 
  events may cause mCurrentTooltip to become null).
- kNameSpaceID_None, not nsnull
- I don't have a testcase for this, but because SetAttr calls creates 
  mutation events, I think we really want to ensure that the listener
  stays alive, so that's why the changes in sTooltipCallback and 
  sAutoHideCallback. (There used to be a kungfuDeathGrip there).

Comment 6

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

The strange alert popup is coming from places, and I have no idea about
that. Different bug anyway.

Comment 7

[Martijn Wargers (dead)]

(In reply to comment #6)
> The strange alert popup is coming from places, and I have no idea about
> that. Different bug anyway.

Ok, I filed bug 384038 for that.

Comment 8

[[e-mail]]

(In reply to comment #5)
>The null check is needed to fix the crash (for example mutation 
>events may cause mCurrentTooltip to become null).
Just out of interest, what's clearing mCurrentTooltip in this case?

Comment 9

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

Attachment: testcase, should be run locally

Easier to just write a testcase than debugging places :p
This shows one way to crash. After allowing privileges you may have to
reload the testcase. Then just hover over the tree cell.

Comment 10

[[e-mail]]

Attachment: Alternative approach

This still allows the tooltip to appear ;-)

Comment 11

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

Comment on attachment 268226 [details] [diff] [review]
Alternative approach

What keeps aTooltip alive after first SetAttr?

Comment 12

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

And I didn't see any problems with my patch.
The tooltip shouldn't be shown if hidePopup is called somehow (in the 
testcase mouseevent cause hiding).

Comment 13

[[e-mail]]

(In reply to comment #11)
>What keeps aTooltip alive after first SetAttr?
nsCOMPtr<nsIDOMXULElement> xulTooltipEl;

Comment 14

[[e-mail]]

Comment on attachment 268072 [details]
testcase, should be run locally

<tooltip default="true"/>
That's mean. We ought to enforce using the anonymous tooltip here.

Comment 15

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

(In reply to comment #14)
> (From update of attachment 268072 [details])
> <tooltip default="true"/>
> That's mean. We ought to enforce using the anonymous tooltip here.
> 

Why? What if someone wants to style the default tooltip in some particular way.

Comment 16

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

Attachment: added still one null check

Comment 17

[[e-mail]]

So, given a comment I fortuitously noticed on IRC, are we actually crashing in Places code because the row we used to be hovering no longer exists?

Comment 18

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

Attachment: change SetTitletipLabel a bit

That seems like the case here yes. GetCellText throws, and that happens 
for example when _ensureValidRow throws an exception 
http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/treeView.js#1005

But in any case, also the testcase in this bug must be handled without 
a crash.

Comment 19

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

Comment on attachment 268477 [details] [diff] [review]
change SetTitletipLabel a bit

Neil D. could you review this.
The null checks are needed because of mutation events and also because GetCellText may do something "bad", for example call hidePopup or open a new window.

Comment 20

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070616 Minefield/3.0a6pre
I'm still seeing the Places assert, but as mentioned in comment 7, that has been filed as bug 384038.