Bug 178301 – On ToT, event.dataTransfer.getData("text/uri-list") returns an empty string when dragging an image

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

After r222656, we consider images on the pasteboard to be files. This is causing DataTransfer.getData to return the empty string for all types, bringing back https://bugs.webkit.org/show_bug.cgi?id=170637.

[Wenson Hsieh]

It looks like the test added in r221343 (editing/pasteboard/drag-drop-href-as-text-data.html) didn't start failing after r222656 because the image being dragged hasn't been decoded yet, so image data isn't written to the pasteboard, which means that the change in r222656 to consider images as files doesn't affect this test.

We'll also need to fix this test to correctly verify that HTTP and HTTPS URLs are exposed to the web, and not anything else.

[Radar WebKit Bug Importer]

<rdar://problem/34990050>

[Wenson Hsieh]

Created attachment 323797 [details]
Patch

[Build Bot]

Comment on attachment 323797 [details]
Patch

Attachment 323797 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4854815

New failing tests:
editing/pasteboard/paste-image-does-not-reveal-file-url.html
editing/pasteboard/data-transfer-item-list-add-file-multiple-times.html

[Build Bot]

Created attachment 323799 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323797 [details]
Patch

Attachment 323797 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4854824

New failing tests:
editing/pasteboard/paste-image-does-not-reveal-file-url.html
editing/pasteboard/data-transfer-item-list-add-file-multiple-times.html

[Build Bot]

Created attachment 323800 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 323797 [details]
Patch

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

> LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7
> +link.href = https://www.webkit.org/
> +url: "https://www.webkit.org/"
> +text/uri-list: "https://www.webkit.org/"
> +link.href = file:///foo/bar/baz
> +url: ""
> +text/uri-list: ""

Looks like we're not seeing the image file?

[Build Bot]

Comment on attachment 323797 [details]
Patch

Attachment 323797 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4854833

New failing tests:
editing/pasteboard/paste-image-does-not-reveal-file-url.html
editing/pasteboard/data-transfer-item-list-add-file-multiple-times.html

[Build Bot]

Created attachment 323801 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323797 [details]
Patch

Attachment 323797 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4854840

New failing tests:
editing/pasteboard/data-transfer-item-list-add-file-multiple-times.html

[Build Bot]

Created attachment 323802 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 323797 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323797&action=review
> 
> > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7
> > +link.href = https://www.webkit.org/
> > +url: "https://www.webkit.org/"
> > +text/uri-list: "https://www.webkit.org/"
> > +link.href = file:///foo/bar/baz
> > +url: ""
> > +text/uri-list: ""
> 
> Looks like we're not seeing the image file?

We're still seeing the image file, we're just not exposing any URL string in this case because it's a file URL. That makes me think — I should augment this test to additionally check that DataTransfer.files contains the image file we're dragging.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #13)
> (In reply to Ryosuke Niwa from comment #8)
> > Comment on attachment 323797 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=323797&action=review
> > 
> > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7
> > > +link.href = https://www.webkit.org/
> > > +url: "https://www.webkit.org/"
> > > +text/uri-list: "https://www.webkit.org/"
> > > +link.href = file:///foo/bar/baz
> > > +url: ""
> > > +text/uri-list: ""
> > 
> > Looks like we're not seeing the image file?
> 
> We're still seeing the image file, we're just not exposing any URL string in
> this case because it's a file URL. That makes me think — I should augment
> this test to additionally check that DataTransfer.files contains the image
> file we're dragging.

Actually, this test already tries to do this! I'll need to investigate further...

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #14)
> (In reply to Wenson Hsieh from comment #13)
> > (In reply to Ryosuke Niwa from comment #8)
> > > Comment on attachment 323797 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=323797&action=review
> > > 
> > > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7
> > > > +link.href = https://www.webkit.org/
> > > > +url: "https://www.webkit.org/"
> > > > +text/uri-list: "https://www.webkit.org/"
> > > > +link.href = file:///foo/bar/baz
> > > > +url: ""
> > > > +text/uri-list: ""
> > > 
> > > Looks like we're not seeing the image file?
> > 
> > We're still seeing the image file, we're just not exposing any URL string in
> > this case because it's a file URL. That makes me think — I should augment
> > this test to additionally check that DataTransfer.files contains the image
> > file we're dragging.
> 
> Actually, this test already tries to do this! I'll need to investigate
> further...

It's also worth mentioning this only reproduces in DumpRenderTree; the file is exposed correctly on the DataTransfer when manually testing drag-drop-href-as-url.html in Minibrowser (both WK1 and WK2) or Safari.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #15)
> (In reply to Wenson Hsieh from comment #14)
> > (In reply to Wenson Hsieh from comment #13)
> > > (In reply to Ryosuke Niwa from comment #8)
> > > > Comment on attachment 323797 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=323797&action=review
> > > > 
> > > > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7
> > > > > +link.href = https://www.webkit.org/
> > > > > +url: "https://www.webkit.org/"
> > > > > +text/uri-list: "https://www.webkit.org/"
> > > > > +link.href = file:///foo/bar/baz
> > > > > +url: ""
> > > > > +text/uri-list: ""
> > > > 
> > > > Looks like we're not seeing the image file?
> > > 
> > > We're still seeing the image file, we're just not exposing any URL string in
> > > this case because it's a file URL. That makes me think — I should augment
> > > this test to additionally check that DataTransfer.files contains the image
> > > file we're dragging.
> > 
> > Actually, this test already tries to do this! I'll need to investigate
> > further...
> 
> It's also worth mentioning this only reproduces in DumpRenderTree; the file
> is exposed correctly on the DataTransfer when manually testing
> drag-drop-href-as-url.html in Minibrowser (both WK1 and WK2) or Safari.

Ok, after some investigation:

We're failing to expose the dragged image as a file in the layout test because we bail early due to one of the (old changeCount) != (new changeCount) security checks in PasteboardCocoa.mm. This is because we call -setData:forType: on drop in -[WebHTMLView pasteboard:provideDataForType:], which is a part of the codepath for supporting promises when dragging image elements. Calling -setData:forType: on drop here bumps the _changeCount of DumpRenderTree's LocalPasteboard in the middle of handling the drop, making us unable to retrieve any further data. This doesn't affect most tests, because we don't usually fall down this promised image data codepath in the other drag and drop paths.

However, according to <https://developer.apple.com/documentation/appkit/nspasteboard/1533544-changecount>, NSPasteboard's changeCount only increments when the pasteboard's owner changes, not whenever pasteboard data is changed (e.g. via -setData:forType:). This is consistent with what happens in Safari/MiniBrowser when dragging an image element normally, which is why the test case works when testing manually.

The fix here is to tweak DRT's LocalPasteboard to behave like NSPasteboard, and only increment the changeCount when the owner changes. After making this change, the layout test runs as expected (with files).

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #16)
> (In reply to Wenson Hsieh from comment #15)
> > (In reply to Wenson Hsieh from comment #14)
> > > (In reply to Wenson Hsieh from comment #13)
> > > > (In reply to Ryosuke Niwa from comment #8)
> > > > > Comment on attachment 323797 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=323797&action=review
> > > > > 
> > > > > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7
> > > > > > +link.href = https://www.webkit.org/
> > > > > > +url: "https://www.webkit.org/"
> > > > > > +text/uri-list: "https://www.webkit.org/"
> > > > > > +link.href = file:///foo/bar/baz
> > > > > > +url: ""
> > > > > > +text/uri-list: ""
> > > > > 
> > > > > Looks like we're not seeing the image file?
> > > > 
> > > > We're still seeing the image file, we're just not exposing any URL string in
> > > > this case because it's a file URL. That makes me think — I should augment
> > > > this test to additionally check that DataTransfer.files contains the image
> > > > file we're dragging.
> > > 
> > > Actually, this test already tries to do this! I'll need to investigate
> > > further...
> > 
> > It's also worth mentioning this only reproduces in DumpRenderTree; the file
> > is exposed correctly on the DataTransfer when manually testing
> > drag-drop-href-as-url.html in Minibrowser (both WK1 and WK2) or Safari.
> 
> Ok, after some investigation:
> 
> We're failing to expose the dragged image as a file in the layout test
> because we bail early due to one of the (old changeCount) != (new
> changeCount) security checks in PasteboardCocoa.mm. This is because we call
> -setData:forType: on drop in -[WebHTMLView pasteboard:provideDataForType:],
> which is a part of the codepath for supporting promises when dragging image
> elements. Calling -setData:forType: on drop here bumps the _changeCount of
> DumpRenderTree's LocalPasteboard in the middle of handling the drop, making
> us unable to retrieve any further data. This doesn't affect most tests,
> because we don't usually fall down this promised image data codepath in the
> other drag and drop paths.
> 
> However, according to
> <https://developer.apple.com/documentation/appkit/nspasteboard/1533544-
> changecount>, NSPasteboard's changeCount only increments when the
> pasteboard's owner changes, not whenever pasteboard data is changed (e.g.
> via -setData:forType:). This is consistent with what happens in
> Safari/MiniBrowser when dragging an image element normally, which is why the
> test case works when testing manually.
> 
> The fix here is to tweak DRT's LocalPasteboard to behave like NSPasteboard,
> and only increment the changeCount when the owner changes. After making this
> change, the layout test runs as expected (with files).

That makes sense. I've changed the call to provideDataForType in LocalPasteboard's declareTypes to getData at some point but I guess there's another code path in which it happens during setData.

[Darin Adler]

Comment on attachment 323797 [details]
Patch

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

> Source/WebCore/dom/DataTransfer.cpp:151
> +    return url.protocolIs("http") || url.protocolIs("https");

URL provides the function protocolIsInHTTPFamily for this kind of check.

[Wenson Hsieh]

(In reply to Darin Adler from comment #18)
> Comment on attachment 323797 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323797&action=review
> 
> > Source/WebCore/dom/DataTransfer.cpp:151
> > +    return url.protocolIs("http") || url.protocolIs("https");
> 
> URL provides the function protocolIsInHTTPFamily for this kind of check.

Thanks! Changed to use protocolIsInHTTPFamily.

[Wenson Hsieh]

Created attachment 323842 [details]
Patch

[Darin Adler]

Comment on attachment 323842 [details]
Patch

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

> Source/WebCore/platform/Pasteboard.h:192
>      static bool isSafeTypeForDOMToReadAndWrite(const String&);
> +    static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&);

I don’t love the word "DOM" here to mean "what we expose to the web in JavaScript bindings" even though it is the term of art. Partly, just like URL, it just doesn’t flow well in these styles of function names since it’s an acronym. Don’t currently have a better idea.

The older function uses the terminology "is safe", and the newer function use the terminology "can" to mean the same thing.

But despite those thoughts I don’t have new function names to propose at this time.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:128
> +    RetainPtr<id> value = [m_pasteboard valuesForPasteboardType:type inItemSet:[NSIndexSet indexSetWithIndex:0]].firstObject;

If we used the retainPtr function here:

    auto value = retainPtr(xxx);

we might get a more specific type than RetainPtr<id>, which would be nice for passing to pasteboardMayContainFilePaths; the compiler doesn’t currently complain about passing id to a more specific type and maybe it never will, but still I think it might be better.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:137
> +    if ([value isKindOfClass:[NSURL class]])
> +        result = [(NSURL *)value absoluteString];
> +
> +    if ([value isKindOfClass:[NSAttributedString class]])
> +        result = [(NSAttributedString *)value string];
> +
> +    if ([value isKindOfClass:[NSString class]])
> +        result = (NSString *)value;

Maybe chain these with else if?

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:92
> +    if (pasteboardType == String(NSURLPboardType)) {

Above you used the format String { kUTTypeURL }, but here it’s the old style.

> LayoutTests/editing/pasteboard/files-during-page-drags.html:84
> +        testFailed("This test is not interactive, please run using DumpRenderTree");

DumpRenderTree is the old one, WebKItTestRunner is the new one. If we are just mentioning one, than I suggest it be the new one.

[Wenson Hsieh]

Comment on attachment 323842 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/platform/Pasteboard.h:192
>> +    static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&);
> 
> I don’t love the word "DOM" here to mean "what we expose to the web in JavaScript bindings" even though it is the term of art. Partly, just like URL, it just doesn’t flow well in these styles of function names since it’s an acronym. Don’t currently have a better idea.
> 
> The older function uses the terminology "is safe", and the newer function use the terminology "can" to mean the same thing.
> 
> But despite those thoughts I don’t have new function names to propose at this time.

I see...then perhaps "bindings" would be a better term to describe this? We do use this in some other places in these same codepaths (e.g. typesSafeForBindings), so maybe it would be good to rename isSafeTypeForDOMToReadAndWrite and other places where we use "DOM" terminology to reference "bindings" instead.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:128
>> +    RetainPtr<id> value = [m_pasteboard valuesForPasteboardType:type inItemSet:[NSIndexSet indexSetWithIndex:0]].firstObject;
> 
> If we used the retainPtr function here:
> 
>     auto value = retainPtr(xxx);
> 
> we might get a more specific type than RetainPtr<id>, which would be nice for passing to pasteboardMayContainFilePaths; the compiler doesn’t currently complain about passing id to a more specific type and maybe it never will, but still I think it might be better.

Ok — done!

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:137
>> +        result = (NSString *)value;
> 
> Maybe chain these with else if?

Done. I suppose adding the else makes the intent here more explicit, though it will result in no change in behavior, since NSURL, NSAttributedString, and NSString are not subclasses of each other.

>> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:92
>> +    if (pasteboardType == String(NSURLPboardType)) {
> 
> Above you used the format String { kUTTypeURL }, but here it’s the old style.

Good catch! Changed to use { } initializer.

>> LayoutTests/editing/pasteboard/files-during-page-drags.html:84
>> +        testFailed("This test is not interactive, please run using DumpRenderTree");
> 
> DumpRenderTree is the old one, WebKItTestRunner is the new one. If we are just mentioning one, than I suggest it be the new one.

Hm...this drag and drop test (files-during-page-drags.html) only runs in WebKit1 though, using DumpRenderTree, and not WK2, which uses WebKitTestRunner. So the description here would be accurate if it mentions DumpRenderTree instead of WebKitTestRunner.

[Wenson Hsieh]

Created attachment 323876 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 323876 [details]
Patch for EWS

Clearing flags on attachment: 323876

Committed r223340: <https://trac.webkit.org/changeset/223340>

[Darin Adler]

Comment on attachment 323842 [details]
Patch

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

>>> Source/WebCore/platform/Pasteboard.h:192
>>> +    static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&);
>> 
>> I don’t love the word "DOM" here to mean "what we expose to the web in JavaScript bindings" even though it is the term of art. Partly, just like URL, it just doesn’t flow well in these styles of function names since it’s an acronym. Don’t currently have a better idea.
>> 
>> The older function uses the terminology "is safe", and the newer function use the terminology "can" to mean the same thing.
>> 
>> But despite those thoughts I don’t have new function names to propose at this time.
> 
> I see...then perhaps "bindings" would be a better term to describe this? We do use this in some other places in these same codepaths (e.g. typesSafeForBindings), so maybe it would be good to rename isSafeTypeForDOMToReadAndWrite and other places where we use "DOM" terminology to reference "bindings" instead.

Yes, I do like the idea of using the word "bindings" instead of the acronym "DOM" wherever it’s practical to make the replacement.

[Wenson Hsieh]

(In reply to Darin Adler from comment #25)
> Comment on attachment 323842 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323842&action=review
> 
> >>> Source/WebCore/platform/Pasteboard.h:192
> >>> +    static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&);
> >> 
> >> I don’t love the word "DOM" here to mean "what we expose to the web in JavaScript bindings" even though it is the term of art. Partly, just like URL, it just doesn’t flow well in these styles of function names since it’s an acronym. Don’t currently have a better idea.
> >> 
> >> The older function uses the terminology "is safe", and the newer function use the terminology "can" to mean the same thing.
> >> 
> >> But despite those thoughts I don’t have new function names to propose at this time.
> > 
> > I see...then perhaps "bindings" would be a better term to describe this? We do use this in some other places in these same codepaths (e.g. typesSafeForBindings), so maybe it would be good to rename isSafeTypeForDOMToReadAndWrite and other places where we use "DOM" terminology to reference "bindings" instead.
> 
> Yes, I do like the idea of using the word "bindings" instead of the acronym
> "DOM" wherever it’s practical to make the replacement.

Sounds good! I'll do this renaming in a followup.

(An aside: this also reminds me that StaticPasteboard should be renamed to something sensible, now that it has taken on a lot more responsibility than it did upon introduction...)