Bug 36482 - Regression(r55766): [Chromium] chromiumDataObject.hasData() reports true for an empty clipboard
Summary: Regression(r55766): [Chromium] chromiumDataObject.hasData() reports true for ...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Windows Vista
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 36518 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-03-23 00:58 PDT by noel gordon
Modified: 2013-04-11 12:25 PDT (History)
6 users (show)

See Also:


Attachments
patch - check whether new URL is valid/empty (2.13 KB, patch)
2010-03-23 03:14 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch - check whether new URL is valid/empty, fix 2 incorrect usages in ClipboardChromium (3.08 KB, patch)
2010-03-24 01:31 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch - complete rewrite, put code into ChromiumDataObject, retain input uri-list string (18.66 KB, patch)
2010-03-24 03:47 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch - now with less compile errors (18.66 KB, patch)
2010-03-24 19:04 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch - add layout test from #36518 (23.46 KB, patch)
2010-03-24 20:21 PDT, Roland Steiner
jianli: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description noel gordon 2010-03-23 00:58:46 PDT
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.
Comment 1 Tony Chang 2010-03-23 01:02:56 PDT
Might be related to the download url changes (e.g., http://trac.webkit.org/changeset/54469).
Comment 2 Roland Steiner 2010-03-23 03:14:03 PDT
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).
Comment 3 Tony Chang 2010-03-23 17:04:13 PDT
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.
Comment 4 Roland Steiner 2010-03-23 20:53:12 PDT
(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.
Comment 5 Tony Chang 2010-03-23 22:11:44 PDT
(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.
Comment 6 Roland Steiner 2010-03-23 23:57:15 PDT
(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. ;) )
Comment 7 Tony Chang 2010-03-24 00:36:13 PDT
(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.
Comment 8 Roland Steiner 2010-03-24 01:31:21 PDT
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.)
Comment 9 Tony Chang 2010-03-24 01:38:43 PDT
(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?
Comment 10 Roland Steiner 2010-03-24 03:47:15 PDT
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").
Comment 11 WebKit Review Bot 2010-03-24 03:48:37 PDT
Attachment 51487 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1198018
Comment 12 Tony Chang 2010-03-24 17:38:51 PDT
After you fix the compile error, LGTM!
Comment 13 Roland Steiner 2010-03-24 19:04:54 PDT
Created attachment 51579 [details]
patch - now with less compile errors

New patch, new compiling luck...
Comment 14 Roland Steiner 2010-03-24 19:14:40 PDT
*** Bug 36518 has been marked as a duplicate of this bug. ***
Comment 15 Roland Steiner 2010-03-24 20:21:47 PDT
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.
Comment 16 Jian Li 2010-04-27 14:56:08 PDT
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.