Bug 26333 – alert during a dragenter event handler will crash the renderer

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Victor Wang]

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);
  }
}

[Victor Wang]

Created attachment 31178 [details]
Porpsed Fix for Bug 26333

[Eric Seidel (no email)]

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  <[e-mail]>

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

Created attachment 31202 [details]
Patch updated per comments

[Eric Seidel (no email)]

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]

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]

I'll try to land this.  Hopefully the PNG part will work.

[Adam Barth]

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.