Bug 172526

Summary: Drag event DataTransfer has unexpected types "dyn.ah62d4..."
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: WebCore Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ap, buildbot, commit-queue, darin, enrica, jlewis3, joepeck, megan_gardner, msaboff, pimvdb, rniwa, ryanhaddad, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=67025
https://bugs.webkit.org/show_bug.cgi?id=177633
Attachments:
Description Flags
[TEST] Test Page
none
EWS test pass
none
Rebase (not for review yet)
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Still need to fix the Win/GTK/WPE builds
none
Attempt to fix Win/GTK/WPE builds
none
Fix Win/GTK/WPE builds, round 2
none
Fix Win/GTK/WPE builds, round 3
none
Fix Win/GTK/WPE builds, round 4
none
Fix the GTK build
none
Fix the GTK build (2)
none
Remove commitStaticPasteboardToPasteboard
none
Remove commitStaticPasteboardToPasteboard and rebase
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Remove commitStaticPasteboardToPasteboard (2)
none
Address review feedback
none
Fix iOS 11.0 build
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Address more review feedback.
none
Address more review feedback, and rebase
rniwa: review+
Holding for EWS... none

Description Joseph Pecoraro 2017-05-23 15:22:05 PDT
Created attachment 311062 [details]
[TEST] Test Page

Summary:
Drag event DataTransfer has unexpected types dynamic types.

Steps to reproduce:
1. Open attached test page
2. Drag the test element into the drop zone
  => Lots of unexpected types

Expected:
text/plain: Drag Data 1
text/x-1: x1
text/x-2: x2
text/x-3: x3
text/x-4: x4

Actual:
Apple WebKit dummy pasteboard type: 
dyn.ah62d4rv4gu81k3p2su11upmv: x1
dyn.ah62d4rv4gu81k3p2su11upmw: x2
dyn.ah62d4rv4gu81k3p2su11upmx: x3
dyn.ah62d4rv4gu81k3p2su11upmy: x4
dyn.ah62d4rv4gu8yc6durvwwaz5fqmf0w7baqv4045p3eb2gc65yqzvg82pwquuhk8puqy: 
text/plain: Drag Data 1
text/x-1: x1
text/x-2: x2
text/x-3: x3
text/x-4: x4

Chrome:
text/plain: Drag Data 1
text/x-1: x1
text/x-2: x2
text/x-3: x3
text/x-4: x4

Firefox:
text/plain: Drag Data 1
text/x-1: x1
text/x-2: x2
text/x-3: x3
text/x-4: x4

Notes:
- Reproduced on STP 30 on macOS Sierra 10.12.5
Comment 1 Radar WebKit Bug Importer 2017-05-24 23:07:30 PDT
<rdar://problem/32396081>
Comment 2 Wenson Hsieh 2017-09-23 05:01:52 PDT
Created attachment 321627 [details]
EWS test pass
Comment 3 Wenson Hsieh 2017-09-23 05:15:21 PDT
Created attachment 321628 [details]
Rebase (not for review yet)
Comment 4 Build Bot 2017-09-23 06:23:30 PDT
Comment on attachment 321628 [details]
Rebase (not for review yet)

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

New failing tests:
editing/pasteboard/datatransfer-types-dropping-text-file.html
http/tests/local/fileapi/send-dragged-file.html
http/tests/security/clipboard/clipboard-file-access.html
editing/pasteboard/dataTransfer-setData-getData.html
http/tests/local/fileapi/send-sliced-dragged-file.html
http/tests/local/fileapi/file-last-modified.html
http/tests/local/fileapi/file-last-modified-after-delete.html
Comment 5 Build Bot 2017-09-23 06:23:32 PDT
Created attachment 321630 [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
Comment 6 Build Bot 2017-09-23 06:39:46 PDT
Comment on attachment 321628 [details]
Rebase (not for review yet)

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

New failing tests:
editing/pasteboard/datatransfer-types-dropping-text-file.html
http/tests/local/fileapi/send-dragged-file.html
http/tests/security/clipboard/clipboard-file-access.html
editing/pasteboard/dataTransfer-setData-getData.html
http/tests/local/fileapi/send-sliced-dragged-file.html
http/tests/local/fileapi/file-last-modified.html
http/tests/local/fileapi/file-last-modified-after-delete.html
Comment 7 Build Bot 2017-09-23 06:39:47 PDT
Created attachment 321631 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-09-23 06:49:59 PDT
Comment on attachment 321628 [details]
Rebase (not for review yet)

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

New failing tests:
editing/pasteboard/data-transfer-get-data-on-paste-rich-text.html
Comment 9 Build Bot 2017-09-23 06:50:00 PDT
Created attachment 321632 [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
Comment 10 Wenson Hsieh 2017-09-24 04:10:01 PDT
Created attachment 321651 [details]
Still need to fix the Win/GTK/WPE builds
Comment 11 Wenson Hsieh 2017-09-24 15:13:19 PDT
Created attachment 321657 [details]
Attempt to fix Win/GTK/WPE builds
Comment 12 Wenson Hsieh 2017-09-24 15:55:37 PDT
Created attachment 321658 [details]
Fix Win/GTK/WPE builds, round 2
Comment 13 Darin Adler 2017-09-24 17:02:27 PDT
Comment on attachment 321658 [details]
Fix Win/GTK/WPE builds, round 2

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

> Source/WebCore/dom/DataTransfer.cpp:239
> +    bool isStaticPasteboard = is<StaticPasteboard>(m_pasteboard.get());
> +    ASSERT(isStaticPasteboard);
> +    ASSERT(!is<StaticPasteboard>(finalPasteboard.get()));
> +    if (!isStaticPasteboard)
> +        return;
> +
> +    auto staticPasteboard = WTFMove(m_pasteboard);
> +    downcast<StaticPasteboard>(*staticPasteboard).commitToPasteboard(*finalPasteboard);
> +    m_pasteboard = WTFMove(finalPasteboard);

Not necessarily for this patch.

I am starting to understand what StaticPasteboard is now. I think that the name "static pasteboard" is not quite right; perhaps "internal pasteboard" to distinguish from "platform pasteboard" is the right way to do it. Also seems like the base class Pasteboard would have two derived classes, one called PlatformPasteboard and a concrete one InternalPasteboard. Unfortunately we use the name PlatformPasteboard for a separate object; too many layers I think! The PlatformPasteboard class could be abstract if we use virtual functions for the cross platform pattern, or concrete if we use the non-virtual compile-time technique.

Can we do this with a virtual function instead of using the is and downcast functions? Maybe something like this:

    if (m_pasteboard->exportContentsToPlatformPasteboard(*finalPasteboard))
        m_pasteboard = WTFMove(finalPasteboard);

Or maybe not. I do think that using distinct types would help this code be readable. The argument would be std::unique_ptr<PlatformPasteboard>&& for example.

> Source/WebCore/page/EventHandler.cpp:3696
> +        pasteboard->clear();

Unclear why "clear" is needed. Probably needs a "why" comment.

> Source/WebCore/platform/Pasteboard.cpp:42
> +    static int currentCustomDataSerializationVersion = 1;

constexpr or const

> Source/WebCore/platform/Pasteboard.cpp:53
> +    static int maxSupportedDataSerializationVersionNumber = 1;

constexpr or const

> Source/WebCore/platform/Pasteboard.cpp:56
> +    WTF::Persistence::Decoder decoder((const uint8_t *)buffer->data(), buffer->size());

Use reinterpret_cast instead of C-style cast. No space before "*". Also consider braces instead of () for constructor arguments?

> Source/WebCore/platform/Pasteboard.cpp:59
> +    int version;
> +    if (!decoder.decode(version) || version > maxSupportedDataSerializationVersionNumber)
> +        return { };

Because we use "int" here there could be negative numbers. I suggest we reject them instead of treating them all as supported versions.

> Source/WebCore/platform/Pasteboard.h:160
> +WEBCORE_EXPORT Ref<SharedBuffer> sharedBufferFromCustomData(const HashMap<String, String>& customData, const Vector<String>& types);

Why doesn’t this take const PasteboardCustomData& instead of taking the map and vector separately?

> Source/WebCore/platform/Pasteboard.h:161
> +WEBCORE_EXPORT HashMap<String, String> readCustomDataFromSharedBuffer(const Ref<SharedBuffer>&, Vector<String>& outTypes);

In new code we can use structs to return multiple return values instead of using out arguments. So this should return both the map and the vector, not return the map and fill in a passed-in vector. I would suggest returning PasteboardCustomData but I don’t understand exactly why that has two maps and this function returns only one.

No reason to take const Ref<SharedBuffer>& instead of taking SharedBuffer& or const SharedBuffer&.

> Source/WebCore/platform/Pasteboard.h:164
> +static const char *customWebKitPasteboardDataType = "org.webkit.custom-pasteboard-data";

Type here should be "const char* const xxx" or "const char xxx[]" not "const char *" (note also no space before the "*"). The type in this patch says that the string is constant but there is a non-constant global variable pointing to it. And since we say static, it’s a separate non-constant global variable in each source file that compiles this header.

> Source/WebCore/platform/Pasteboard.h:168
> +WEBCORE_EXPORT String webExposedPasteboardTypeForPlatformType(const String& platformType);

Does this need to use String? I think these are always compile-time constants that can use "const char*"; we never need to convert them into a String. Would be more efficient to leave them that way instead of making a String unless there is something about that which would be unclear.

> Source/WebCore/platform/StaticPasteboard.cpp:63
> +    m_changeCount++;

We normally write pre-decrement, although I suppose post-decrement is OK.

> Source/WebCore/platform/StaticPasteboard.cpp:78
> +    m_changeCount++;

Ditto.

> Source/WebCore/platform/StaticPasteboard.cpp:99
> +    PasteboardCustomData data;
> +    data.webExposedTypes = m_types;
> +    data.publicData = m_webExposedData;
> +    data.privateData = m_customData;
> +    pasteboard.writeCustomData(data);

Unfortunate that this has to copy the vector and the two maps just to pass them to a function that doesn’t need to keep the copy around. Should refactor the code to avoid having to make those copies.

> Source/WebCore/platform/StaticPasteboard.h:83
> +    long m_changeCount;

Looks like this is uninitialized. Should we initialize it to zero?

> Source/WebCore/platform/ios/PasteboardIOS.mm:150
> +        return "text/plain";

Normally we use ASCIILiteral in a case like this.

> Source/WebCore/platform/ios/PasteboardIOS.mm:153
> +        return "text/html";

Ditto.

> Source/WebCore/platform/ios/PasteboardIOS.mm:156
> +        return "text/uri-list";

Ditto.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:448
> +    Vector<String> result;
> +    copyToVector(webExposedTypes, result);
> +    return result;

Clearly we need a version of copyToVector with a return value instead of one that uses an out argument. Maybe we don’t need the one that takes a vector argument at all any more.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:476
> +        if (!data.publicData.contains(type))
> +            continue;
> +
> +        NSString *stringValue = data.publicData.get(type);

This is a double hash lookup. That’s never needed and always twice as slow as the correct idiom. This can be done by relying on the fact that get returns null string when there is no string, or it can be done with find instead of get.

    NSString *stringValue = data.publicData.get(type);
    if (!stringValue)
        continue;

Or:

    auto value = data.publicData.find(type);
    if (value == data.publicData.end())
        continue;

    NSString *stringValue = value->value;

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:512
> +        if (customData.contains(type))
> +            return customData.get(type);

This is a double hash lookup. That’s never needed and always twice as slow as the correct idiom. This can be done by relying on the fact that get returns null string when there is no string, or it can be done with find instead of get.

    auto data = readCustomDataFromSharedBuffer(*buffer, types).get(type);
    if (!data.isNull())
        return data;

Or:

    auto customData = readCustomDataFromSharedBuffer(*buffer, types);
    auto data = customData.find(type);
    if (data != customData.end())
        return data->value;

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:527
> +        value = [[[NSString alloc] initWithData:(NSData *)value encoding:NSUTF8StringEncoding] autorelease];

Unfortunate to use autorelease here. Instead I suggest we consider making "value" be a RetainPtr<id> and using adoptNS here.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:555
> +        if ([registeredTypeIdentifier isEqualToString:@(customWebKitPasteboardDataType)])
> +            [typesToLoad addObject:@(customWebKitPasteboardDataType)];

A real shame that this allocates two NSString objects. There might be an idiom that avoids that.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:584
> +        foundAnyDataToLoad = foundAnyDataToLoad || typeIdentifiersToLoad.count;

Use ||= ?

> Source/WebCore/platform/mac/PasteboardMac.mm:536
> +        return "text/plain";

ASCIILiteral.

> Source/WebCore/platform/mac/PasteboardMac.mm:539
> +        return "text/uri-list";

ASCIILiteral.

> Source/WebCore/platform/mac/PasteboardMac.mm:542
> +        return "text/html";

ASCIILiteral.

> Source/WebCore/platform/mac/PasteboardMac.mm:552
> +        return "Files";

ASCIILiteral.
Comment 14 Wenson Hsieh 2017-09-24 17:03:41 PDT
Created attachment 321661 [details]
Fix Win/GTK/WPE builds, round 3
Comment 15 Ryosuke Niwa 2017-09-24 17:11:48 PDT
Comment on attachment 321658 [details]
Fix Win/GTK/WPE builds, round 2

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

The general approach looks great but we need to add this feature inside a runtime flag for app compatibility.
e.g. third party app that embeds WebKit might be expecting to be able to write app specific UTI into system clipboard.

> Source/WebCore/ChangeLog:15
> +        empty string anyways), but can additionally pose security issues by exposing information meant only for native
> +        applications to unvetted web content.

Letting web content read these types only pose privacy concerns. Letting them write to these types can pose security concerns.

> Source/WebCore/ChangeLog:26
> +        This patch adopts this same approach in WebKit, and introduces the org.webkit.custom-pasteboard-data UTI (refer

We should use com.apple.WebKit.custom-pasteboard-data since that's what we use elsewhere.

> Source/WebCore/ChangeLog:52
> +        Pasteboard, clears out the DataTransfer's StaticPasteboard, and replaces it with the new Pasteboard.

Why do we need to do that? I don't see why we'd like to have that kind of side-effect in commitStaticPasteboardToPasteboard.
If you're doing this for drag & drop, then a better thing to do is to create a new DataTransfer for dragstart event as mandated by the spec.

> Source/WebCore/dom/DataTransfer.cpp:235
> +    ASSERT(isStaticPasteboard);
> +    ASSERT(!is<StaticPasteboard>(finalPasteboard.get()));
> +    if (!isStaticPasteboard)
> +        return;

I would rather turn this into RELEASE_ASSERT than adding an early return.
This is not a perf sensitive code, and it's pretty obvious from your code change that it should never happen in practice.

> Source/WebCore/dom/DataTransfer.cpp:239
> +    auto staticPasteboard = WTFMove(m_pasteboard);
> +    downcast<StaticPasteboard>(*staticPasteboard).commitToPasteboard(*finalPasteboard);
> +    m_pasteboard = WTFMove(finalPasteboard);

I don't think we should do this.

> Source/WebCore/page/EventHandler.cpp:3697
>          m_mouseDownMayStartDrag = dispatchDragStartEvent(hasNonDefaultPasteboardData);
>  
> +        auto pasteboard = Pasteboard::createForDragAndDrop();
> +        pasteboard->clear();
> +        dragState().dataTransfer->commitStaticPasteboardToPasteboard(WTFMove(pasteboard));

Instead of doing this, create a new DataTransfer object with StaticPasteboard, dispatch dragstart with that Pasteboard,
and then commit that to dragState().dataTransfer which continues to use the "real" pasteboard.
That is, flip around the pasteboards you're using.

> Source/WebCore/platform/Pasteboard.cpp:56
> +    WTF::Persistence::Decoder decoder((const uint8_t *)buffer->data(), buffer->size());

Please use static_cast.

> Source/WebCore/platform/Pasteboard.h:164
> +static const char *customWebKitPasteboardDataType = "org.webkit.custom-pasteboard-data";

Use com.apple.WebKit.custom-pasteboard-data.

> Source/WebCore/platform/ios/PasteboardIOS.mm:447
> +    return platformStrategies()->pasteboardStrategy()->webExposedTypes(m_pasteboardName);

Nice. I was gonna add this thing for my image patch.

> Source/WebCore/platform/win/PasteboardWin.cpp:1061
> +long Pasteboard::changeCount() const
> +{
> +    return 0;
> +}
> +

We shouldn't be adding changeCount to other platforms since that's not a thing
in terms of how clipboard works in Windows, GTK+, etc...
Comment 16 Ryosuke Niwa 2017-09-24 17:18:04 PDT
Comment on attachment 321658 [details]
Fix Win/GTK/WPE builds, round 2

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

>>> Source/WebCore/dom/DataTransfer.cpp:239
>>> +    m_pasteboard = WTFMove(finalPasteboard);
>> 
>> Not necessarily for this patch.
>> 
>> I am starting to understand what StaticPasteboard is now. I think that the name "static pasteboard" is not quite right; perhaps "internal pasteboard" to distinguish from "platform pasteboard" is the right way to do it. Also seems like the base class Pasteboard would have two derived classes, one called PlatformPasteboard and a concrete one InternalPasteboard. Unfortunately we use the name PlatformPasteboard for a separate object; too many layers I think! The PlatformPasteboard class could be abstract if we use virtual functions for the cross platform pattern, or concrete if we use the non-virtual compile-time technique.
>> 
>> Can we do this with a virtual function instead of using the is and downcast functions? Maybe something like this:
>> 
>>     if (m_pasteboard->exportContentsToPlatformPasteboard(*finalPasteboard))
>>         m_pasteboard = WTFMove(finalPasteboard);
>> 
>> Or maybe not. I do think that using distinct types would help this code be readable. The argument would be std::unique_ptr<PlatformPasteboard>&& for example.
> 
> I don't think we should do this.

I think it makes sense to make Pasteboard an abstract class in the long term.
Wenson & I came up with alternatives: StagingPasteboard & PrivatePasteboard but InternalPasteboard sounds good too.
One challenge is that there's a lot of platform specific code in Pasteboard object so we need to do more refactoring.
Once that's done, we just need to add a function in PlatformStrategy to create a WK2 proxy, and we're good to go.
So we'd have three kinds: InternalPasteboard, SystemPasteboard (current Pasteboard), & WebKit2Pasteboard.
WebKit2Pasteboard would simply funnel through IPCs to talk to SystemPasteboard in UIProcess.
Comment 17 Wenson Hsieh 2017-09-24 21:18:37 PDT
Comment on attachment 321658 [details]
Fix Win/GTK/WPE builds, round 2

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

>> Source/WebCore/ChangeLog:15
>> +        applications to unvetted web content.
> 
> Letting web content read these types only pose privacy concerns. Letting them write to these types can pose security concerns.

Good point. Tweaked the wording to reflect this.

>> Source/WebCore/ChangeLog:26
>> +        This patch adopts this same approach in WebKit, and introduces the org.webkit.custom-pasteboard-data UTI (refer
> 
> We should use com.apple.WebKit.custom-pasteboard-data since that's what we use elsewhere.

Sounds good -- done!

I originally chose an org.webkit.~ prefix here so that if other ports wanted to adopt this strategy, they wouldn't have to declare their own custom data pasteboard type, but since this is only for Cocoa platforms right now, I suppose that's OK.

>> Source/WebCore/ChangeLog:52
>> +        Pasteboard, clears out the DataTransfer's StaticPasteboard, and replaces it with the new Pasteboard.
> 
> Why do we need to do that? I don't see why we'd like to have that kind of side-effect in commitStaticPasteboardToPasteboard.
> If you're doing this for drag & drop, then a better thing to do is to create a new DataTransfer for dragstart event as mandated by the spec.

Correct -- I made this change in adopting StaticPasteboard during dragstart. How this works currently is:

  - Create a new DataTransfer using a StaticPasteboard (this is new in the patch)
  - Compute and set the default drag image if we're dragging a `draggable=true` element
  - Dispatch dragstart to the page, where JS may setData and setDragImage
  - Write the StaticPasteboard's contents to the platform if needed (this is also new in the patch)
  - Connect the DataTransfer to the platform pasteboard
  - Invalidate the DataTransfer, preventing the page from making any further changes

...and then, later down the road in DragController::startDrag...

  - If the page didn't set any data during dragstart, write default data to the platform pasteboard
  - Likewise, if the page didn't set any drag image during dragstart, compute and set the drag image

If I understand correctly, are you proposing the new flow should be something like this?

  - Create a new DataTransfer using a StaticPasteboard
  - Compute and set the default drag image if we're dragging a `draggable=true` element
  - Dispatch dragstart to the page, where JS may setData and setDragImage
  - Write the StaticPasteboard's contents to the platform if needed

    (...this is where things start to diverge...)

  - Create a new DataTransfer using the platform Pasteboard
  - Commit first DataTransfer's StaticPasteboard to the platform Pasteboard of the new DataTransfer
  - Copy all the information from the old DataTransfer (i.e. drag image, drag image element, drag offset, etc.) into the new DataTransfer
  - Invalidate the new DataTransfer, and set dragState().dataTransfer to the new DataTransfer

    (...and then commence with the new DataTransfer in DragController::startDrag, writing to the pasteboard as needed)

>> Source/WebCore/dom/DataTransfer.cpp:235
>> +        return;
> 
> I would rather turn this into RELEASE_ASSERT than adding an early return.
> This is not a perf sensitive code, and it's pretty obvious from your code change that it should never happen in practice.

Sounds good!

>> Source/WebCore/page/EventHandler.cpp:3696
>> +        pasteboard->clear();
> 
> Unclear why "clear" is needed. Probably needs a "why" comment.

Added!

>> Source/WebCore/page/EventHandler.cpp:3697
>> +        dragState().dataTransfer->commitStaticPasteboardToPasteboard(WTFMove(pasteboard));
> 
> Instead of doing this, create a new DataTransfer object with StaticPasteboard, dispatch dragstart with that Pasteboard,
> and then commit that to dragState().dataTransfer which continues to use the "real" pasteboard.
> That is, flip around the pasteboards you're using.

There's a lot of state on DataTransfer that would also need to be ferried over from the old DataTransfer to the new one, but this is feasible. I'll look into it.

>> Source/WebCore/platform/Pasteboard.cpp:42
>> +    static int currentCustomDataSerializationVersion = 1;
> 
> constexpr or const

👍🏻

>> Source/WebCore/platform/Pasteboard.cpp:53
>> +    static int maxSupportedDataSerializationVersionNumber = 1;
> 
> constexpr or const

👍🏻

>>> Source/WebCore/platform/Pasteboard.cpp:56
>>> +    WTF::Persistence::Decoder decoder((const uint8_t *)buffer->data(), buffer->size());
>> 
>> Use reinterpret_cast instead of C-style cast. No space before "*". Also consider braces instead of () for constructor arguments?
> 
> Please use static_cast.

Changed to reinterpret_cast, and braces for constructor arguments. The compiler doesn't allow static_casting from (const char*) to (const uint8_t*).

>> Source/WebCore/platform/Pasteboard.cpp:59
>> +        return { };
> 
> Because we use "int" here there could be negative numbers. I suggest we reject them instead of treating them all as supported versions.

Good point! I changed from int type to unsigned for the version number (it don't think it makes sense to allow negative version numbers anyways).

>> Source/WebCore/platform/Pasteboard.h:160
>> +WEBCORE_EXPORT Ref<SharedBuffer> sharedBufferFromCustomData(const HashMap<String, String>& customData, const Vector<String>& types);
> 
> Why doesn’t this take const PasteboardCustomData& instead of taking the map and vector separately?

Sure -- I could do that. I didn't include the public data of PasteboardCustomData isn't included in this shared buffer, since the platform pasteboard is solely responsible for storing it.

>> Source/WebCore/platform/Pasteboard.h:161
>> +WEBCORE_EXPORT HashMap<String, String> readCustomDataFromSharedBuffer(const Ref<SharedBuffer>&, Vector<String>& outTypes);
> 
> In new code we can use structs to return multiple return values instead of using out arguments. So this should return both the map and the vector, not return the map and fill in a passed-in vector. I would suggest returning PasteboardCustomData but I don’t understand exactly why that has two maps and this function returns only one.
> 
> No reason to take const Ref<SharedBuffer>& instead of taking SharedBuffer& or const SharedBuffer&.

True. I changed this to take a const SharedBuffer& and return a PasteboardCustomData.

>>> Source/WebCore/platform/Pasteboard.h:164
>>> +static const char *customWebKitPasteboardDataType = "org.webkit.custom-pasteboard-data";
>> 
>> Type here should be "const char* const xxx" or "const char xxx[]" not "const char *" (note also no space before the "*"). The type in this patch says that the string is constant but there is a non-constant global variable pointing to it. And since we say static, it’s a separate non-constant global variable in each source file that compiles this header.
> 
> Use com.apple.WebKit.custom-pasteboard-data.

👍🏻

>> Source/WebCore/platform/Pasteboard.h:168
>> +WEBCORE_EXPORT String webExposedPasteboardTypeForPlatformType(const String& platformType);
> 
> Does this need to use String? I think these are always compile-time constants that can use "const char*"; we never need to convert them into a String. Would be more efficient to leave them that way instead of making a String unless there is something about that which would be unclear.

Sounds good. Changed to return const char* here.

>> Source/WebCore/platform/StaticPasteboard.cpp:63
>> +    m_changeCount++;
> 
> We normally write pre-decrement, although I suppose post-decrement is OK.

Oh, right -- I forgot about that guideline. Changed to ++m_changeCount;

>> Source/WebCore/platform/StaticPasteboard.cpp:78
>> +    m_changeCount++;
> 
> Ditto.

👍🏻

>> Source/WebCore/platform/StaticPasteboard.cpp:99
>> +    pasteboard.writeCustomData(data);
> 
> Unfortunate that this has to copy the vector and the two maps just to pass them to a function that doesn’t need to keep the copy around. Should refactor the code to avoid having to make those copies.

Good point. Could we address this by WTFMoving m_types, m_webExposedData, m_customData into a new PasteboardCustomData when we writeCustomData?

>> Source/WebCore/platform/StaticPasteboard.h:83
>> +    long m_changeCount;
> 
> Looks like this is uninitialized. Should we initialize it to zero?

👍🏻 Good catch!

>> Source/WebCore/platform/ios/PasteboardIOS.mm:150
>> +        return "text/plain";
> 
> Normally we use ASCIILiteral in a case like this.

👍🏻

>> Source/WebCore/platform/ios/PasteboardIOS.mm:153
>> +        return "text/html";
> 
> Ditto.

👍🏻

>> Source/WebCore/platform/ios/PasteboardIOS.mm:156
>> +        return "text/uri-list";
> 
> Ditto.

👍🏻

>> Source/WebCore/platform/ios/PasteboardIOS.mm:447
>> +    return platformStrategies()->pasteboardStrategy()->webExposedTypes(m_pasteboardName);
> 
> Nice. I was gonna add this thing for my image patch.

\o/

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:448
>> +    return result;
> 
> Clearly we need a version of copyToVector with a return value instead of one that uses an out argument. Maybe we don’t need the one that takes a vector argument at all any more.

I did find copyToVector somewhat cumbersome to use -- a version of this that just returns a vector would be nice. I would prefer to do this refactoring in a followup to prevent this patch from getting much bigger though, since there are other places in WebKit that would read more clearly with a return version of copyToVector, and I'd like to change all of these places to use the new copyToVector.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:476
>> +        NSString *stringValue = data.publicData.get(type);
> 
> This is a double hash lookup. That’s never needed and always twice as slow as the correct idiom. This can be done by relying on the fact that get returns null string when there is no string, or it can be done with find instead of get.
> 
>     NSString *stringValue = data.publicData.get(type);
>     if (!stringValue)
>         continue;
> 
> Or:
> 
>     auto value = data.publicData.find(type);
>     if (value == data.publicData.end())
>         continue;
> 
>     NSString *stringValue = value->value;

Good point -- fixed!

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:512
>> +            return customData.get(type);
> 
> This is a double hash lookup. That’s never needed and always twice as slow as the correct idiom. This can be done by relying on the fact that get returns null string when there is no string, or it can be done with find instead of get.
> 
>     auto data = readCustomDataFromSharedBuffer(*buffer, types).get(type);
>     if (!data.isNull())
>         return data;
> 
> Or:
> 
>     auto customData = readCustomDataFromSharedBuffer(*buffer, types);
>     auto data = customData.find(type);
>     if (data != customData.end())
>         return data->value;

Fixed! (I went with the former approach).

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:527
>> +        value = [[[NSString alloc] initWithData:(NSData *)value encoding:NSUTF8StringEncoding] autorelease];
> 
> Unfortunate to use autorelease here. Instead I suggest we consider making "value" be a RetainPtr<id> and using adoptNS here.

👍🏻

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:555
>> +            [typesToLoad addObject:@(customWebKitPasteboardDataType)];
> 
> A real shame that this allocates two NSString objects. There might be an idiom that avoids that.

Good point. FWIW, I tweaked this so we make a new NSString out of customWebKitPasteboardDataType outside this loop and use that NSString within the loop, so that we don't make 1 or more NSStrings per iteration, but rather just one at the beginning.

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:584
>> +        foundAnyDataToLoad = foundAnyDataToLoad || typeIdentifiersToLoad.count;
> 
> Use ||= ?

👍🏻

>> Source/WebCore/platform/mac/PasteboardMac.mm:536
>> +        return "text/plain";
> 
> ASCIILiteral.

👍🏻

>> Source/WebCore/platform/mac/PasteboardMac.mm:539
>> +        return "text/uri-list";
> 
> ASCIILiteral.

👍🏻

>> Source/WebCore/platform/mac/PasteboardMac.mm:542
>> +        return "text/html";
> 
> ASCIILiteral.

👍🏻

>> Source/WebCore/platform/mac/PasteboardMac.mm:552
>> +        return "Files";
> 
> ASCIILiteral.

👍🏻

>> Source/WebCore/platform/win/PasteboardWin.cpp:1061
>> +
> 
> We shouldn't be adding changeCount to other platforms since that's not a thing
> in terms of how clipboard works in Windows, GTK+, etc...

Ah, good point. Restored this to being PLATFORM(COCOA)
Comment 18 Wenson Hsieh 2017-09-25 08:03:24 PDT
Created attachment 321684 [details]
Fix Win/GTK/WPE builds, round 4
Comment 19 Wenson Hsieh 2017-09-25 11:39:46 PDT
Created attachment 321710 [details]
Fix the GTK build
Comment 20 Wenson Hsieh 2017-09-25 12:49:28 PDT
Created attachment 321726 [details]
Fix the GTK build (2)
Comment 21 Wenson Hsieh 2017-09-25 21:59:34 PDT
Created attachment 321791 [details]
Remove commitStaticPasteboardToPasteboard
Comment 22 Wenson Hsieh 2017-09-25 22:08:10 PDT
Created attachment 321792 [details]
Remove commitStaticPasteboardToPasteboard and rebase
Comment 23 Build Bot 2017-09-25 23:19:23 PDT
Comment on attachment 321792 [details]
Remove commitStaticPasteboardToPasteboard and rebase

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

New failing tests:
fast/events/mousedown-inside-dragstart-should-not-cause-crash.html
Comment 24 Build Bot 2017-09-25 23:19:26 PDT
Created attachment 321798 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 25 Build Bot 2017-09-25 23:24:27 PDT
Comment on attachment 321792 [details]
Remove commitStaticPasteboardToPasteboard and rebase

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

New failing tests:
fast/events/mousedown-inside-dragstart-should-not-cause-crash.html
Comment 26 Build Bot 2017-09-25 23:24:28 PDT
Created attachment 321799 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 27 Build Bot 2017-09-25 23:41:42 PDT
Comment on attachment 321792 [details]
Remove commitStaticPasteboardToPasteboard and rebase

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

New failing tests:
fast/events/mousedown-inside-dragstart-should-not-cause-crash.html
Comment 28 Build Bot 2017-09-25 23:41:43 PDT
Created attachment 321801 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 29 Wenson Hsieh 2017-09-26 00:20:48 PDT
Created attachment 321805 [details]
Remove commitStaticPasteboardToPasteboard (2)
Comment 30 Ryosuke Niwa 2017-09-26 14:35:23 PDT
Comment on attachment 321805 [details]
Remove commitStaticPasteboardToPasteboard (2)

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

> Source/WebCore/dom/DataTransfer.cpp:228
> +    auto dataTransfer = createWithStaticPasteboard(Type::InputEvent);
> +    dataTransfer->pasteboard().writeString(ASCIILiteral("text/plain"), plainText);
> +    dataTransfer->pasteboard().writeString(ASCIILiteral("text/html"), htmlText);
> +    return dataTransfer;

I think it's cleaner to directly create a StaticPasteboard, call writeString on it, and then construct DataTransfer.

> Source/WebCore/dom/DataTransfer.cpp:234
> +Ref<DataTransfer> DataTransfer::createWithStaticPasteboard(Type type)
> +{
> +    return adoptRef(*new DataTransfer(StoreMode::ReadWrite, std::make_unique<StaticPasteboard>(), type));
>  }

I think it's cleaner to have this code in createForCopyAndPaste and createForInputEvent instead of having this helper function
since createForInputEvent requires us to pre-fill the contents of the pasteboard.

> Source/WebCore/dom/DataTransfer.cpp:470
> +void DataTransfer::moveDraggingInformationFromDataTransfer(Ref<DataTransfer>&& other)

I would have called this moveDragState() or something.
Given this is on DataTransfer which takes DataTransfer, saying "FromDataTransfer" is redundant.

> Source/WebCore/dom/DataTransfer.cpp:476
> +    RELEASE_ASSERT(is<StaticPasteboard>(other->pasteboard()));

This assert should appear before the comment.

> Source/WebCore/dom/DataTransfer.h:51
> +    enum class Type { CopyAndPaste, DragAndDropData, DragAndDropFiles, InputEvent };
> +    static Ref<DataTransfer> createWithStaticPasteboard(Type);
> +

We shouldn't let client specify a random type like this. That can lead to people abusing it.

> Source/WebCore/page/EventHandler.cpp:3557
>  #if PLATFORM(COCOA)
> -    auto changeCountBeforeDragStart = dragState().dataTransfer->pasteboard().changeCount();
> +    auto changeCountBeforeDragStart = pasteboard.changeCount();
>  #endif
>  

We can just delete this code since changeCount isn't going to change within StaticPasteboard.

> Source/WebCore/page/EventHandler.cpp:3558
> +    bool mayStartDrag = !dispatchDragEvent(eventNames().dragstartEvent, *dragState().source, m_mouseDown, &dataTransfer);

It's important to distinguish events that are dispatched on the source element of the drag versus where it's being dropped.
This is why this method was called dispatchDragSrcEvent. We should perhaps rename it to dispatchDragEventOnSourceElement
but we should keep it clear that this is about dispatching the event on the originating element of the current drag session.

> Source/WebCore/page/EventHandler.cpp:3564
>  #if PLATFORM(COCOA)
> -    hasNonDefaultPasteboardData = changeCountBeforeDragStart != dragState().dataTransfer->pasteboard().changeCount() ? HasNonDefaultPasteboardData::Yes : HasNonDefaultPasteboardData::No;
> +    hasNonDefaultPasteboardData = changeCountBeforeDragStart != pasteboard.changeCount() ? HasNonDefaultPasteboardData::Yes : HasNonDefaultPasteboardData::No;
>  #else
> -    hasNonDefaultPasteboardData = dragState().dataTransfer->pasteboard().hasData() ? HasNonDefaultPasteboardData::Yes : HasNonDefaultPasteboardData::No;
> +    hasNonDefaultPasteboardData = pasteboard.hasData() ? HasNonDefaultPasteboardData::Yes : HasNonDefaultPasteboardData::No;
>  #endif

Again, we can just use non-Cocoa code path for this.

> Source/WebCore/page/EventHandler.cpp:3676
> +        auto dragStartDataTransfer = DataTransfer::createWithStaticPasteboard(DataTransfer::Type::DragAndDropData);

We should create a new static function like DataTransfer::createForDragStart instead of exposing internal states like DataTransfer::Type.

> Source/WebCore/page/EventHandler.cpp:3689
>                  m_mouseDownMayStartDrag = false;
>                  invalidateDataTransfer();
>                  dragState().source = nullptr;

There's a bug here that we don't fire dragend here. Conceptually, drag has already started once we've fired dragstart so we need to end it by firing dragend.
We can fix it in a separate bug of course.

> Source/WebCore/page/mac/EventHandlerMac.mm:-723
> -Ref<DataTransfer> EventHandler::createDraggingDataTransfer() const

Nice!

> Source/WebCore/platform/Pasteboard.cpp:35
> +bool isWebExposedPasteboardType(const String& type)

We usually use "DOM" or "bindings" mean "web exposed" since Web* historically refers to WebKitLegacy classes.

> Source/WebCore/platform/Pasteboard.h:151
> +// PasteboardCustomData represents custom data supplied by web content. Public data represents data that
> +// is exposed to other web pages (regardless of origin) upon paste or drop; private data represents data
> +// that is exposed only to frames that share the same origin.

Instead of adding a long comment like this, we should just rename "publicData" to "webExposedData" and "privateData" to "sameOriginCustomData"

> Source/WebCore/platform/Pasteboard.h:153
> +// We also include an ordered list of pasteboard types to ensure that the order in which the destination
> +// enumerates pasteboard types is the same as the order in which they were written by the source.

Again, we should just rename "webExposedTypes" to "orderedTypes" instead of adding a comment like this.

> Source/WebCore/platform/Pasteboard.h:246
> -    long changeCount() const;
> +    virtual long changeCount() const;

I don't think StaticPasteboard needs to support changeCount once you've made the change I suggested in EventHandler.cpp.

> Source/WebCore/platform/StaticPasteboard.h:75
> +    HashMap<String, String> m_webExposedData;
> +    HashMap<String, String> m_customData;

So, both of them are exposed to Web API so it's a bit misleading to call it m_webExposedData and m_customData.
How about m_nativeData and m_customData? or m_platformData and m_customData?

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:37
> +bool supportsCustomPasteboardData()
> +{
> +    return false;
> +}

Why isn't this a runtime/build flag? If not, it should at least be a member of Pasteboard class.

> Source/WebCore/platform/ios/PasteboardIOS.mm:374
> +    if (type == "text/html")

Can we just call platformPasteboardTypeForWebExposedType here instead of duplicating code?

> Source/WebCore/platform/ios/PasteboardIOS.mm:452
> +    return platformStrategies()->pasteboardStrategy()->webExposedTypes(m_pasteboardName);

I don't think the fact these types are exposed to Web is all that interesting.
It's more about the fact these types can be read & written directly from Web API.
How about typesSafeForDOMToReadWrite?
Comment 31 Ryosuke Niwa 2017-09-26 14:45:45 PDT
Comment on attachment 321805 [details]
Remove commitStaticPasteboardToPasteboard (2)

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

> Source/WebCore/platform/mac/PasteboardMac.mm:555
> +    // For compatibility on Mac, we need to additionally supply the 'Files' pasteboard type to indicate
> +    // that files are present on the pasteboard. This is only on the pasteboard reading side (i.e. there
> +    // is no mapping from the web-exposed 'Files' type to a platform Cocoa type) because web content
> +    // shouldn't be able to write file URLs to the pasteboard using the DataTransfer's setData API in the
> +    // first place. Since DataTransfer.getData('Files') should never return an actual file URL, this is
> +    // effectively just a flag on DataTransfer that web content uses to determine that DataTrasfer.files
> +    // contains a list of File objects.

I don't think we need this comment since this is a spec'ed behavior.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:95
> +        // If we were unable to initialize a URL using -URLFromPasteboard:, fall back to reading the string data and create a new NSURL with it.

This comment just repeats the code. Please remove.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:107
> +    // First, include all dynamically inserted types.

I don't think we need this comment.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:115
> +    // Then, add all web exposed types, coercing platform types into web-exposed types as needed.

Or this comment.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/dump-datatransfer-types.html:66
> +    "baz/uri-list" : "https://www.webkit.org"

You should have types with emoji, CJK characters, & unicode control characters to make sure they don't go haywire.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/dump-datatransfer-types.html:71
> +    let pasteboard = event.dataTransfer || event.clipboardData;
> +    let eventData = {};

const?

> LayoutTests/editing/pasteboard/data-transfer-get-data-on-drop-custom.html:36
> +    let eventData = {};

const?

> LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-plain-text.html:37
> +    let eventData = {};
> +    for (let type of event.clipboardData.types)

const?

> LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-rich-text.html:37
> +    let eventData = {};
> +    for (let type of event.clipboardData.types)

const?
Comment 32 Wenson Hsieh 2017-09-26 20:10:10 PDT
Comment on attachment 321805 [details]
Remove commitStaticPasteboardToPasteboard (2)

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

>> Source/WebCore/dom/DataTransfer.cpp:228
>> +    return dataTransfer;
> 
> I think it's cleaner to directly create a StaticPasteboard, call writeString on it, and then construct DataTransfer.

Ok, done!

>> Source/WebCore/dom/DataTransfer.cpp:234
>>  }
> 
> I think it's cleaner to have this code in createForCopyAndPaste and createForInputEvent instead of having this helper function
> since createForInputEvent requires us to pre-fill the contents of the pasteboard.

Ok. Removed createWithStaticPasteboard().

>> Source/WebCore/dom/DataTransfer.cpp:470
>> +void DataTransfer::moveDraggingInformationFromDataTransfer(Ref<DataTransfer>&& other)
> 
> I would have called this moveDragState() or something.
> Given this is on DataTransfer which takes DataTransfer, saying "FromDataTransfer" is redundant.

Ok, done. I originally had something similar to moveDragState here, but decided against it to avoid any relation to the WebCore::DragState struct.

>> Source/WebCore/dom/DataTransfer.cpp:476
>> +    RELEASE_ASSERT(is<StaticPasteboard>(other->pasteboard()));
> 
> This assert should appear before the comment.

Ah, yes. Fixed.

>> Source/WebCore/dom/DataTransfer.h:51
>> +
> 
> We shouldn't let client specify a random type like this. That can lead to people abusing it.

Moved back to private, and removed createWithStaticPasteboard.

>> Source/WebCore/page/EventHandler.cpp:3557
>>  
> 
> We can just delete this code since changeCount isn't going to change within StaticPasteboard.

Good point! Removed.

>> Source/WebCore/page/EventHandler.cpp:3558
>> +    bool mayStartDrag = !dispatchDragEvent(eventNames().dragstartEvent, *dragState().source, m_mouseDown, &dataTransfer);
> 
> It's important to distinguish events that are dispatched on the source element of the drag versus where it's being dropped.
> This is why this method was called dispatchDragSrcEvent. We should perhaps rename it to dispatchDragEventOnSourceElement
> but we should keep it clear that this is about dispatching the event on the originating element of the current drag session.

Sounds good! Changed to dispatchDragStartEventOnSourceElement.

>> Source/WebCore/page/EventHandler.cpp:3564
>>  #endif
> 
> Again, we can just use non-Cocoa code path for this.

👍🏻 Removed the platform special casing so that we just call Pasteboard::hasData() after dispatching the dragstart, at the call site.

>> Source/WebCore/page/EventHandler.cpp:3676
>> +        auto dragStartDataTransfer = DataTransfer::createWithStaticPasteboard(DataTransfer::Type::DragAndDropData);
> 
> We should create a new static function like DataTransfer::createForDragStart instead of exposing internal states like DataTransfer::Type.

(After talking in person, we agreed to go with createForDrag and createForDragStartEvent for the platform-connected-pasteboard and static-pasteboard-backed DataTransfers, respectively).

>> Source/WebCore/page/EventHandler.cpp:3689
>>                  dragState().source = nullptr;
> 
> There's a bug here that we don't fire dragend here. Conceptually, drag has already started once we've fired dragstart so we need to end it by firing dragend.
> We can fix it in a separate bug of course.

Good catch! I fixed it here.

>> Source/WebCore/page/mac/EventHandlerMac.mm:-723
>> -Ref<DataTransfer> EventHandler::createDraggingDataTransfer() const
> 
> Nice!

:)

>> Source/WebCore/platform/Pasteboard.cpp:35
>> +bool isWebExposedPasteboardType(const String& type)
> 
> We usually use "DOM" or "bindings" mean "web exposed" since Web* historically refers to WebKitLegacy classes.

Got it -- what about isDOMPasteboardType?

>> Source/WebCore/platform/Pasteboard.h:151
>> +// that is exposed only to frames that share the same origin.
> 
> Instead of adding a long comment like this, we should just rename "publicData" to "webExposedData" and "privateData" to "sameOriginCustomData"

Ok! (I went with platformData and sameOriginCustomData, per your comments below).

(An aside: it's probably worth having a FIXME here mentioning that the same origin policy for drag data needs to actually be implemented!)

>> Source/WebCore/platform/Pasteboard.h:153
>> +// enumerates pasteboard types is the same as the order in which they were written by the source.
> 
> Again, we should just rename "webExposedTypes" to "orderedTypes" instead of adding a comment like this.

Done!

>> Source/WebCore/platform/Pasteboard.h:246
>> +    virtual long changeCount() const;
> 
> I don't think StaticPasteboard needs to support changeCount once you've made the change I suggested in EventHandler.cpp.

Yep! Removed.

>> Source/WebCore/platform/StaticPasteboard.h:75
>> +    HashMap<String, String> m_customData;
> 
> So, both of them are exposed to Web API so it's a bit misleading to call it m_webExposedData and m_customData.
> How about m_nativeData and m_customData? or m_platformData and m_customData?

I like m_platformData and m_customData.

>> Source/WebCore/platform/gtk/PasteboardGtk.cpp:37
>> +}
> 
> Why isn't this a runtime/build flag? If not, it should at least be a member of Pasteboard class.

I hadn't added the runtime enabling/disabling yet in this patch (I had originally intended to do it in a separate followup).

>> Source/WebCore/platform/ios/PasteboardIOS.mm:374
>> +    if (type == "text/html")
> 
> Can we just call platformPasteboardTypeForWebExposedType here instead of duplicating code?

Good call. Fixed.

>> Source/WebCore/platform/ios/PasteboardIOS.mm:452
>> +    return platformStrategies()->pasteboardStrategy()->webExposedTypes(m_pasteboardName);
> 
> I don't think the fact these types are exposed to Web is all that interesting.
> It's more about the fact these types can be read & written directly from Web API.
> How about typesSafeForDOMToReadWrite?

Changed to typesSafeForDOMToReadAndWrite

>> Source/WebCore/platform/mac/PasteboardMac.mm:555
>> +    // contains a list of File objects.
> 
> I don't think we need this comment since this is a spec'ed behavior.

Ah, I see. Removed.

>> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:95
>> +        // If we were unable to initialize a URL using -URLFromPasteboard:, fall back to reading the string data and create a new NSURL with it.
> 
> This comment just repeats the code. Please remove.

Removed.

>> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:107
>> +    // First, include all dynamically inserted types.
> 
> I don't think we need this comment.

Removed.

>> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:115
>> +    // Then, add all web exposed types, coercing platform types into web-exposed types as needed.
> 
> Or this comment.

Removed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/dump-datatransfer-types.html:71
>> +    let eventData = {};
> 
> const?

Done!

>> LayoutTests/editing/pasteboard/data-transfer-get-data-on-drop-custom.html:36
>> +    let eventData = {};
> 
> const?

Changed all of these to use const eventData = {};

>> LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-plain-text.html:37
>> +    for (let type of event.clipboardData.types)
> 
> const?

Changed these all to for (const type of event.~.types)

>> LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-rich-text.html:37
>> +    for (let type of event.clipboardData.types)
> 
> const?

Done!
Comment 33 Ryosuke Niwa 2017-09-26 21:47:30 PDT
Is a new patch coming??
Comment 34 Wenson Hsieh 2017-09-27 00:03:22 PDT
Created attachment 321941 [details]
Address review feedback
Comment 35 Wenson Hsieh 2017-09-27 00:07:16 PDT
(In reply to Ryosuke Niwa from comment #33)
> Is a new patch coming??

Sorry for the delay -- ran into some snags when refactoring code/tests and adding the runtime switch. New patch up now.
Comment 36 Wenson Hsieh 2017-09-27 00:38:10 PDT
Created attachment 321942 [details]
Fix iOS 11.0 build
Comment 37 Build Bot 2017-09-27 01:53:29 PDT
Comment on attachment 321942 [details]
Fix iOS 11.0 build

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

New failing tests:
editing/pasteboard/dataTransfer-setData-getData.html
Comment 38 Build Bot 2017-09-27 01:53:30 PDT
Created attachment 321944 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 39 Ryosuke Niwa 2017-09-27 01:57:45 PDT
Comment on attachment 321942 [details]
Fix iOS 11.0 build

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

This patch appears to be getting confused about MIME types used in DOM versus Cocoa types.
We should only use MIME types used in DOM inside our custom data map.

> Source/WebCore/ChangeLog:83
> +        instead of using platform-dependent logic to compare changeCounts.

You should also mention why this code is no longer needed
that static pasteboard can't change underneath us or contain additional items at the beginning.

> Source/WebCore/ChangeLog:87
> +        Tweak dispatchDragStartEvent to take a DataTransfer to expose to the page; at the call site in handleDrag,

Nit: expose to DOM/bindings.

> Source/WebCore/platform/Pasteboard.cpp:36
> +bool isDOMPasteboardType(const String& type)

Why don't call this isSafeTypeForDOMToReadAndWrite to match typesSafeForDOMToReadAndWrite?

Also, looks like these functions can be protected members of Pasteboard?
It's probably better to do that instead of making it available globally in any translation unit where Pasteboard.h is included.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:429
> +            domPasteboardTypes.add(type);

This implies that custom data contains a map of MIME type as used in DOM but down in PlatformPasteboard::readString,
doing the following where type is cocoa type. I don't think that'd work.
customDataFromSharedBuffer(*buffer).sameOriginCustomData.get(type)

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:442
> +        auto coercedType = String::fromUTF8(domPasteboardTypeForPlatformType(type));
> +        if (!coercedType.isEmpty())

Instead of converting to string and then checking emptiness, why don't we just do:
if (auto* domType = domPasteboardTypeForPlatformType(type))
    domPasteboardTypes.add(String::fromUTF8(domType));

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:508
> +    if (auto buffer = readBuffer(index, customWebKitPasteboardDataType)) {

It comes off a little inefficient to check the custom data types first since that would require decoding the contents.
Why don't we check this only when the type isn't safe for DOM to read/write?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:-422
> -    if (![pasteboardItem count])
> -        return String();

We need to have an early return here when types is not safe for DOM and customPasteboardDataEnabled() is enabled
since DataTransfer::getData lets you specify arbitrary mime type for now.

> Source/WebCore/platform/mac/PasteboardMac.mm:531
> +const char* domPasteboardTypeForPlatformType(const String& platformType)

Looks like this should be a private/protected member.

> Source/WebCore/platform/mac/PasteboardMac.mm:587
> +String platformPasteboardTypeForDOMType(const String& domType)

Ditto.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:87
> +        auto customDataValue = customDataFromSharedBuffer(buffer.get()).sameOriginCustomData.get(pasteboardType);

Again, this pasteboardType had already been converted to cocoa type.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:125
> +            auto coercedType = String::fromUTF8(domPasteboardTypeForPlatformType(type));
> +            if (coercedType == "Files" && !numberOfFiles())
> +                continue;
> +            if (!coercedType.isEmpty())

Again, why don't we avoid creating string until we're confirmed it's not empty string?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/dump-datatransfer-types.html:66
> +    "baz/uri-list" : "https://www.webkit.org"

Again, we need tests with exotic characters in type name.
Given the bug this patch appears to have (cocoa/DOM type confusion),
we should probably also test actual cocoa type we don't expose to DOM like public.rtf and public.file-url
Comment 40 Build Bot 2017-09-27 02:07:28 PDT
Comment on attachment 321942 [details]
Fix iOS 11.0 build

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

New failing tests:
editing/pasteboard/drag-link-with-data-transfer-adds-trusted-link-to-pasteboard.html
editing/pasteboard/dataTransfer-setData-getData.html
Comment 41 Build Bot 2017-09-27 02:07:29 PDT
Created attachment 321945 [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
Comment 42 Wenson Hsieh 2017-09-27 10:18:10 PDT
Comment on attachment 321942 [details]
Fix iOS 11.0 build

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

>> Source/WebCore/ChangeLog:83
>> +        instead of using platform-dependent logic to compare changeCounts.
> 
> You should also mention why this code is no longer needed
> that static pasteboard can't change underneath us or contain additional items at the beginning.

👍🏻

>> Source/WebCore/ChangeLog:87
>> +        Tweak dispatchDragStartEvent to take a DataTransfer to expose to the page; at the call site in handleDrag,
> 
> Nit: expose to DOM/bindings.

👍🏻

>> Source/WebCore/platform/Pasteboard.cpp:36
>> +bool isDOMPasteboardType(const String& type)
> 
> Why don't call this isSafeTypeForDOMToReadAndWrite to match typesSafeForDOMToReadAndWrite?
> 
> Also, looks like these functions can be protected members of Pasteboard?
> It's probably better to do that instead of making it available globally in any translation unit where Pasteboard.h is included.

The set of types safe to read and write (any of the three special-cased types here *or* any custom type supplied by the same origin) is different from the set of types here (which is just the three special types). I wanted to avoid conflating the two. Additionally, the other two helper functions should ideally refer to the special DOM/bindings-exposed types in the same way, so platformPasteboardTypeForDOMType would need to be platformPasteboardTypeForSafeTypeForDOMToReadAndWrite, which I thought was too verbose. But I can rename this.

Also, isSafeTypeForDOMToReadAndWrite cannot be protected in Pasteboard because it's used in both the PlatformPasteboards and StaticPasteboard, and platformPasteboardTypeForSafeTypeForDOMToReadAndWrite cannot be protected in Pasteboard because it's used in Pasteboard as well as the PlatformPasteboards. However, safeTypeForDOMToReadAndWriteForPlatformType is just used in PlatformPasteboard{Mac|IOS}, so we can just make it static there and not expose it at all from any header. I'll do that.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:426
> +        Vector<String> types;

(Whoops, I meant to remove this. Removed.)

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:429
>> +            domPasteboardTypes.add(type);
> 
> This implies that custom data contains a map of MIME type as used in DOM but down in PlatformPasteboard::readString,
> doing the following where type is cocoa type. I don't think that'd work.
> customDataFromSharedBuffer(*buffer).sameOriginCustomData.get(type)

Oh, good point! To rephrase this slightly: the concern is that orderedTypes is a list of (DOM type) and sameOriginCustomData is a map of (DOM type) => value, but PlatformPasteboard::readString takes a (Cocoa type) and searches sameOriginCustomData using this (Cocoa type). So something like this following scenario won't work:

    1. On dragstart, execute event.dataTransfer.setData("text/rtf", "foo")
    2. On drop, attempting to do event.dataTransfer.getData("text/rtf") yields "" because "text/rtf" was mapped to "public.rtf" by Pasteboard{IOS|Mac}. (Or, in the case of Mac, we actually bail altogether in cocoaTypeFromHTMLClipboardType and just return "" for the type!)

...we can fix this by sending the type that the DOM asked for to PlatformPasteboard, adding something like a domType argument to readString(). Or, we can fix this by having Pasteboard ask the PlatformPasteboard for the custom data buffer itself. I have a strong preference for the the latter, because we could then put that logic in Pasteboard.cpp and avoid more platform-dependent code. What do you think?

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:442
>> +        if (!coercedType.isEmpty())
> 
> Instead of converting to string and then checking emptiness, why don't we just do:
> if (auto* domType = domPasteboardTypeForPlatformType(type))
>     domPasteboardTypes.add(String::fromUTF8(domType));

Nice! That's definitely cleaner than what I'm doing here.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:508
>> +    if (auto buffer = readBuffer(index, customWebKitPasteboardDataType)) {
> 
> It comes off a little inefficient to check the custom data types first since that would require decoding the contents.
> Why don't we check this only when the type isn't safe for DOM to read/write?

Sure, we could do that.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:-422
>> -        return String();
> 
> We need to have an early return here when types is not safe for DOM and customPasteboardDataEnabled() is enabled
> since DataTransfer::getData lets you specify arbitrary mime type for now.

PlatformPasteboard logic is run in the UI process, so it won't be able to check Settings::customPasteboardDataEnabled(). I think we should perform this check earlier, in Pasteboard::getData(), so we return "" for the value of all MIME types that are not safe for DOM read/write. I plan to do this in a followup that also caches the "DOM safe types" in the web process so we don't have to keep asking the platform pasteboard every time DataTransfer.types is invoked.

>> Source/WebCore/platform/mac/PasteboardMac.mm:531
>> +const char* domPasteboardTypeForPlatformType(const String& platformType)
> 
> Looks like this should be a private/protected member.

Yeah -- I made this static in PlatformPasteboard.

>> Source/WebCore/platform/mac/PasteboardMac.mm:587
>> +String platformPasteboardTypeForDOMType(const String& domType)
> 
> Ditto.

(From my previous comment further up)

platformPasteboardTypeForSafeTypeForDOMToReadAndWrite cannot be protected in Pasteboard because it's used in Pasteboard as well as the PlatformPasteboards. However, this did make me realize that there's no need to WEBCORE_EXPORT these. I removed the exports, so at least we can make them only accessible within WebCore.

>> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:125
>> +            if (!coercedType.isEmpty())
> 
> Again, why don't we avoid creating string until we're confirmed it's not empty string?

Fixed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/dump-datatransfer-types.html:66
>> +    "baz/uri-list" : "https://www.webkit.org"
> 
> Again, we need tests with exotic characters in type name.
> Given the bug this patch appears to have (cocoa/DOM type confusion),
> we should probably also test actual cocoa type we don't expose to DOM like public.rtf and public.file-url

This patch tests exotic characters in values (see below), but not types, so I'll add them to the types as well.

Cocoa types are still readable from bindings (both before and after this patch), and this is an issue I will address separately. This patch just makes DataTransfer.types not expose them.
Comment 43 Wenson Hsieh 2017-09-27 10:25:30 PDT
> ...this check earlier, in Pasteboard::getData(), so we return "" for the value...

Sorry, I meant DataTransfer::getData()
Comment 44 Wenson Hsieh 2017-09-27 14:49:36 PDT
Created attachment 322019 [details]
Address more review feedback.
Comment 45 Wenson Hsieh 2017-09-27 14:53:47 PDT
Created attachment 322022 [details]
Address more review feedback, and rebase
Comment 46 Ryosuke Niwa 2017-09-27 15:52:30 PDT
Comment on attachment 322022 [details]
Address more review feedback, and rebase

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

> Source/WebCore/platform/Pasteboard.cpp:49
> +    encoder << data.sameOriginCustomData;

Could you add an empty origin here so that we don't have to change the encoded data format when we add the origin check?
It would be silly having to bump up the version number just to deal with people using spades/STP.

> Source/WebCore/platform/mac/PasteboardMac.mm:584
> +String platformPasteboardTypeForSafeTypeForDOMToReadAndWrite(const String& domType)

Looks like this should just be a public static member function of PlatformPasteboard. It even says PlatformPasteboard in its name!
Comment 47 Wenson Hsieh 2017-09-27 16:31:42 PDT
Comment on attachment 322022 [details]
Address more review feedback, and rebase

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

>> Source/WebCore/platform/Pasteboard.cpp:49
>> +    encoder << data.sameOriginCustomData;
> 
> Could you add an empty origin here so that we don't have to change the encoded data format when we add the origin check?
> It would be silly having to bump up the version number just to deal with people using spades/STP.

Ah, good point -- I hadn't considered combat issues with STP/spades. Added.

>> Source/WebCore/platform/mac/PasteboardMac.mm:584
>> +String platformPasteboardTypeForSafeTypeForDOMToReadAndWrite(const String& domType)
> 
> Looks like this should just be a public static member function of PlatformPasteboard. It even says PlatformPasteboard in its name!

Sounds good!
Comment 48 Wenson Hsieh 2017-09-27 17:41:08 PDT
Created attachment 322047 [details]
Holding for EWS...
Comment 49 Wenson Hsieh 2017-09-27 19:01:50 PDT
Comment on attachment 322047 [details]
Holding for EWS...

It looks like these Mac LayoutTest failures (IndexedDB tests and js/regexp-named-capture-groups.html) aren't relevant.
Comment 50 WebKit Commit Bot 2017-09-27 19:29:21 PDT
Comment on attachment 322047 [details]
Holding for EWS...

Clearing flags on attachment: 322047

Committed r222595: <http://trac.webkit.org/changeset/222595>
Comment 51 Wenson Hsieh 2017-09-27 20:20:52 PDT
Followup build fix: <http://trac.webkit.org/changeset/222596>
Comment 52 Ryosuke Niwa 2017-09-27 21:45:25 PDT
*** Bug 67025 has been marked as a duplicate of this bug. ***
Comment 53 Alexey Proskuryakov 2017-09-28 09:22:09 PDT
> It looks like these Mac LayoutTest failures (IndexedDB tests and js/regexp-named-capture-groups.html) aren't relevant.

Correct, I believe those are related to bug 177423, and should be fixed now.