Bug 131770 – Dropzone effects don't work in non-file documents

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

[Alexey Proskuryakov]

We have dropzone implemented (with prefix), and fast/events/dropzone-004.html verifies that setting effects works. But the test passes almost by accident - the code only works in local files, because it uses DataTransfer.files, and only local documents can read that while dragging.

[Alexey Proskuryakov]

Created attachment 230110 [details]
proposed fix

[Darin Adler]

Comment on attachment 230110 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=230110&action=review

r=me as is, although I have a few suggestions for future consideration

> Source/WebCore/ChangeLog:10
> +        2. On Mac, sandbox doesn't prevent File object creation, as we alraedy have the access.

typo: alraedy

> Source/WebCore/dom/DataTransfer.cpp:187
> +bool DataTransfer::hasFileOfType(const String& type)

Sure would be nice if this took a StringView instead of a const String&.

> Source/WebCore/dom/DataTransfer.cpp:189
> +    ASSERT_WITH_SECURITY_IMPLICATION(canReadTypes());

Why not just return false in this case instead of asserting? Are callers really in a good position to alway preflight correctly?

> Source/WebCore/dom/DataTransfer.cpp:199
> +bool DataTransfer::hasStringOfType(const String& type)

Sure would be nice if this took a StringView instead of a const String&.

> Source/WebCore/dom/DataTransfer.cpp:201
> +    ASSERT_WITH_SECURITY_IMPLICATION(canReadTypes());

Same question.

> Source/WebCore/page/EventHandler.cpp:2068
> +        return dataTransfer.hasFileOfType(keyword.substring(5));

If hasFileOfType took a StringView, then this would not need to allocate memory. Could just do StringView(keyword).substring(5) or have this function itself take a StringView.

> Source/WebCore/page/EventHandler.cpp:2071
> +        return dataTransfer.hasStringOfType(keyword.substring(7));

Ditto.

[Alexey Proskuryakov]

>> Source/WebCore/dom/DataTransfer.cpp:189
>> +    ASSERT_WITH_SECURITY_IMPLICATION(canReadTypes());
>
>Why not just return false in this case instead of asserting? Are callers really in a good position to alway preflight correctly?

Yes, I think that they should be in good position. canReadTypes() is not quite the right check anyway, because it's literally "can get dataTransfer.types from JavaScript". It doesn't matter for dropzone, because it only calls these functions when dataTransfer.types can be read. What I wanted to prevent here was potential future callers exposing data to the web without consideration - but if the assertion proves too restrictive, it can be improved when the need arises.

There is quite a bit of fragility in how this class is used to expose data to JS code, and is also used in native code (the confusion that caused this bug in the first place).

[Alexey Proskuryakov]

It would be nice to teach equalIgnoringCase about StringViews I guess - without that, switching to StringView was somewhat too annoying here. 

Committed <http://trac.webkit.org/r167784>.

[Darin Adler]

(In reply to comment #4)
> It would be nice to teach equalIgnoringCase about StringViews

OK. I’ll do that when I get a chance.