20565 2008-08-29 01:26:47 -0700 Drag and drop issues after DOM modifications 2010-04-19 11:34:54 -0700 1 1 1 Unclassified WebKit New Bugs 525.x (Safari 3.1) PC Windows Vista RESOLVED WORKSFORME http://skypher.com/SkyLined/Repro/Safari/AVR%5B3c%5D@WebKit.dll+4c00%20%23bd95c6be/repro.html InRadar P1 Critical --- 1 skylined webkit-unassigned aegolden oliver oldest_to_newest 89728 0 skylined 2008-08-29 01:26:47 -0700 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=???????? 133223 1 ddkilzer 2009-07-20 14:12:35 -0700 <rdar://problem/7075690> 134783 2 33498 aegolden 2009-07-25 16:42:16 -0700 Created attachment 33498 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. 135760 3 33498 oliver 2009-07-29 18:04:40 -0700 Comment on attachment 33498 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()) { 135776 4 aegolden 2009-07-29 19:22:45 -0700 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? 180564 5 oliver 2010-01-13 23:13:20 -0800 (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. 214128 6 skylined 2010-04-19 11:34:54 -0700 Seems to have been fixed at some point for it no longer reproduces. 33498 2009-07-25 16:42:16 -0700 2009-07-29 18:04:40 -0700 Patch to use m_documentUnderMouse instead of element->ownerDocument() when element is NULL. patch.txt text/plain 1413 aegolden SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NjQwMSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMDktMDctMjUgIEFhcm9uIEdvbGRlbiAgPGFnb2xkZW5AYXBwbGUu Y29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDU2NQorCisgICAgICAgIElm IHdlIGRvbid0IGdldCBhIHVzYWJsZSByZXN1bHQgZnJvbSBlbGVtZW50RnJvbVBvaW50IHRoZW4K KyAgICAgICAgd2Ugc2hvdWxkIGp1c3QgdXNlIG1fZG9jdW1lbnRVbmRlck1vdXNlIGRpcmVjdGx5 LCBpbnN0ZWFkIG9mCisgICAgICAgIGNyYXNoaW5nLgorCisgICAgICAgICogcGFnZS9EcmFnQ29u dHJvbGxlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpEcmFnQ29udHJvbGxlcjo6Y29uY2x1ZGVF ZGl0RHJhZyk6CisKIDIwMDktMDctMTYgIFNoaW5pY2hpcm8gSGFtYWppICA8aGFtYWppQGNocm9t aXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBPbGl2ZXIgSHVudC4KSW5kZXg6IFdlYkNv cmUvcGFnZS9EcmFnQ29udHJvbGxlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wYWdlL0Ry YWdDb250cm9sbGVyLmNwcAkocmV2aXNpb24gNDY0MDEpCisrKyBXZWJDb3JlL3BhZ2UvRHJhZ0Nv bnRyb2xsZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zNDEsOCArMzQxLDEyIEBACiAKICAgICBJ bnRQb2ludCBwb2ludCA9IG1fZG9jdW1lbnRVbmRlck1vdXNlLT52aWV3KCktPndpbmRvd1RvQ29u dGVudHMoZHJhZ0RhdGEtPmNsaWVudFBvc2l0aW9uKCkpOwogICAgIEVsZW1lbnQqIGVsZW1lbnQg PSAgbV9kb2N1bWVudFVuZGVyTW91c2UtPmVsZW1lbnRGcm9tUG9pbnQocG9pbnQueCgpLCBwb2lu dC55KCkpOwotICAgIEFTU0VSVChlbGVtZW50KTsKLSAgICBGcmFtZSogaW5uZXJGcmFtZSA9IGVs ZW1lbnQtPm93bmVyRG9jdW1lbnQoKS0+ZnJhbWUoKTsKKyAgICBGcmFtZSogaW5uZXJGcmFtZSA9 IE5VTEw7CisgICAgaWYgKGVsZW1lbnQpCisgICAgICAgIGlubmVyRnJhbWUgPSBlbGVtZW50LT5v d25lckRvY3VtZW50KCktPmZyYW1lKCk7CisgICAgZWxzZQorICAgICAgICBpbm5lckZyYW1lID0g bV9kb2N1bWVudFVuZGVyTW91c2UtPmZyYW1lKCk7CisgICAgCiAgICAgQVNTRVJUKGlubmVyRnJh bWUpOwogCiAgICAgaWYgKGRyYWdEYXRhLT5jb250YWluc0NvbG9yKCkpIHsK