Bug 174647 – Don't write file URLs to iOS Pasteboard

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Don't write file URLs to iOS Pasteboard

[Megan Gardner]

Created attachment 315862 [details]
Patch

[Build Bot]

Comment on attachment 315862 [details]
Patch

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

New failing tests:
fast/images/image-copy-memory-usage.html

[Build Bot]

Created attachment 315872 [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.12.5

[Wenson Hsieh]

Comment on attachment 315862 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174647

Please include a radar number here.

> Source/WebCore/editing/ios/EditorIOS.mm:210
> +    if (!url.isLocalFile()) {

Our logic here sets pasteboardImage.url.url to the passed in url, but falls back to imageSourceURL if url is empty, so if url.isEmpty() and imageSourceURL is a file URL, we'll still end up writing a file URL here. We should account for this here.

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174647

Ditto.

> Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:-169
> -    checkTypeIdentifierPrecedesOtherTypeIdentifier(dataInteractionSimulator.get(), (NSString *)kUTTypePNG, (NSString *)kUTTypeFileURL);

We should still check that kUTTypePNG is in the item provider, and then additionally check that kUTTypeFileURL is not in the item provider.

> Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:-189
> -    checkTypeIdentifierPrecedesOtherTypeIdentifier(dataInteractionSimulator.get(), (NSString *)kUTTypePNG, (NSString *)kUTTypeFileURL);

Ditto.

> Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:-994
> -    checkTypeIdentifierPrecedesOtherTypeIdentifier(dataInteractionSimulator.get(), (NSString *)kUTTypePNG, (NSString *)kUTTypeFileURL);

Ditto.

[Wenson Hsieh]

Comment on attachment 315862 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Tests updated to reflect the changes.

I would also give a very brief recap of the change in Editor here (maybe mention that this impacts both DnD and copy/paste).

[Megan Gardner]

Created attachment 315938 [details]
Patch

[Wenson Hsieh]

Comment on attachment 315938 [details]
Patch

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

LGTM, but I'm a bit concerned about the failing test (fast/images/image-copy-memory-usage.html). It does seem to exercise this Editor::writeImageToPasteboard codepath. Let's see if this test is actually catching anything useful, and if we just need to rebaseline it.

> Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:122
> +    EXPECT_TRUE([registeredTypes indexOfObject:firstType] < [registeredTypes indexOfObject:secondType]);

We don't need this last check here, since [registeredTypes indexOfObject:secondType] should be NSNotFound anyways.

[Wenson Hsieh]

> LGTM, but I'm a bit concerned about the failing test
> (fast/images/image-copy-memory-usage.html). It does seem to exercise this
> Editor::writeImageToPasteboard codepath. Let's see if this test is actually
> catching anything useful, and if we just need to rebaseline it.

Oh, after actually looking at the LayoutTest failure logs, you'll probably need to update PlatformPasteboard::write() as well to not crash when pasteboardImage.url.url is empty.

[Build Bot]

Comment on attachment 315938 [details]
Patch

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

New failing tests:
fast/images/image-copy-memory-usage.html

[Build Bot]

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

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

[Megan Gardner]

Created attachment 315951 [details]
Patch

[Wenson Hsieh]

Comment on attachment 315951 [details]
Patch

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

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:328
> +        if (!pasteboardImage.url.url.isNull())

This should probably be an isEmpty() check, which encompasses isNull()

> Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:122
> +    EXPECT_TRUE([registeredTypes indexOfObject:firstType] < [registeredTypes indexOfObject:secondType]);

We don't need this last check here, since [registeredTypes indexOfObject:secondType] should be NSNotFound anyways.

[Megan Gardner]

Created attachment 315952 [details]
Patch

[Wenson Hsieh]

> > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:328
> > +        if (!pasteboardImage.url.url.isNull())
> 
> This should probably be an isEmpty() check, which encompasses isNull()

On second thought, it looks like isNull() is OK here (a URL created with an empty string is converted to a non-null NSURL, which will be OK to add as a value to an NSDictionary).

[Megan Gardner]

https://trac.webkit.org/changeset/219666/webkit