Bug 20565 – Drag and drop issues after DOM modifications

• Component: New Bugs
• Status: RESOLVED WORKSFORME

[Berend-Jan Wever]

The repro files for bug 20540 and bug 19516 no longer crash Safari with nightly when they are loaded but if either one of the repro's is drag-and-drop-ed into Safari twice, the second drag-and-drop causes a NULL pointer crash.

(f6c.df0): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
WebKit!WebCore::DragController::concludeDrag+0x3a:
00000000`6d4a0cda 8b03            mov     eax,dword ptr [ebx]
ds:002b:00000000`00000000=????????

[David Kilzer (:ddkilzer)]

<rdar://problem/7075690>

[Aaron Golden]

Created attachment 33498 [details]
Patch to use m_documentUnderMouse instead of element->ownerDocument() when element is NULL.

This patch prevents the crash by avoiding using a NULL result from elementFromPoint.  I'm not sure if this is what we want or if we should, instead, prevent elementFromPoint from ever returning NULL in the first place.

[Oliver Hunt]

Comment on attachment 33498 [details]
Patch to use m_documentUnderMouse instead of element->ownerDocument() when element is NULL.

This is basically a good patch but it needs a testcase, and i think there should be an assertion:
> +    if (element) {
           ASSERT(element->ownerDocument() == m_documentUnderMouse);
> +        innerFrame = element->ownerDocument()->frame();
> +    } else
> +        innerFrame = m_documentUnderMouse->frame();
> +    
>      ASSERT(innerFrame);
>  
>      if (dragData->containsColor()) {

[Aaron Golden]

I can add the test case, but I'm confused by the assertion.

If element->ownerDocument() == m_documentUnderMouse then it seems like we should just use m_documentUnderMouse and not even bother getting the element.  Is that right?

[Oliver Hunt]

(In reply to comment #4)
> I can add the test case, but I'm confused by the assertion.
> 
> If element->ownerDocument() == m_documentUnderMouse then it seems like we
> should just use m_documentUnderMouse and not even bother getting the element. 
> Is that right?

Actually i think that assertion would be incorrect, but likewise i think m_documentUnderMouse would be wrong, take the following:

1. I start to drag content over an element
2. page load occurs, resulting in a new document
3. i drop

now the issue will be the m_documentUnderMouse will be bogus, and i think that's actually the underlying problem.

[Berend-Jan Wever]

Seems to have been fixed at some point for it no longer reproduces.