If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

()

Core
XUL
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: smaug)

Tracking

({crash, regression})

Trunk
x86
Windows XP
crash, regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

10 years ago
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)
(Reporter)

Comment 2

10 years ago
Just before that.
(Reporter)

Comment 3

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

Comment 4

10 years ago
I can reproduce this now.
(Assignee)

Updated

10 years ago
Assignee: jag → Olli.Pettay
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 years ago
Created attachment 267987 [details] [diff] [review]
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).
Attachment #267987 - Flags: superreview?(neil)
Attachment #267987 - Flags: review?(neil)
(Assignee)

Comment 6

10 years ago
The strange alert popup is coming from places, and I have no idea about
that. Different bug anyway.
(Reporter)

Comment 7

10 years ago
(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

10 years ago
(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?
(Assignee)

Comment 9

10 years ago
Created attachment 268072 [details]
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

10 years ago
Created attachment 268226 [details] [diff] [review]
Alternative approach

This still allows the tooltip to appear ;-)
(Assignee)

Comment 11

10 years ago
Comment on attachment 268226 [details] [diff] [review]
Alternative approach

What keeps aTooltip alive after first SetAttr?
(Assignee)

Comment 12

10 years ago
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

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

Comment 14

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

Comment 15

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

Comment 16

10 years ago
Created attachment 268338 [details] [diff] [review]
added still one null check
Attachment #267987 - Attachment is obsolete: true
Attachment #268338 - Flags: superreview?(neil)
Attachment #268338 - Flags: review?(neil)
Attachment #267987 - Flags: superreview?(neil)
Attachment #267987 - Flags: review?(neil)

Comment 17

10 years ago
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?
(Assignee)

Comment 18

10 years ago
Created attachment 268477 [details] [diff] [review]
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.
Attachment #268338 - Attachment is obsolete: true
Attachment #268477 - Flags: superreview?(neil)
Attachment #268477 - Flags: review?(neil)
Attachment #268338 - Flags: superreview?(neil)
Attachment #268338 - Flags: review?(neil)

Updated

10 years ago
Attachment #268477 - Attachment is patch: true
Attachment #268477 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 19

10 years ago
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.
Attachment #268477 - Flags: review?(neil) → review?(enndeakin)

Updated

10 years ago
Attachment #268477 - Flags: superreview?(neil) → superreview+

Updated

10 years ago
Attachment #268477 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 20

10 years ago
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Crash Signature: [@ nsXULTooltipListener::LaunchTooltip]
You need to log in before you can comment on or make changes to this bug.