Bug 26333 - alert during a dragenter event handler will crash the renderer
Summary: alert during a dragenter event handler will crash the renderer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P2 Normal
Assignee: Adam Barth
URL: http://nigel.tao.googlepages.com/how-...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-11 14:58 PDT by Victor Wang
Modified: 2009-06-13 14:35 PDT (History)
2 users (show)

See Also:


Attachments
Porpsed Fix for Bug 26333 (4.11 KB, patch)
2009-06-11 16:55 PDT, Victor Wang
eric: review-
Details | Formatted Diff | Diff
Patch updated per comments (4.23 KB, patch)
2009-06-12 10:56 PDT, Victor Wang
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Wang 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);
  }
}
Comment 1 Victor Wang 2009-06-11 16:55:42 PDT
Created attachment 31178 [details]
Porpsed Fix for Bug 26333
Comment 2 Eric Seidel (no email) 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.
Comment 3 Eric Seidel (no email) 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. :)
Comment 4 Victor Wang 2009-06-12 10:56:51 PDT
Created attachment 31202 [details]
Patch updated per comments
Comment 5 Eric Seidel (no email) 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
Comment 6 Victor Wang 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
> 
Comment 7 Adam Barth 2009-06-13 11:11:28 PDT
I'll try to land this.  Hopefully the PNG part will work.
Comment 8 Adam Barth 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.