REGRESSION (231276): Attempting to copy an image fails

• Component: HTML Editing
• Status: RESOLVED FIXED

[Aditya Keerthi]

r210683 introduced logic to prevent file URLs from being copied to the clipboard in unexpected cases. In order to achieve this functionality, checkURLReceivedFromWebProcess was called on all items in the pathnames array passed into WebPasteboardProxy::setPasteboardPathnamesForType. However, this method is a misnomer, as the pathnames array always contains exactly one URL and one title for the URL.

We should not call checkURLReceivedFromWebProcess on the title and we should refactor the associated methods to avoid referring to multiple pathnames.

[Radar WebKit Bug Importer]

<rdar://problem/41663196>

[Aditya Keerthi]

Created attachment 343988 [details]
Patch

[EWS Watchlist]

Comment on attachment 343988 [details]
Patch

Attachment 343988 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8391722

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 343994 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Aditya Keerthi]

Created attachment 343996 [details]
Patch

[EWS Watchlist]

Comment on attachment 343996 [details]
Patch

Attachment 343996 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8392716

New failing tests:
editing/mac/pasteboard/can-copy-url-without-title.html

[EWS Watchlist]

Created attachment 344001 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[Aditya Keerthi]

The added LayoutTest should be mac-wk2 only.

[EWS Watchlist]

Comment on attachment 343996 [details]
Patch

Attachment 343996 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8392848

New failing tests:
editing/mac/pasteboard/can-copy-url-without-title.html

[EWS Watchlist]

Created attachment 344003 [details]
Archive of layout-test-results from ews113 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 343996 [details]
Patch

Attachment 343996 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8394955

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344012 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Aditya Keerthi]

Created attachment 344043 [details]
Patch

[Darin Adler]

Comment on attachment 344043 [details]
Patch

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

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:311
> +    RetainPtr<NSMutableArray> paths = adoptNS([[NSMutableArray alloc] init]);
> +    NSURL *cocoaURL = pasteboardURL.url;
> +    [paths.get() addObject:[NSArray arrayWithObject:[cocoaURL absoluteString]]];
> +    [paths.get() addObject:[NSArray arrayWithObject:pasteboardURL.title]];

Here’s a way to write this with array literals:

    NSURL *cocoaURL = pasteboardURL.url;
    NSArray *paths = @[ @[ cocoaURL.absoluteString ], @[ pasteboardURL.title ] ];

This has advantages like not creating a mutable array, sizing the array better, and being two lines of code instead of four, but a tiny disadvantage of doing one more autorelease.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:313
> +    NSString *pasteboardType  = [NSString stringWithFormat:@"%s", WebURLsWithTitlesPboardType];

This should be one of these instead:

    NSString *pasteboardType = [NSString stringWithCString:WebURLsWithTitlesPboardType encoding:NSASCIIStringEncoding];
    NSString *pasteboardType = [NSString stringWithUTF8String:WebURLsWithTitlesPboardType];

Using the formatting version instead is less efficient. I also think it’s a bit peculiar that we have constants named WebURLsWithTitlesPboardType in both WebCore and WebKit, but in WebCore it’s a C string whereas in WebKit it’s an NSString. Might be nice to get rid of that pattern some day. If this was already an NSString it would be better for this call site, but I presume it would be worse for other call sites.

[Aditya Keerthi]

Created attachment 344273 [details]
Patch

[EWS Watchlist]

Comment on attachment 344273 [details]
Patch

Attachment 344273 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8436553

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 344286 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Wenson Hsieh]

Comment on attachment 344273 [details]
Patch

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

> LayoutTests/TestExpectations:-201
> -webkit.org/b/186901 imported/w3c/web-platform-tests/css/css-display/display-contents-first-letter-002.html [ Skip ]

Did you intend to skip this test?

[Wenson Hsieh]

Comment on attachment 344273 [details]
Patch

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

>> LayoutTests/TestExpectations:-201
>> -webkit.org/b/186901 imported/w3c/web-platform-tests/css/css-display/display-contents-first-letter-002.html [ Skip ]
> 
> Did you intend to skip this test?

(Unskip, rather)

[Wenson Hsieh]

Comment on attachment 344273 [details]
Patch

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

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:309
> +    NSArray *paths = @[ @[ cocoaURL.absoluteString ], @[ pasteboardURL.title ] ];

Nit - I think calling this variable `paths` is a bit misleading. Perhaps something like `urlAndTitle` instead?

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1671
> +        return false;

Now that PasteboardURL can be sent over IPC, we probably want to ensure that all of its additional platform-specific members (`userVisibleForm` on macOS and `markup` in GTK) are also serialized over IPC.

[Darin Adler]

Comment on attachment 344273 [details]
Patch

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

> Source/WebCore/platform/PlatformPasteboard.h:88
> +    WEBCORE_EXPORT long setPasteboardURL(const PasteboardURL&);

I don’t think having the word "Pasteboard" in this function makes sense. The entire file is about pasteboard, so why have this one function mention Pasteboard

>> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:309
>> +    NSArray *paths = @[ @[ cocoaURL.absoluteString ], @[ pasteboardURL.title ] ];
> 
> Nit - I think calling this variable `paths` is a bit misleading. Perhaps something like `urlAndTitle` instead?

Another way to dodge that strangeness is to call this "array".

[Aditya Keerthi]

(In reply to Darin Adler from comment #21)
> Comment on attachment 344273 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344273&action=review
> 
> > Source/WebCore/platform/PlatformPasteboard.h:88
> > +    WEBCORE_EXPORT long setPasteboardURL(const PasteboardURL&);
> 
> I don’t think having the word "Pasteboard" in this function makes sense. The
> entire file is about pasteboard, so why have this one function mention
> Pasteboard

I named it setPasteboardURL due to the parameter type. I feel that 'setURL' would not be correct as a URL is not the only information that is contained within a PasteboardURL - we also have the title.

[Aditya Keerthi]

Created attachment 344315 [details]
Patch

[EWS Watchlist]

Comment on attachment 344315 [details]
Patch

Attachment 344315 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8443411

New failing tests:
animations/needs-layout.html

[EWS Watchlist]

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

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

[Darin Adler]

Comment on attachment 344273 [details]
Patch

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

>>> Source/WebCore/platform/PlatformPasteboard.h:88
>>> +    WEBCORE_EXPORT long setPasteboardURL(const PasteboardURL&);
>> 
>> I don’t think having the word "Pasteboard" in this function makes sense. The entire file is about pasteboard, so why have this one function mention Pasteboard
> 
> I named it setPasteboardURL due to the parameter type. I feel that 'setURL' would not be correct as a URL is not the only information that is contained within a PasteboardURL - we also have the title.

Sure, but due to overloading C++ functions don’t always have to mention their parameter types. I am not suggesting setURL as the name. In fact there are no other "set" functions in this class, only "set for type" functions. I am not even sure I understand the difference between this function and the write function that takes a PasteboardURL.

[Ryosuke Niwa]

Comment on attachment 344315 [details]
Patch

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

r=me provided all my comments below are addressed.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:306
> +long PlatformPasteboard::setPasteboardURL(const PasteboardURL& pasteboardURL)

Given there is no other function which writes URL, I agree with Darin that don't need Pasteboard in this name.
I'd suggest just calling it setURL instead.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1659
> +void ArgumentCoder<PasteboardURL>::encode(Encoder& encoder, const PasteboardURL& content)

Why are we moving these functions? Please keep them at their original locations so that the change we're making is clear.

> Source/WebKit/Shared/WebCoreArgumentCoders.h:-416
> -template<> struct ArgumentCoder<WebCore::PasteboardURL> {

What's the point of this re-ordering? We should avoid unnecessary code change like this.

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:-123
> -void WebPasteboardProxy::setPasteboardPathnamesForType(IPC::Connection& connection, const String& pasteboardName, const String& pasteboardType, const Vector<String>& pathnames, uint64_t& newChangeCount)

Again, please don't move all these functions.
Make the code change at the location they already exist so that we can see what changed in the diff, not the entire function.

> LayoutTests/platform/mac/TestExpectations:1780
> +webkit.org/b/187230 editing/mac/pasteboard/can-copy-url-without-title.html [ Skip ]

Just add this to LayoutTests/platform/mac-wk1/TestExpectations instead.

[Wenson Hsieh]

Comment on attachment 344315 [details]
Patch

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

>> Source/WebKit/Shared/WebCoreArgumentCoders.h:-416
>> -template<> struct ArgumentCoder<WebCore::PasteboardURL> {
> 
> What's the point of this re-ordering? We should avoid unnecessary code change like this.

This encode/decode logic is currently only PLATFORM(IOS).

The patch moves it out to cross-platform code.

[Aditya Keerthi]

Created attachment 344693 [details]
Patch

[EWS Watchlist]

Comment on attachment 344693 [details]
Patch

Attachment 344693 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8494964

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 344703 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 344693 [details]
Patch

Rejecting attachment 344693 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 344693, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=344693&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=187212&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 344693 from bug 187212.
Fetching: https://bugs.webkit.org/attachment.cgi?id=344693
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 24 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKitLegacy/mac/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/PasteboardStrategy.h
patching file Source/WebCore/platform/PlatformPasteboard.h
patching file Source/WebCore/platform/ios/PlatformPasteboardIOS.mm
patching file Source/WebCore/platform/mac/PasteboardMac.mm
patching file Source/WebCore/platform/mac/PlatformPasteboardMac.mm
patching file Source/WebKit/Shared/WebCoreArgumentCoders.cpp
patching file Source/WebKit/Shared/WebCoreArgumentCoders.h
patching file Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm
patching file Source/WebKit/UIProcess/WebPasteboardProxy.h
patching file Source/WebKit/UIProcess/WebPasteboardProxy.messages.in
patching file Source/WebKit/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp
patching file Source/WebKit/WebProcess/WebCoreSupport/WebPlatformStrategies.h
patching file Source/WebKitLegacy/mac/WebCoreSupport/WebPlatformStrategies.h
patching file Source/WebKitLegacy/mac/WebCoreSupport/WebPlatformStrategies.mm
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/Tests/mac/ContextMenuCanCopyURL.html
patching file Tools/TestWebKitAPI/Tests/mac/ContextMenuCanCopyURL.mm
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/editing/mac/pasteboard/can-copy-url-without-title-expected.txt
patching file LayoutTests/editing/mac/pasteboard/can-copy-url-without-title.html
patching file LayoutTests/platform/mac-wk1/TestExpectations
Hunk #1 FAILED at 564.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk1/TestExpectations.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/8507090

[Ryosuke Niwa]

Committed r233753: <https://trac.webkit.org/changeset/233753>