1.22 KB, application/vnd.mozilla.xul+xml
1.28 KB, patch
|Details | Diff | Splinter Review|
3.39 KB, patch
Neil Deakin: review+
|Details | Diff | Splinter Review|
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.
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)
Just before that.
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.
I can reproduce this now.
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).
The strange alert popup is coming from places, and I have no idea about that. Different bug anyway.
(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.
(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?
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.
Created attachment 268226 [details] [diff] [review] Alternative approach This still allows the tooltip to appear ;-)
Comment on attachment 268226 [details] [diff] [review] Alternative approach What keeps aTooltip alive after first SetAttr?
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).
(In reply to comment #11) >What keeps aTooltip alive after first SetAttr? nsCOMPtr<nsIDOMXULElement> xulTooltipEl;
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.
(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.
Created attachment 268338 [details] [diff] [review] added still one null check
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?
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.
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.
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.
10 years ago