RESOLVED FIXED 26333
alert during a dragenter event handler will crash the renderer
https://bugs.webkit.org/show_bug.cgi?id=26333
Summary alert during a dragenter event handler will crash the renderer
Victor Wang
Reported 2009-06-11 14:58:11 PDT
The steps to reproduce the problem: 1. Go to the test URL above. 2. Drag the image. 3. Watch the renderer crash. The crux of the page is this: function doOnLoad() { var e = document.documentElement; var f = function() { alert('You is pwned.'); }; if (e.addEventListener) { e.addEventListener('dragenter', f, false); } else { e.attachEvent('ondragenter', f); } }
Attachments
Porpsed Fix for Bug 26333 (4.11 KB, patch)
2009-06-11 16:55 PDT, Victor Wang
eric: review-
Patch updated per comments (4.23 KB, patch)
2009-06-12 10:56 PDT, Victor Wang
eric: review+
Victor Wang
Comment 1 2009-06-11 16:55:42 PDT
Created attachment 31178 [details] Porpsed Fix for Bug 26333
Eric Seidel (no email)
Comment 2 2009-06-12 02:53:43 PDT
Comment on attachment 31178 [details] Porpsed Fix for Bug 26333 You should set CHANGELOG_NAME in your environment. or REAL_NAME. prepare-ChangeLog isn't smart enough on windows to be able to get your real name automatically, hence: 1 2009-06-11 victorw <victorw@chromium.org> You should mention why this needs to be a manual test (because DRT doesn't show alerts) Officially our style guidelines don't use { } on single line ifs: + if (window.layoutTestController) { + layoutTestController.dumpAsText(); + } I'm not sure that our style guide is clear about that for JavaScript, so it might not matter. WK style is also to use more descriptive variable names: + var e = document.documentElement; + var f = function() { + alert('Click OK button.'); + }; at least in code. ;) Probably should say WebKit: + <p>Do the following and see if Chromium crashes.</p> Because that will crash Safari too, no? Otherwise looks fine. r- for the nits above.
Eric Seidel (no email)
Comment 3 2009-06-12 02:54:47 PDT
Comment on attachment 31178 [details] Porpsed Fix for Bug 26333 Your SVN is also not configured correctly: Index: WebCore/manual-tests/resources/drag-image.png =================================================================== Cannot display: file marked as a binary type. svn:mime-type = application/octet-stream Property changes on: WebCore/manual-tests/resources/drag-image.png ___________________________________________________________________ Added: svn:executable + * Added: svn:mime-type + application/octet-stream It isn't correctly detecting the png type. Hence PrettyPatch isn't displaying the image inline in the bug. :)
Victor Wang
Comment 4 2009-06-12 10:56:51 PDT
Created attachment 31202 [details] Patch updated per comments
Eric Seidel (no email)
Comment 5 2009-06-12 12:12:03 PDT
Comment on attachment 31202 [details] Patch updated per comments Added: Added: svn:mine-type + image/png by accident it seems. :) I'm not sure what is confusing PrettyPatch. Windows SVN adds executable properties which it shouldn't. This looks fine. In the future you need to remember to mark your patches for review if you want them reviewed though. :) http://webkit.org/coding/contributing.html
Victor Wang
Comment 6 2009-06-12 12:17:18 PDT
Sure. Was trying to figure out the png display issue before I send out request to review the new patch. All comments addressed. I don't have commit access, could you land my patch if it looks good to you? Thanks! (In reply to comment #5) > (From update of attachment 31202 [details] [review]) > Added: > Added: svn:mine-type > + image/png > > by accident it seems. :) > > I'm not sure what is confusing PrettyPatch. > > Windows SVN adds executable properties which it shouldn't. > > This looks fine. In the future you need to remember to mark your patches for > review if you want them reviewed though. :) > http://webkit.org/coding/contributing.html >
Adam Barth
Comment 7 2009-06-13 11:11:28 PDT
I'll try to land this. Hopefully the PNG part will work.
Adam Barth
Comment 8 2009-06-13 14:35:38 PDT
Sending WebCore/ChangeLog Adding WebCore/manual-tests/drag-enter-alert.html Adding (bin) WebCore/manual-tests/resources/drag-image.png Sending WebCore/page/DragController.cpp Transmitting file data .... Committed revision 44659.
Note You need to log in before you can comment on or make changes to this bug.