Bug 30266 – REGRESSION(r47852): Crash on drag and drop

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Following up on a comment by [e-mail] 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>.

[Daniel Bates]

Created attachment 40974 [details]
Crash dump

Crash dump with respect to r47852.

[Daniel Bates]

Created attachment 40977 [details]
LayoutTest case

A DRT test case. Passes under revisions < 47852. Also works in Firefox 3.5.3.

[Daniel Bates]

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.

[Daniel Bates]

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.

[Daniel Bates]

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.

Test case no longer crashes recent WebKit nightlies.
Tested on r49550, Mac OS X 10.6.

[Daniel Bates]

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.

[Daniel Bates]

Comment on attachment 41326 [details]
Layout test

Clearing flags on attachment: 41326

Committed r49701: <http://trac.webkit.org/changeset/49701>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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

[Daniel Bates]

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?

[Alexey Proskuryakov]

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

[Daniel Bates]

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.

[Brian Weinstein]

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?

[Daniel Bates]

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?

[Daniel Bates]

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.

[Eric Seidel (no email)]

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.

[Daniel Bates]

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.

[Daniel Bates]

Committed r49811: <http://trac.webkit.org/changeset/49811>

[noel gordon]

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.

[Daniel Bates]

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?

[noel gordon]

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.

[Daniel Bates]

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 :)
...

[Daniel Bates]

Created attachment 42296 [details]
Layout test text/plain fix

Works in Chrome 4.0.223.11 on Mac.

[Daniel Bates]

Created attachment 42297 [details]
Layout test text/plain fix

Minor change in wording for fail message.

[Daniel Bates]

Re-opening this bug so that we can make this layout test work in Chrome.

[Daniel Bates]

Comment on attachment 42297 [details]
Layout test text/plain fix

Marked for review.

[noel gordon]

Thanks Daniel.  Works on Chrome 3.0.195.27 Vista.  Eric may have some final comments, and give you an r+.

[Eric Seidel (no email)]

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?

[Daniel Bates]

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?

[noel gordon]

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.

[Eric Seidel (no email)]

*** Bug 31004 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Comment on attachment 42297 [details]
Layout test text/plain fix

I think this is an OK fix.

[Daniel Bates]

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.

[Daniel Bates]

Comment on attachment 42297 [details]
Layout test text/plain fix

Clearing flags on attachment: 42297

Committed r50532: <http://trac.webkit.org/changeset/50532>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.