Add a unit test that filenames aren't unintentionally converted to URLs.

Add a unit test that filenames aren't unintentionally converted to URLs.

Also fixes two issues in OSExchangeDataProviderWin:
- It used a disjoint set of clipboard formats when handling
  GetUrl(..., true /* filename conversion */) vs GetFilenames(...), so the
  actual returned results would vary depending on which one was called.
- It incorrectly used ::DragFinish() instead of ::ReleaseStgMedium().
  ::DragFinish() is only meant to be used in conjunction with WM_DROPFILES.

BUG=346135
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283226

[dcheng, 2014-07-09 00:55:32]

This is a test I should have added when I fixed the original security bug.

[dcheng, 2014-07-09 21:28:31]

Fix Windows

[dcheng, 2014-07-09 21:31:56]

+sky, mind taking a look?

I'm going to be testing locally to make sure it didn't regress anything else by
accident.

[dcheng, 2014-07-10 20:47:06]

On 2014/07/09 at 21:31:56, dcheng (OOO wrote:
> +sky, mind taking a look?
> 
> I'm going to be testing locally to make sure it didn't regress anything else
by accident.

OK, did some local testing, and nothing /appears/ to have regressed. Overall, I
think unifying the Windows codepaths for extracting filenames from
OSExchangeData is a net win.

[sky, 2014-07-10 22:44:34]

https://codereview.chromium.org/380553002/diff/40001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_util_win.cc (right):

https://codereview.chromium.org/380553002/diff/40001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_util_win.cc:143: return url->is_valid();
Under what circumstance would url not be valid here and in other places? This
seems like it should be a DCHECK with a return true.

https://codereview.chromium.org/380553002/diff/40001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_util_win.cc:190: GlobalUnlock(medium.hGlobal);
I don't like having to worry if GlobalUnlock is balanced. How about a scoper?

[dcheng, 2014-07-11 22:32:58]

(I also fixed an incorrect usage of DragFinish instead of ReleaseStgMedium)

https://codereview.chromium.org/380553002/diff/40001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_util_win.cc (right):

https://codereview.chromium.org/380553002/diff/40001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_util_win.cc:143: return url->is_valid();
On 2014/07/10 22:44:33, sky wrote:
> Under what circumstance would url not be valid here and in other places? This
> seems like it should be a DCHECK with a return true.

Other apps may have written a malformed URL to the clipboard.

https://codereview.chromium.org/380553002/diff/40001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_util_win.cc:190: GlobalUnlock(medium.hGlobal);
On 2014/07/10 22:44:34, sky wrote:
> I don't like having to worry if GlobalUnlock is balanced. How about a scoper?

So it turns out ScopedHGlobal is poorly suited for this task. Internally, it
adds a pointer qualifier to the template argument, so it ends up thinking that
its referring to a HDROP*--but HDROP itself is actually already an opaque
handle/pointer to the actual data. I hacked around this for now by
reinterpret_casting the result of get() back to a HDROP but I'm trying to think
of a better solution...

[dcheng, 2014-07-11 22:45:30]

https://codereview.chromium.org/380553002/diff/40001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_util_win.cc (right):

https://codereview.chromium.org/380553002/diff/40001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_util_win.cc:190: GlobalUnlock(medium.hGlobal);
On 2014/07/11 22:32:58, dcheng (OOO) wrote:
> On 2014/07/10 22:44:34, sky wrote:
> > I don't like having to worry if GlobalUnlock is balanced. How about a
scoper?
> 
> So it turns out ScopedHGlobal is poorly suited for this task. Internally, it
> adds a pointer qualifier to the template argument, so it ends up thinking that
> its referring to a HDROP*--but HDROP itself is actually already an opaque
> handle/pointer to the actual data. I hacked around this for now by
> reinterpret_casting the result of get() back to a HDROP but I'm trying to
think
> of a better solution...

Actually I just forced everyone who wants a T* to explicitly say so in the
template argument. PTAL. I also found out something somewhat concerning: some of
the unit tests don't seem to be actually getting built at all:
https://code.google.com/p/chromium/issues/detail?id=393364

[sky, 2014-07-11 22:58:12]

LGTM

[dcheng, 2014-07-11 23:01:50]

+cpu for base/win OWNERS review
+thestig for printing OWNERS review
+garykac for remoting OWNERS review

Note there is an alternative approach--rsleevi@ notes that we could import
add_pointer into base/type_traits.h (C++11 has this natively) and use that to
automatically pointerize only when appropriate. I opted for the explicit
approach in this patch, but am open to feedback.

[garykac, 2014-07-11 23:28:21]

remoting lgtm

[Lei Zhang, 2014-07-11 23:33:05]

printing lgtm

[cpu_(ooo_6.6-7.5), 2014-07-15 00:25:01]

base/win lgtm

[dcheng, 2014-07-15 00:27:05]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-07-15 00:28:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/380553002/100001

[commit-bot: I haz the power, 2014-07-15 16:49:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-15 19:04:41]

Change committed as 283226