Bug 36482 – Regression(r55766): [Chromium] chromiumDataObject.hasData() reports true for an empty clipboard

• Component: Platform
• Status: RESOLVED WONTFIX

[noel gordon]

Drag a virtual system object such as the "Recycle Bin" onto a web page,
there is nothing on the clipboard in this case, however, routine 

  chromiumDataObject.hasData()

returns true.  For some reason, the uriList member is non empty.

[Tony Chang]

Might be related to the download url changes (e.g., http://trac.webkit.org/changeset/54469).

[Roland Steiner]

Created attachment 51403 [details]
patch - check whether new URL is valid/empty

Simple patch that fixes the bug with the empty string in the uriList member: resets both url and uriList if the passed-in URL is empty (or invalid).

[Tony Chang]

Comment on attachment 51403 [details]
patch - check whether new URL is valid/empty

> +        No new tests. (simple code change)

Is it possible to write a layout test for this?  Would be nice to make sure we don't regress in the future.

> +void ChromiumDataObject::setURL(const KURL& newURL)
> +{
> +    ASSERT(newURL.isValid() || newURL.isEmpty());

Do we really want to assert here?  Can't javascript try to set an invalid URL?  We shouldn't assert in that case.

> +    uriList.clear();
> +    if (newURL.isValid()) {

What about the constructor?  It doesn't go through this code path.

[Roland Steiner]

(In reply to comment #3)
> (From update of attachment 51403 [details])
> > +        No new tests. (simple code change)
> 
> Is it possible to write a layout test for this?  Would be nice to make sure we
> don't regress in the future.

This is hit in chrome/src/webkit/glue/webdropdata.cc, during the conversion from WebDropData to WebDragData. This doesn't seem to be used in the layout test shell.
So I'm not sure there is a easy way to test this (except adding a spurious copy operation somewhere), and adding an interface seemed overkill for such a small change.

> > +void ChromiumDataObject::setURL(const KURL& newURL)
> > +{
> > +    ASSERT(newURL.isValid() || newURL.isEmpty());
> 
> Do we really want to assert here?  Can't javascript try to set an invalid URL? 
> We shouldn't assert in that case.

This should not be exposed to JavaScript directly - for JS, sanitizing is done in ClipboardChromium::setData().

Conceivably, there may be an invalid URL passed-in on the clipboard from an external process, but again I think it'd be better if we sanitized this before accepting it. Hence the ASSERT.

However, I don't feel strongly about this and could be convinced that we want to accept invalid URLs in the clipboard data, and remove the ASSERT.
 
> > +    uriList.clear();
> > +    if (newURL.isValid()) {
> 
> What about the constructor?  It doesn't go through this code path.

There is only a default and a copy constructor. For the first there is no issue, and for the second it's assumed the data in the copied ChromiumDataObject is already legal.

[Tony Chang]

(In reply to comment #4)
> So I'm not sure there is a easy way to test this (except adding a spurious copy
> operation somewhere), and adding an interface seemed overkill for such a small
> change.

I guess I'm a bit confused.  There are layout tests that test dragging URLs in a page (e.g., fast/events/drag-in-frames.html).  Do they go through this code path?

> > > +void ChromiumDataObject::setURL(const KURL& newURL)
> > > +{
> > > +    ASSERT(newURL.isValid() || newURL.isEmpty());
> > 
> > Do we really want to assert here?  Can't javascript try to set an invalid URL? 
> > We shouldn't assert in that case.
> 
> This should not be exposed to JavaScript directly - for JS, sanitizing is done
> in ClipboardChromium::setData().
> 
> Conceivably, there may be an invalid URL passed-in on the clipboard from an
> external process, but again I think it'd be better if we sanitized this before
> accepting it. Hence the ASSERT.
> 
> However, I don't feel strongly about this and could be convinced that we want
> to accept invalid URLs in the clipboard data, and remove the ASSERT.

I agree that we should sanitize the data.  Isn't that what the if statement after the assert does?  It seems like we shouldn't crash in debug because the user dragged the recycling bin onto chrome.

[Roland Steiner]

(In reply to comment #5)
> I guess I'm a bit confused.  There are layout tests that test dragging URLs in
> a page (e.g., fast/events/drag-in-frames.html).  Do they go through this code
> path?

AFAICT the only place where it is called is in the copying code within webdropdata.cc I mentioned. I.e., WebDropTarget::OnDragEnter creates a WebDropData from the Windows clipboard and forwards it via IPC. On the other side, in RenderView::OnDragTargetDragEnter(), it is immediately converted to a WebDragData object, and during that conversion setURL(url) is called.

This call is made whether or not there is an original URL, which triggered the bug. The change I made is just to clear the url and uriList members if the input is empty.

As this is used in IPC code, I'm not sure what would be the best way to write a test for it in the test shell.

Aside: Note that using this method one cannot distinguish between "set nothing" and "set an empty url", but I guess this is inherent, and the latter is not really needed for this.

> I agree that we should sanitize the data.  Isn't that what the if statement
> after the assert does?  It seems like we shouldn't crash in debug because the
> user dragged the recycling bin onto chrome.

When dragging the recycle bin, newURL.isEmpty() is true, so the ASSERT doesn't trigger. (It's for this case that I made the change after all. ;) )

[Tony Chang]

(In reply to comment #6)
> (In reply to comment #5)
> > I guess I'm a bit confused.  There are layout tests that test dragging URLs in
> > a page (e.g., fast/events/drag-in-frames.html).  Do they go through this code
> > path?
> 
> AFAICT the only place where it is called is in the copying code within
> webdropdata.cc I mentioned. I.e., WebDropTarget::OnDragEnter creates a
> WebDropData from the Windows clipboard and forwards it via IPC. On the other
> side, in RenderView::OnDragTargetDragEnter(), it is immediately converted to a
> WebDragData object, and during that conversion setURL(url) is called.

Aha, found it.  ChromiumClipboard is a friend class of ChromiumDataObject and it sets url directly in a lot of places (declareAndWriteDragImage, writeURL, etc).  I guess they should no longer be friends?  That causes this check to be missed most of the time.

> > I agree that we should sanitize the data.  Isn't that what the if statement
> > after the assert does?  It seems like we shouldn't crash in debug because the
> > user dragged the recycling bin onto chrome.
> 
> When dragging the recycle bin, newURL.isEmpty() is true, so the ASSERT doesn't
> trigger. (It's for this case that I made the change after all. ;) )

Ah I see.  Sorry I misunderstood the ASSERT, it seems fine.

[Roland Steiner]

Created attachment 51485 [details]
patch - check whether new URL is valid/empty, fix 2 incorrect usages in ClipboardChromium

> Aha, found it.  ChromiumClipboard is a friend class of ChromiumDataObject and
> it sets url directly in a lot of places (declareAndWriteDragImage, writeURL,
> etc).  I guess they should no longer be friends?  That causes this check to be
> missed most of the time.

The friend clause was actually by design, so that ClipboardChromium (and only ClipboardChromium) can populate a ChromiumDataObject without too much fuss. But you're right, the last 2 places (declareAndWriteDragImage, writeURL) are incorrect. That's actually a bug from an earlier patch, but I can just as well fix it here.

Uploaded new patch that fixes the incorrect usage.

(Aside: Note that this code may well be quite short-lived once Daniel Cheng finishes his general MIME-support refactoring.)

[Tony Chang]

(In reply to comment #8)
> The friend clause was actually by design, so that ClipboardChromium (and only
> ClipboardChromium) can populate a ChromiumDataObject without too much fuss.

It looks like everything is public other than url and uriList.  I think you can
remove the friend declaration.

> But
> you're right, the last 2 places (declareAndWriteDragImage, writeURL) are
> incorrect. That's actually a bug from an earlier patch, but I can just as well
> fix it here.

There's also assignment to url in setData (two places).  I think we want to use
setURL there too.

Now that drag and drop is going through this code, can we write a layout test
for it?

[Roland Steiner]

Created attachment 51487 [details]
patch - complete rewrite, put code into ChromiumDataObject, retain input uri-list string

I guess it's better to re-write the code after all:

.) Put the processing code into ChromiumDataObject, removed the friend clause, replaced the code in ClipboardChromium with one-liners.
.) Added a layout test (skipped on all platforms besides Chromium)

This change also goes towards MIME-type handling, in that the input uri-list is no longer stored as a vector, but as a simple string.
It is returned verbatim on getData("text/uri-list"). The code still extracts the first valid URL within that string (may be empty) and will return that on getData("URL").

[WebKit Review Bot]

Attachment 51487 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1198018

[Tony Chang]

After you fix the compile error, LGTM!

[Roland Steiner]

Created attachment 51579 [details]
patch - now with less compile errors

New patch, new compiling luck...

[Roland Steiner]

*** Bug 36518 has been marked as a duplicate of this bug. ***

[Roland Steiner]

Created attachment 51584 [details]
patch - add layout test from #36518

Since this new version subsumes functionality from the patch in (the now obsolete) issue 36518, I added that layout test modification to this patch. No change to the actual code.

[Jian Li]

Comment on attachment 51584 [details]
patch - add layout test from #36518

What you have done in this bug, specially ChromiumDataObject::setURL, seems to have the same effect as bug 38159 (https://bugs.webkit.org/show_bug.cgi?id=38159). Could you please sync and resubmit your patch with the change from 38159 merged? Thanks.

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=230511