Bug 190530 - [Cocoa] Attachment dropped from one web view to another is missing its file wrapper
Summary: [Cocoa] Attachment dropped from one web view to another is missing its file w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
: 188904 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-10-12 11:32 PDT by mitz
Modified: 2019-04-06 19:34 PDT (History)
6 users (show)

See Also:

Attachments
Patch (44.62 KB, patch)
2018-10-30 14:45 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Try to fix macOS builds (44.62 KB, patch)
2018-10-30 15:28 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Try to fix the Windows build (44.89 KB, patch)
2018-10-30 16:45 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Try to fix the Windows build (2) (44.91 KB, patch)
2018-10-30 19:10 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Try to fix the Windows build (3) (45.06 KB, patch)
2018-10-30 19:52 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Try to fix the Windows build (4) (45.07 KB, patch)
2018-10-30 20:11 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Try to fix the Windows build (4.1) (45.12 KB, patch)
2018-10-30 20:23 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Try to fix the Windows build (5) (45.09 KB, patch)
2018-10-30 20:34 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2018-10-12 11:32:06 PDT 
To reproduce this bug:

1. Create two editable, attachment-enabled web views
2. Drop a file into the first web view, thus creating an attachment
3. Drag the attachment from the first web view to the other one

Notice that the _WKAttachment instance passed to -_webView:didInsertAttachment:withSource: has a nil info.fileWrapper.
Comment 1 mitz 2018-10-12 11:38:26 PDT 
<rdar://problem/45232149>
Comment 2 mitz 2018-10-12 12:41:32 PDT 
Seems to also happen with copy and paste!
Comment 3 Wenson Hsieh 2018-10-30 14:45:53 PDT 
Created attachment 353411 [details]
Patch
Comment 4 Wenson Hsieh 2018-10-30 15:28:30 PDT 
Created attachment 353417 [details]
Try to fix macOS builds
Comment 5 Wenson Hsieh 2018-10-30 16:45:57 PDT 
Created attachment 353435 [details]
Try to fix the Windows build
Comment 6 Tim Horton 2018-10-30 16:56:09 PDT 
Comment on attachment 353435 [details]
Try to fix the Windows build

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

> Source/WebCore/html/HTMLAttachmentElement.cpp:91
> +    return URLParser(makeString("applewebdata://attachment/"_s, identifier)).result();

It feels weird/scary to build a URL by concatenating strings. But it seems like we don't have a mutable URL? What happens if identifier (which comes from where?) has non-path-safe characters?
Comment 7 Wenson Hsieh 2018-10-30 17:42:51 PDT 
Comment on attachment 353435 [details]
Try to fix the Windows build

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

>> Source/WebCore/html/HTMLAttachmentElement.cpp:91
>> +    return URLParser(makeString("applewebdata://attachment/"_s, identifier)).result();
> 
> It feels weird/scary to build a URL by concatenating strings. But it seems like we don't have a mutable URL? What happens if identifier (which comes from where?) has non-path-safe characters?

Good point — the identifier _normally_ comes from within WebKit (createCanonicalUUIDString()) and it's only exposed as a readonly attribute to internal clients. That being said, we do have a mechanism for temporarily saving the identifier as an attribute on the attachment element (see "webkitattachmentid"). Using this, it should be possible to influence the identifier of a pasted/dropped attachment element by writing a webarchive to the pasteboard with an attachment element that contains this attribute...

I've changed this to use URL::setPath() to set the path of the URL, and verified that non-path-safe characters (e.g. space) are URL-encoded.
Comment 8 Wenson Hsieh 2018-10-30 19:10:00 PDT 
Created attachment 353456 [details]
Try to fix the Windows build (2)
Comment 9 Wenson Hsieh 2018-10-30 19:52:26 PDT 
Created attachment 353458 [details]
Try to fix the Windows build (3)
Comment 10 Wenson Hsieh 2018-10-30 20:11:31 PDT 
Created attachment 353459 [details]
Try to fix the Windows build (4)
Comment 11 Wenson Hsieh 2018-10-30 20:23:42 PDT 
Created attachment 353460 [details]
Try to fix the Windows build (4.1)
Comment 12 Wenson Hsieh 2018-10-30 20:34:16 PDT 
Created attachment 353461 [details]
Try to fix the Windows build (5)
Comment 13 WebKit Commit Bot 2018-10-30 22:29:26 PDT 
Comment on attachment 353461 [details]
Try to fix the Windows build (5)

Clearing flags on attachment: 353461

Committed r237624: <https://trac.webkit.org/changeset/237624>
Comment 14 WebKit Commit Bot 2018-10-30 22:29:28 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 mitz 2019-04-06 19:34:18 PDT 
*** Bug 188904 has been marked as a duplicate of this bug. ***