Bug 9984

Summary: ASSERTION FAILURE: _private->mouseDownEvent != nil (WebKit/WebView/WebHTMLView.m:4863 -[WebHTMLView(WebInternal) _delegateDragSourceActionMask])
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: sullivan
Priority: P1    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Assertion failure crash log
none
Patch, including change log and a manual test
sullivan: review-
Updated patch sullivan: review+

Description David Kilzer (:ddkilzer) 2006-07-18 04:17:55 PDT
Saw an assertion failure in a locally-built debug build of WebKit r15500 while playing with a test case for Bug 9982 (see Attachment 9543 [details]).  Will try to reproduce.
Comment 1 David Kilzer (:ddkilzer) 2006-07-18 04:19:33 PDT
Created attachment 9546 [details]
Assertion failure crash log
Comment 2 David Kilzer (:ddkilzer) 2006-07-18 04:19:59 PDT
The assertion failure on the console:

=================
ASSERTION FAILED: _private->mouseDownEvent != nil (/Users/ddkilzer/Projects/Cocoa/WebKit/WebKit/WebView/WebHTMLView.m:4863 -[WebHTMLView(WebInternal) _delegateDragSourceActionMask])
=================
Segmentation fault

Comment 3 David Kilzer (:ddkilzer) 2006-07-18 04:28:07 PDT
Steps to reproduce:

1. Open Attachment 9543 [details] (from Bug 9982).
2. Right-click on the word "editable" in the iframe.  This should highlight the word and bring up a contextual menu WITHOUT changing focus to the iframe (note the color of the highlight).
3. Left-click on the word "editable" (which is still highlighted) to close the contextual menu.  Note that the word "editable" is still highlighted, but the iframe still does not have focus.
4. Left-click a second time on the word "editable".

Expected results:

Safari/WebKit should not crash debug builds (assertion error).

Actual results:

Safari/WebKit crashes with an assertion failure in debug builds.

Notes:

Tested with locally-built WebKit r15500 using Safari 2.0.4 (419.3) on Mac OS X 10.4.7 (8J135/PowerPC).

Comment 4 mitz 2006-07-19 09:49:16 PDT
Created attachment 9570 [details]
Patch, including change log and a manual test
Comment 5 John Sullivan 2006-07-24 06:17:22 PDT
Comment on attachment 9570 [details]
Patch, including change log and a manual test

I don't understand how this patch would cause the assert not to fire. Formerly, the assert would only be checked when self was not topHTMLView. But the patch only affects the case when self is topHTMLView. So it seems that for any case in which the assertion was formerly firing, the assertion will still fire. Am I missing something?
Comment 6 mitz 2006-07-24 06:22:52 PDT
Comment on attachment 9570 [details]
Patch, including change log and a manual test

(In reply to comment #5)
> (From update of attachment 9570 [details] [edit])
> I don't understand how this patch would cause the assert not to fire. Formerly,
> the assert would only be checked when self was not topHTMLView. But the patch
> only affects the case when self is topHTMLView. So it seems that for any case
> in which the assertion was formerly firing, the assertion will still fire. Am I
> missing something?

I think you are:

+        [topHTMLView _setMouseDownEvent:_private->mouseDownEvent];

This means that when -_delegateDragSourceActionMask is invoked on the topHTMLView (in the next line), its mouseDownEvent will have been set, and thus the assertion will not fire.

Please reconsider.
Comment 7 Darin Adler 2006-07-24 08:29:16 PDT
Comment on attachment 9570 [details]
Patch, including change log and a manual test

This looks good to me, but I'm concerned that these events are never released. It seems that someone should be calling _setMouseDownEvent:nil somewhere.
Comment 8 mitz 2006-07-24 08:35:35 PDT
(In reply to comment #7)
> (From update of attachment 9570 [details] [edit])
> This looks good to me, but I'm concerned that these events are never released.
> It seems that someone should be calling _setMouseDownEvent:nil somewhere.
> 

It's at most one NSEvent per WebHTMLView. It is released in -[WebHTMLViewPrivate dealloc].
Comment 9 John Sullivan 2006-07-24 09:22:18 PDT
Comment on attachment 9570 [details]
Patch, including change log and a manual test

Mitz is going to look into how mouseDownEvent is used to try to come up with an earlier place to clear it. It's not leaking, but it would be better not to be hanging onto the mouse down event beyond the lifetime of the mouse interaction.
Comment 10 mitz 2006-07-24 11:33:56 PDT
Created attachment 9653 [details]
Updated patch
Comment 11 John Sullivan 2006-07-24 12:07:02 PDT
Comment on attachment 9653 [details]
Updated patch

I asked Mitz some questions on IRC about this, and he convinced me that this is the right approach not only for fixing the assertion but for releasing the event at an appropriately early time. r=me.
Comment 12 Darin Adler 2006-07-24 13:44:13 PDT
Maybe we need to save/restore for reentrancy? Like if the mouse down event has a handler that does showModalDialog?
Comment 13 Darin Adler 2006-07-29 08:13:09 PDT
Committed revision 15688.