Bug 30266 - REGRESSION(r47852): Crash on drag and drop
Summary: REGRESSION(r47852): Crash on drag and drop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Critical
Assignee: Nobody
URL: http://decafbad.com/2009/07/drag-and-...
Keywords: Regression
: 31004 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-10-09 16:30 PDT by Daniel Bates
Modified: 2009-11-04 15:59 PST (History)
6 users (show)

See Also:


Attachments
Crash dump (34.01 KB, text/plain)
2009-10-09 16:32 PDT, Daniel Bates
no flags Details
LayoutTest case (4.62 KB, patch)
2009-10-09 17:56 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Self-contained test case (included in patch "LayoutTest case") (9.47 KB, text/html)
2009-10-09 18:10 PDT, Daniel Bates
no flags Details
Layout test (5.51 KB, patch)
2009-10-16 14:26 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Win Skip List Fix (1.81 KB, patch)
2009-10-19 13:00 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Layout test text/plain fix (1.80 KB, patch)
2009-10-21 17:46 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Layout test text/plain fix (2.29 KB, patch)
2009-11-01 21:10 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Layout test text/plain fix (2.28 KB, patch)
2009-11-01 21:14 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-10-09 16:30:58 PDT
Following up on a comment by webkit@leemcdermott.co.uk on bug #23695, crash confirmed when dragging the box marked "Text" (or "Text/HTML/URI" or "Disallowed") to the drop target on the page:
http://decafbad.com/2009/07/drag-and-drop/api-demos.html#data_transfer


Confirmed that r47852 is at fault, <http://trac.webkit.org/changeset/47852>.
Comment 1 Daniel Bates 2009-10-09 16:32:24 PDT
Created attachment 40974 [details]
Crash dump

Crash dump with respect to r47852.
Comment 2 Daniel Bates 2009-10-09 17:56:55 PDT
Created attachment 40977 [details]
LayoutTest case

A DRT test case. Passes under revisions < 47852. Also works in Firefox 3.5.3.
Comment 3 Daniel Bates 2009-10-09 17:59:36 PDT
I should mention, that the crash occurs when you try to access event.dataTransfer.types on the event object passed to the ondrop event handler routine. See comment "// This line causes the crash." in the LayoutTest case patch.
Comment 4 Daniel Bates 2009-10-09 18:10:12 PDT
Created attachment 40978 [details]
Self-contained test case (included in patch "LayoutTest case")

For convenience, here is a self-contained version of the test case included in the patch "LayoutTest case". Note, opening the test page will not crash Safari, but performing the test (i.e. dropping and red square onto the drop target) WILL CRASH Safari.
Comment 5 Daniel Bates 2009-10-09 20:03:02 PDT
Only effects the Macintosh platform.

Running the test under Windows Safari 4.0.3 (531.9.1), "event.dataTransfer.types" is NULL. Probably best to file another bug report for this.
Comment 6 webkit 2009-10-14 10:14:24 PDT
Test case no longer crashes recent WebKit nightlies.
Tested on r49550, Mac OS X 10.6.
Comment 7 Daniel Bates 2009-10-16 14:26:23 PDT
Created attachment 41326 [details]
Layout test

Although this issue has been resolved as of nightly r49550 we should add a test case so that we can prevent a regression of this issue.
Comment 8 Daniel Bates 2009-10-16 14:34:20 PDT
Comment on attachment 41326 [details]
Layout test

Clearing flags on attachment: 41326

Committed r49701: <http://trac.webkit.org/changeset/49701>
Comment 9 Daniel Bates 2009-10-16 14:34:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Alexey Proskuryakov 2009-10-16 15:05:14 PDT
>        Although this issue has been resolved as of nightly r49550 we should add a
>        test case so that we can prevent a regression of this issue.

I think this was resolved in <http://trac.webkit.org/changeset/49513>, which already includes a test case. Is this new test different in some interesting way?
Comment 11 Daniel Bates 2009-10-16 15:36:12 PDT
There is probably overlap here.

Microsoft differentiates the dataTransfer and clipboardData objects. The former is with respect to event objects and the latter with respect to the window object (see first sentence of "Data Transfer Objects" at http://msdn.microsoft.com/en-us/library/ms537658%28VS.85%29.aspx).

It sounds like WebKit uses the same pipeline for handling this kind of functionality, but I have not checked. Though, it wouldn't surprise me if they did at some point since these objects share a common goal of transferring data.

For your reference, this test case tests the issue with respect to event.dataTransfer. The test case <http://trac.webkit.org/browser/trunk/LayoutTests/editing/pasteboard/crash-accessing-clipboardData-types.html?rev=49513> seems to test with respect to window.clipboardData (but it even looks like window.clipboardData == event.clipboardData == event.dataTransfer - but I haven't confirmed this).

We can remove the test case if you want. What are your thoughts? 

(In reply to comment #10)
> >        Although this issue has been resolved as of nightly r49550 we should add a
> >        test case so that we can prevent a regression of this issue.
> 
> I think this was resolved in <http://trac.webkit.org/changeset/49513>, which
> already includes a test case. Is this new test different in some interesting
> way?
Comment 12 Alexey Proskuryakov 2009-10-16 15:42:04 PDT
> We can remove the test case if you want. What are your thoughts? 

I do not have a strong opinion - if you think that having both tests is better, we should certainly keep both.
Comment 13 Daniel Bates 2009-10-16 21:40:53 PDT
Lets leave it in for now since dataTransfer and clipboardData are for different purposes according to the aforementioned MSDN documentation.

It may turn out to be that currently WebKit uses the same pipeline for both.

(In reply to comment #12)
> > We can remove the test case if you want. What are your thoughts? 
> 
> I do not have a strong opinion - if you think that having both tests is better,
> we should certainly keep both.
Comment 14 Brian Weinstein 2009-10-19 11:16:38 PDT
The test for this inside platform/win is not passing on the bots:

http://build.webkit.org/results/Windows%20Release%20(Tests)/r49789%20(5277)/fast/events/drag-and-drop-dataTransfer-types-nocrash-pretty-diff.html

Re-opening the bug and adding this test to the Windows Skipped List. If this only affects Mac should we make this a Mac only test?
Comment 15 Daniel Bates 2009-10-19 12:32:30 PDT
Oops. Yes, the test should be Mac only, see bug #30527 (also mentioned in comment #5 on this bug)

(In reply to comment #14)
> The test for this inside platform/win is not passing on the bots:
> 
> http://build.webkit.org/results/Windows%20Release%20(Tests)/r49789%20(5277)/fast/events/drag-and-drop-dataTransfer-types-nocrash-pretty-diff.html
> 
> Re-opening the bug and adding this test to the Windows Skipped List. If this
> only affects Mac should we make this a Mac only test?
Comment 16 Daniel Bates 2009-10-19 13:00:07 PDT
Created attachment 41442 [details]
Win Skip List Fix

Added a comment that explains that we need to skip this test under Windows until we resolve bug #30527.

Also, minor grammatical changes.
Comment 17 Eric Seidel (no email) 2009-10-19 13:54:23 PDT
Comment on attachment 41442 [details]
Win Skip List Fix

Would be more useful to link to the bug here:
+# This test has never passed on Windows, reopening its bug

Currently that comment is just confusing, at least the reopening part.
Comment 18 Daniel Bates 2009-10-19 14:15:20 PDT
I will fix this before I land.

(In reply to comment #17)
> (From update of attachment 41442 [details])
> Would be more useful to link to the bug here:
> +# This test has never passed on Windows, reopening its bug
> 
> Currently that comment is just confusing, at least the reopening part.
Comment 19 Daniel Bates 2009-10-19 14:40:33 PDT
Committed r49811: <http://trac.webkit.org/changeset/49811>
Comment 20 noel gordon 2009-10-20 08:12:09 PDT
In the layout test committed r49701, I noticed at line 43 that you call event.dataTransfer.setData() with 'text' as the first argument.

Perhaps you meant to use a mine type 'text/plain', or the special value
for same defined in HTML5 -- 'Text', for the data type?

Some embedders are picky about the case-sensitivity of the type or will
only accept the HTML5 defined special values, or mime types.  Your test
wont pass on Chrome for example.
Comment 21 Daniel Bates 2009-10-21 17:46:55 PDT
Created attachment 41625 [details]
Layout test text/plain fix

Updated the test according to Noel's comment.

I don't have Chrome/Chromium. Noel, can you see if this patch makes Chrome happy?
Comment 22 noel gordon 2009-10-22 01:02:40 PDT
Test in Chrome 3 (3.0.195.27) I checked both 'Text' and 'text/plain'.  Either work, but only with a small change to your test :)

When text is being dragged, 'Text' _and_ 'plain/text' are both present in the
event.dataTransfer.types array, in some implementation-defined order.

Line 88 of your test reads:

  shouldBeEqualToString('event.dataTransfer.types[0]', FORMAT_TYPE);

Here types[0] is assumed -- that can fail in other implementations.  So maybe
something like:

  if (event.dataTransfer.types.indexOf(FORMAT_TYPE) == -1)
    // Test FAILED ...
  else
    // Test PASSED ...

would be better.  I'll leave you to fill in the test logging details, and re-
run the tests.
Comment 23 Daniel Bates 2009-10-26 02:01:59 PDT
Thanks Noel.

Just want to let people know, I have not forgotten about this issue. I hope to have an updated layout test (that works in Chrome) by the end the week.

(In reply to comment #22)
> Test in Chrome 3 (3.0.195.27) I checked both 'Text' and 'text/plain'.  Either
> work, but only with a small change to your test :)
...
Comment 24 Daniel Bates 2009-11-01 21:10:31 PST
Created attachment 42296 [details]
Layout test text/plain fix

Works in Chrome 4.0.223.11 on Mac.
Comment 25 Daniel Bates 2009-11-01 21:14:13 PST
Created attachment 42297 [details]
Layout test text/plain fix

Minor change in wording for fail message.
Comment 26 Daniel Bates 2009-11-01 21:49:58 PST
Re-opening this bug so that we can make this layout test work in Chrome.
Comment 27 Daniel Bates 2009-11-01 21:50:38 PST
Comment on attachment 42297 [details]
Layout test text/plain fix

Marked for review.
Comment 28 noel gordon 2009-11-01 22:53:38 PST
Thanks Daniel.  Works on Chrome 3.0.195.27 Vista.  Eric may have some final comments, and give you an r+.
Comment 29 Eric Seidel (no email) 2009-11-01 22:57:16 PST
Comment on attachment 42297 [details]
Layout test text/plain fix

Is it supposed to contain both "text" and "text/plain"?  Is this spec'd somewhere?  I don't think Chrome and Apple WebKit should diverge here, so I'm confused as to why this change would be needed.  Is the types sort-order different for chrome, or are the exposed types different?
Comment 30 Daniel Bates 2009-11-01 23:27:24 PST
Yes, there is a discrepancy between the Safari and Chrome builds for some reason that I am unclear about at the the moment (will look into). I just filed bug #31003 with regards to this issue.

Do you want to hold off on this patch until we fix bug #31003?

(In reply to comment #29)
> (From update of attachment 42297 [details])
> Is it supposed to contain both "text" and "text/plain"?  Is this spec'd
> somewhere?  I don't think Chrome and Apple WebKit should diverge here, so I'm
> confused as to why this change would be needed.  Is the types sort-order
> different for chrome, or are the exposed types different?
Comment 31 noel gordon 2009-11-03 10:09:56 PST
To answer eric's questions:

> Is it supposed to contain both "text" and "text/plain"?  Is this spec'd
> somewhere?  I don't think Chrome and Apple WebKit should diverge here, so
> I'm confused as to why this change would be needed.

Agree, not looking to diverge here.  And for the first two questions;
when text is added to the dataTransfer object, format "text/plain", and
the relevant Section is 7.9.2 The DragEvent and DataTransfer Interfaces of
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#dnd
 
Section 7.9.2 first talks about data "formats" and states that the formats
are case-sensitive and that there are legacy values:

'DataTransfer objects can hold pieces of data, each associated with a unique
 format. Formats are generally given by MIME types, with some values special-
 cased for legacy reasons. For the purposes of this API, however, the format
 strings are opaque, case-sensitive, strings, and the empty string is a valid
 format string.'

The data formats are used only in the Section 7.9.2 dataTransfer methods clearData(format), getData(format), and setData(format, data).  These methods mention the handling of the legacy formats similarly, and setData() is a
good example:

'The setData(format, data) method must add data to the data stored in the
 DataTransfer object, labeled as being of the type format. This must replace
 any previous data that had been set for that format. If format is the value
 "Text", then it must be treated as "text/plain". If the format is "URL",
 then it must be treated as "text/uri-list".'

So for setData("text/plain", "foo"), I'd expect to see "text/plain" in the
dataTransfer.types, and if "Text" must be treated as "text/plain", is it
reasonable to conclude that setData("Text", "foo") must do likewise?

I also see nothing in Section 7.9 Drag and Drop, that restricts the UA from
adding other formats during drag drop initialization, and they typically do.
If I select some text on the page, drag drop it on a target in the page, and enumerate the dataTransfer.types in the drop event, my results are:

Safari 4 Mac OS X
 0:public.utf8-plain-text
 1:dyn.agu8y63n2nuuha5dbrf1ca2pxqry0wkduqf31k3pcr7u1e3basv61a3k
 2:text/plain

Chrome 3.0.195.27 Vista
 0:Text
 1:text/plain

FF 3.5.3, 3.6b1 Vista
 0:text/_moz_htmlcontext
 1:text/_moz_htmlinfo
 2:text/html
 3:text/plain

Hope that answers your last question about sort order etc.  When I write
cross-browser js for HTML5 text drag drop, I look for "text/plain" in the
dataTransfer.types only.  That's the only point of convergence I've found,
and agrees with my understanding of the HTML5 drag drop spec.
Comment 32 Eric Seidel (no email) 2009-11-03 12:15:31 PST
*** Bug 31004 has been marked as a duplicate of this bug. ***
Comment 33 Eric Seidel (no email) 2009-11-03 12:57:51 PST
Comment on attachment 42297 [details]
Layout test text/plain fix

I think this is an OK fix.
Comment 34 Daniel Bates 2009-11-04 15:58:01 PST
Comment on attachment 41442 [details]
Win Skip List Fix

Clearing review flag so I can land the latest patch using bugzilla-tool :-). For reference, Eric r+'ed the WinSkipFix.patch.
Comment 35 Daniel Bates 2009-11-04 15:59:48 PST
Comment on attachment 42297 [details]
Layout test text/plain fix

Clearing flags on attachment: 42297

Committed r50532: <http://trac.webkit.org/changeset/50532>
Comment 36 Daniel Bates 2009-11-04 15:59:55 PST
All reviewed patches have been landed.  Closing bug.