Bug 9984 – ASSERTION FAILURE: _private->mouseDownEvent != nil (WebKit/WebView/WebHTMLView.m:4863 -[WebHTMLView(WebInternal) _delegateDragSourceActionMask])

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 9546 [details]
Assertion failure crash log

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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).

Created attachment 9570 [details]
Patch, including change log and a manual test

[John Sullivan]

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 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.

[Darin Adler]

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.

(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].

[John Sullivan]

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.

Created attachment 9653 [details]
Updated patch

[John Sullivan]

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.

[Darin Adler]

Maybe we need to save/restore for reentrancy? Like if the mouse down event has a handler that does showModalDialog?

[Darin Adler]

Committed revision 15688.