314543 - Evil xul testcase, tree:hover{display:inline;}, crashes Mozilla

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase. 
When hovering over the content area, Mozilla crashes.
This seems a recent regression, 2005-10-17 build doesn't crash 2005-10-18 build does crash.
I've also tried a 1.5 branch build of 2005-10-23, it doesn't crash.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-10-17+06%3A00%3A00&maxdate=2005-10-18+13%3A00%3A00&cvsroot=%2Fcvsroot
I've no idea which bug could be responsible

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: backtrace

This is the backtrace from the assertion I get prior to the crash:
###!!! ASSERTION: no scroll bar: 'mScrollbar', file c:/mozilla/mozilla/layout/xu
l/base/src/tree/src/nsTreeBodyFrame.cpp, line 782
Break: at file c:/mozilla/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.c
pp, line 782

After that, the backtrace of the sigsegv follows.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I've no idea which bug could be responsible

Based on the crash stack, bug 307678.  This doesn't crash on Linux.

Comment 4

[Ere Maijala (slow)]

Attachment: Patch v1

Use only weak refs to nsWindow in mouse trailer to avoid all kinds of problems such as re-entry to nsWindow destructor.

Comment 5

[Ere Maijala (slow)]

Comment on attachment 201543 [details] [diff] [review]
Patch v1

Oops. A very flawed patch here...

Comment 6

[Ere Maijala (slow)]

Attachment: Patch v1.1

Second try. Use weak refs but don't use NS_IF_RELEASE to get there to avoid nullifying the pointers.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // We're not going to hold a reference to the window. nsWindow just
+  // needs to do the right thing (nullify this if necessary) in its destructor.
+  if (topWin) {
+    topWin->Release();
+  }
   if (mHoldMouseWindow != topWin && mTimer) {
     // Make sure TimerProc is fired at least once for the old window
     TimerProc(nsnull, nsnull);
   }
-  NS_IF_RELEASE(mHoldMouseWindow);
   mHoldMouseWindow = topWin;

Is it possible for the TimerProc call to cause topWin to be destroyed? If so this would leave a dangling reference in mHoldMouseWindow.

Comment 8

[Ere Maijala (slow)]

Comment on attachment 201546 [details] [diff] [review]
Patch v1.1

I have to assume it's possible (and it is of course). Which leads to a situation I'm not sure how to handle: if we don't hold the ref, we'll have a dangling pointer, if we do, we might hold the last ref to it and cause a horrible loop when releasing it.

Roc, how about just backing out the patch from bug 307678 and going for TrackMouseEvent?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I think that's the way to go on trunk. And we won't do anything about 307678 for branch, OK?

Comment 10

[Ere Maijala (slow)]

Attachment: Backout patch

Yes, unless someone finds a critical problem with that version too. 

This is a patch to backout the patch of bug 307678.

Comment 11

[Ere Maijala (slow)]

Patch checked in. Work will continue in bug 312566.

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2005-11-07-10 of SeaMonkey trunk on Windows XP; no crash.