Bug 142801 – <attachment> should put URLs on the pasteboard so that Finder can accept drops

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

Tracks the work required to be able to drag an attachment element out of a WebKit view and into a different target.

rdar://problem/19982527

[Enrica Casucci]

Created attachment 248886 [details]
Patch

[WebKit Commit Bot]

Attachment 248886 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DragController.cpp:51:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Enrica Casucci]

I fixed the include order in DragController.cpp after posting the patch.

[Tim Horton]

Comment on attachment 248886 [details]
Patch

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

> Source/WebCore/page/DragController.cpp:629
> +    // Attachment elements can have DragSourceActionNone even if
> +    // selected, if the selection contains only the attachement element.

This is not a 'why?' comment, but I wish it were.

> Source/WebCore/page/DragController.cpp:759
> +    m_draggingAttachmentURL = URL();

Vaguely interesting that you reset m_draggingAttachmentURL here, but this isn't where m_draggingImageURL is reset. But maybe there's a reason!

> Source/WebCore/rendering/HitTestResult.cpp:330
> +    return m_innerNonSharedNode->document().completeURL(attachmentFile->path());

This doesn't look right... is it? Is the file relative to the document's URL? Why are we completeURLing?

> Source/WebKit2/UIProcess/mac/PageClientImpl.mm:410
>  void PageClientImpl::setPromisedData(const String& pasteboardName, PassRefPtr<SharedBuffer> imageBuffer, const String& filename, const String& extension, const String& title, const String& url, const String& visibleUrl, PassRefPtr<SharedBuffer> archiveBuffer)

It's pretty weird that this function takes an ImageBuffer and we call it even in the non-image case. Maybe it needs a rename and duplication?

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:413
> +    RefPtr<SharedMemory> sharedMemoryImage = (!imageHandle.isNull()) ? SharedMemory::create(imageHandle, SharedMemory::ReadOnly) : nullptr;

Ditto here, I think this should be written differently. Maybe split into setPromisedData and setPromisedImage (and setPromisedImage calls setPromisedData after extracting the data?)

> Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:275
> +    NSMutableArray *types = [[NSMutableArray alloc] initWithObjects:NSFilesPromisePboardType, nil];

Isn't this leaking?

> Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:292
> +            [paths.get() addObject:title];

no .get()

[Enrica Casucci]

Created attachment 248893 [details]
Patch that fixes builds and style

[Enrica Casucci]

(In reply to comment #4)
> Comment on attachment 248886 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248886&action=review
> 
> > Source/WebCore/page/DragController.cpp:629
> > +    // Attachment elements can have DragSourceActionNone even if
> > +    // selected, if the selection contains only the attachement element.
> 
> This is not a 'why?' comment, but I wish it were.
I don't understand your comment. This is exactly what we do. What am I missing?
> 
> > Source/WebCore/page/DragController.cpp:759
> > +    m_draggingAttachmentURL = URL();
> 
> Vaguely interesting that you reset m_draggingAttachmentURL here, but this
> isn't where m_draggingImageURL is reset. But maybe there's a reason!
Moved it up simply not to add another #if ENABLE(ATTACHMENT_ELEMENT)
> 
> > Source/WebCore/rendering/HitTestResult.cpp:330
> > +    return m_innerNonSharedNode->document().completeURL(attachmentFile->path());
> 
> This doesn't look right... is it? Is the file relative to the document's
> URL? Why are we completeURLing?
I believe you are right, copy paste mistake.
> 
> > Source/WebKit2/UIProcess/mac/PageClientImpl.mm:410
> >  void PageClientImpl::setPromisedData(const String& pasteboardName, PassRefPtr<SharedBuffer> imageBuffer, const String& filename, const String& extension, const String& title, const String& url, const String& visibleUrl, PassRefPtr<SharedBuffer> archiveBuffer)
> 
> It's pretty weird that this function takes an ImageBuffer and we call it
> even in the non-image case. Maybe it needs a rename and duplication?
> 
> > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:413
> > +    RefPtr<SharedMemory> sharedMemoryImage = (!imageHandle.isNull()) ? SharedMemory::create(imageHandle, SharedMemory::ReadOnly) : nullptr;
> 
> Ditto here, I think this should be written differently. Maybe split into
> setPromisedData and setPromisedImage (and setPromisedImage calls
> setPromisedData after extracting the data?)
>
I was not entirely convinced to duplicate the code, but maybe I should...
 
> > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:275
> > +    NSMutableArray *types = [[NSMutableArray alloc] initWithObjects:NSFilesPromisePboardType, nil];
> 
> Isn't this leaking?
I only moved up an existing line. I've noticed that in many places where declareTypes is called we don't worry about the array we pass.
> 
> > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:292
> > +            [paths.get() addObject:title];
> 
> no .get()
ok.

[Tim Horton]

(In reply to comment #6)
> (In reply to comment #4)
> > Comment on attachment 248886 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=248886&action=review
> > 
> > > Source/WebCore/page/DragController.cpp:629
> > > +    // Attachment elements can have DragSourceActionNone even if
> > > +    // selected, if the selection contains only the attachement element.
> > 
> > This is not a 'why?' comment, but I wish it were.
> I don't understand your comment. This is exactly what we do. What am I
> missing?

Exactly. The comment is describing what the code does, but not why it does that. If I wanted to know what it does, I would read the code. If I wanted to know why it does it... I would come ask you, because the comment doesn't help me.

> > 
> > > Source/WebCore/page/DragController.cpp:759
> > > +    m_draggingAttachmentURL = URL();
> > 
> > Vaguely interesting that you reset m_draggingAttachmentURL here, but this
> > isn't where m_draggingImageURL is reset. But maybe there's a reason!
> Moved it up simply not to add another #if ENABLE(ATTACHMENT_ELEMENT)

Ah! OK.

> > > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:413
> > > +    RefPtr<SharedMemory> sharedMemoryImage = (!imageHandle.isNull()) ? SharedMemory::create(imageHandle, SharedMemory::ReadOnly) : nullptr;
> > 
> > Ditto here, I think this should be written differently. Maybe split into
> > setPromisedData and setPromisedImage (and setPromisedImage calls
> > setPromisedData after extracting the data?)
> >
> I was not entirely convinced to duplicate the code, but maybe I should...

It doesn't need to be much duplication (I think!) if you have setPromisedImage extract the image data and then just go ahead and call setPromisedData.

> > > Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:275
> > > +    NSMutableArray *types = [[NSMutableArray alloc] initWithObjects:NSFilesPromisePboardType, nil];
> > 
> > Isn't this leaking?
> I only moved up an existing line. I've noticed that in many places where
> declareTypes is called we don't worry about the array we pass.

Oh! No, there's an explicit -release sent there (it's now far away from the alloc/init, so I missed it).

This should probably use RetainPtr, but I don't care if you fix it in this patch.

[Alexey Proskuryakov]

Does this patch introduce sandbox escape opportunities? Can malicious native code running inside WebContent process fool UI process into giving it full file system access?

Files inside drag normally result in getting such access.

[Enrica Casucci]

Created attachment 248979 [details]
Patch3

This patch addresses the comments and the security concerns.

[Enrica Casucci]

Created attachment 248983 [details]
Patch4

Trying to make EWS happy.

[WebKit Commit Bot]

Attachment 248983 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/PageClient.h:198:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 248983 [details]
Patch4

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

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:105
> +    NSURL* nsURL = (NSURL *)url;

one of the stars is on the wrong side

> Source/WebKit/mac/Misc/WebNSPasteboardExtras.mm:292
> +            [paths.get() addObject:title];

no .get()

> LayoutTests/ChangeLog:13
> +<<<<<<< .mine

Something bad happened here.

> LayoutTests/editing/pasteboard/drag-and-drop-attachment-contenteditable.html:1
> +

blank line?

> LayoutTests/editing/pasteboard/drag-and-drop-attachment-contenteditable.html:26
> +   shouldBe('target.getElementsByTagName("attachment").length', '1');

indentation error

> LayoutTests/editing/pasteboard/drag-and-drop-attachment-contenteditable.html:32
> +{

this { is inconsistent with the others.

[Enrica Casucci]

Committed revision 181760.