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

ui/base/clipboard/clipboard_util_win.cc

Index: ui/base/clipboard/clipboard_util_win.cc
diff --git a/ui/base/clipboard/clipboard_util_win.cc b/ui/base/clipboard/clipboard_util_win.cc
index aa61e6a43d60cc1de330524c2d231c4d1bca3502..0da308d3b1e3e1992255381e69bc51886583e5c6 100644
--- a/ui/base/clipboard/clipboard_util_win.cc
+++ b/ui/base/clipboard/clipboard_util_win.cc
@@ -12,10 +12,13 @@
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/scoped_hglobal.h"
+#include "net/base/filename_util.h"
 #include "ui/base/clipboard/clipboard.h"
 #include "ui/base/clipboard/custom_data_helper.h"
+#include "url/gurl.h"
 
 namespace ui {
 
@@ -34,7 +37,7 @@ bool GetData(IDataObject* data_object,
 }
 
 bool GetUrlFromHDrop(IDataObject* data_object,
-                     base::string16* url,
+                     GURL* url,
                      base::string16* title) {
   DCHECK(data_object && url && title);
 
@@ -54,10 +57,10 @@ bool GetUrlFromHDrop(IDataObject* data_object,
     if (0 == _wcsicmp(PathFindExtensionW(filename), L".url") &&
         GetPrivateProfileStringW(L"InternetShortcut", L"url", 0, url_buffer,
                                  arraysize(url_buffer), filename)) {
-      url->assign(url_buffer);
+      *url = GURL(url_buffer);
       PathRemoveExtension(filename);
       title->assign(PathFindFileName(filename));
-      success = true;
+      success = url->is_valid();
     }
   }
 
@@ -69,68 +72,19 @@ bool GetUrlFromHDrop(IDataObject* data_object,
 }
 
 void SplitUrlAndTitle(const base::string16& str,
-                      base::string16* url,
+                      GURL* url,
                       base::string16* title) {
   DCHECK(url && title);
   size_t newline_pos = str.find('\n');
   if (newline_pos != base::string16::npos) {
-    url->assign(str, 0, newline_pos);
+    *url = GURL(base::string16(str, 0, newline_pos));
     title->assign(str, newline_pos + 1, base::string16::npos);
   } else {
-    url->assign(str);
+    *url = GURL(str);
     title->assign(str);
   }
 }
 
-bool GetFileUrl(IDataObject* data_object, base::string16* url,
-                base::string16* title) {
-  STGMEDIUM store;
-  if (GetData(data_object, Clipboard::GetFilenameWFormatType(), &store)) {
-    bool success = false;
-    {
-      // filename using unicode
-      base::win::ScopedHGlobal<wchar_t> data(store.hGlobal);
-      if (data.get() && data.get()[0] &&
-          (PathFileExists(data.get()) || PathIsUNC(data.get()))) {
-        wchar_t file_url[INTERNET_MAX_URL_LENGTH];
-        DWORD file_url_len = arraysize(file_url);
-        if (SUCCEEDED(::UrlCreateFromPathW(data.get(), file_url, &file_url_len,
-                                           0))) {
-          url->assign(file_url);
-          title->assign(file_url);
-          success = true;
-        }
-      }
-    }
-    ReleaseStgMedium(&store);
-    if (success)
-      return true;
-  }
-
-  if (GetData(data_object, Clipboard::GetFilenameFormatType(), &store)) {
-    bool success = false;
-    {
-      // filename using ascii
-      base::win::ScopedHGlobal<char> data(store.hGlobal);
-      if (data.get() && data.get()[0] && (PathFileExistsA(data.get()) ||
-                                          PathIsUNCA(data.get()))) {
-        char file_url[INTERNET_MAX_URL_LENGTH];
-        DWORD file_url_len = arraysize(file_url);
-        if (SUCCEEDED(::UrlCreateFromPathA(data.get(), file_url, &file_url_len,
-                                           0))) {
-          url->assign(base::UTF8ToWide(file_url));
-          title->assign(*url);
-          success = true;
-        }
-      }
-    }
-    ReleaseStgMedium(&store);
-    if (success)
-      return true;
-  }
-  return false;
-}
-
 }  // namespace
 
 bool ClipboardUtil::HasUrl(IDataObject* data_object, bool convert_filenames) {
@@ -138,14 +92,14 @@ bool ClipboardUtil::HasUrl(IDataObject* data_object, bool convert_filenames) {
   return HasData(data_object, Clipboard::GetMozUrlFormatType()) ||
          HasData(data_object, Clipboard::GetUrlWFormatType()) ||
          HasData(data_object, Clipboard::GetUrlFormatType()) ||
-         (convert_filenames && (
-             HasData(data_object, Clipboard::GetFilenameWFormatType()) ||
-             HasData(data_object, Clipboard::GetFilenameFormatType())));
+         (convert_filenames && HasFilenames(data_object));
 }
 
 bool ClipboardUtil::HasFilenames(IDataObject* data_object) {
   DCHECK(data_object);
-  return HasData(data_object, Clipboard::GetCFHDropFormatType());
+  return HasData(data_object, Clipboard::GetCFHDropFormatType()) ||
+         HasData(data_object, Clipboard::GetFilenameWFormatType()) ||
+         HasData(data_object, Clipboard::GetFilenameFormatType());
 }
 
 bool ClipboardUtil::HasFileContents(IDataObject* data_object) {
@@ -166,7 +120,9 @@ bool ClipboardUtil::HasPlainText(IDataObject* data_object) {
 }
 
 bool ClipboardUtil::GetUrl(IDataObject* data_object,
-    base::string16* url, base::string16* title, bool convert_filenames) {
+                           GURL* url,
+                           base::string16* title,
+                           bool convert_filenames) {
   DCHECK(data_object && url && title);
   if (!HasUrl(data_object, convert_filenames))
     return false;
@@ -184,7 +140,7 @@ bool ClipboardUtil::GetUrl(IDataObject* data_object,
       SplitUrlAndTitle(data.get(), url, title);
     }
     ReleaseStgMedium(&store);
-    return true;
+    return url->is_valid();

[sky, 2014/07/10 22:44:33]

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.

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

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

   }
 
   if (GetData(data_object, Clipboard::GetUrlFormatType(), &store)) {
@@ -194,14 +150,19 @@ bool ClipboardUtil::GetUrl(IDataObject* data_object,
       SplitUrlAndTitle(base::UTF8ToWide(data.get()), url, title);
     }
     ReleaseStgMedium(&store);
-    return true;
+    return url->is_valid();
   }
 
   if (convert_filenames) {
-    return GetFileUrl(data_object, url, title);
-  } else {
-    return false;
+    std::vector<base::string16> filenames;
+    if (!GetFilenames(data_object, &filenames))
+      return false;
+    DCHECK_GT(filenames.size(), 0U);
+    *url = net::FilePathToFileURL(base::FilePath(filenames[0]));
+    return url->is_valid();
   }
+
+  return false;
 }
 
 bool ClipboardUtil::GetFilenames(IDataObject* data_object,
@@ -211,27 +172,50 @@ bool ClipboardUtil::GetFilenames(IDataObject* data_object,
     return false;
 
   STGMEDIUM medium;
-  if (!GetData(data_object, Clipboard::GetCFHDropFormatType(), &medium))
-    return false;
+  if (GetData(data_object, Clipboard::GetCFHDropFormatType(), &medium)) {
+    HDROP hdrop = static_cast<HDROP>(GlobalLock(medium.hGlobal));
+    if (!hdrop)
+      return false;
+
+    const int kMaxFilenameLen = 4096;
+    const unsigned num_files = DragQueryFileW(hdrop, 0xffffffff, 0, 0);
+    for (unsigned int i = 0; i < num_files; ++i) {
+      wchar_t filename[kMaxFilenameLen];
+      if (!DragQueryFileW(hdrop, i, filename, kMaxFilenameLen))
+        continue;
+      filenames->push_back(filename);
+    }
 
-  HDROP hdrop = static_cast<HDROP>(GlobalLock(medium.hGlobal));
-  if (!hdrop)
-    return false;
+    DragFinish(hdrop);
+    GlobalUnlock(medium.hGlobal);

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

I don't like having to worry if GlobalUnlock is balanced. How about a scoper?

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

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]

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

+    // We don't need to call ReleaseStgMedium here because as far as I can tell,
+    // DragFinish frees the hGlobal for us.
+    return true;
+  }
 
-  const int kMaxFilenameLen = 4096;
-  const unsigned num_files = DragQueryFileW(hdrop, 0xffffffff, 0, 0);
-  for (unsigned int i = 0; i < num_files; ++i) {
-    wchar_t filename[kMaxFilenameLen];
-    if (!DragQueryFileW(hdrop, i, filename, kMaxFilenameLen))
-      continue;
-    filenames->push_back(filename);
+  if (GetData(data_object, Clipboard::GetFilenameWFormatType(), &medium)) {
+    {
+      // filename using unicode
+      base::win::ScopedHGlobal<wchar_t> data(medium.hGlobal);
+      if (data.get() && data.get()[0])
+        filenames->push_back(data.get());
+    }
+    ReleaseStgMedium(&medium);
+    return true;
   }
 
-  DragFinish(hdrop);
-  GlobalUnlock(medium.hGlobal);
-  // We don't need to call ReleaseStgMedium here because as far as I can tell,
-  // DragFinish frees the hGlobal for us.
-  return true;
+  if (GetData(data_object, Clipboard::GetFilenameFormatType(), &medium)) {
+    {
+      // filename using ascii
+      base::win::ScopedHGlobal<char> data(medium.hGlobal);
+      if (data.get() && data.get()[0])
+        filenames->push_back(base::SysNativeMBToWide(data.get()));
+    }
+    ReleaseStgMedium(&medium);
+    return true;
+  }
+
+  return false;
 }
 
 bool ClipboardUtil::GetPlainText(IDataObject* data_object,
@@ -263,8 +247,13 @@ bool ClipboardUtil::GetPlainText(IDataObject* data_object,
 
   // If a file is dropped on the window, it does not provide either of the
   // plain text formats, so here we try to forcibly get a url.
+  GURL url;
   base::string16 title;
-  return GetUrl(data_object, plain_text, &title, false);
+  if (GetUrl(data_object, &url, &title, false)) {
+    *plain_text = base::UTF8ToUTF16(url.spec());
+    return true;
+  }
+  return false;
 }
 
 bool ClipboardUtil::GetHtml(IDataObject* data_object,