Experimentally isolate GetOpenFileName in a utility process.

GetOpenFileName can cause 3rd-party shell extensions to be loaded into the caller's process. By putting it in a separate process we protect the browser process from potential instability.

BUG=73098

Review URL: https://codereview.chromium.org/419523006

Cr-Commit-Position: refs/heads/master@{#289625}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289625 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chrome_browser_main.cc

@@ -165,6 +165,7 @@
 #include "base/win/windows_version.h"
 #include "chrome/browser/browser_util_win.h"
 #include "chrome/browser/chrome_browser_main_win.h"
+#include "chrome/browser/chrome_select_file_dialog_factory_win.h"
 #include "chrome/browser/component_updater/sw_reporter_installer_win.h"
 #include "chrome/browser/first_run/try_chrome_dialog_view.h"
 #include "chrome/browser/first_run/upgrade_util_win.h"
@@ -175,6 +176,7 @@
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util_win.h"
 #include "ui/gfx/win/dpi.h"
+#include "ui/shell_dialogs/select_file_dialog.h"
 #endif  // defined(OS_WIN)
 
 #if defined(OS_MACOSX)
@@ -1101,6 +1103,9 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
     return ChromeBrowserMainPartsWin::HandleIconsCommands(
         parsed_command_line_);
   }
+
+  ui::SelectFileDialog::SetFactory(new ChromeSelectFileDialogFactory(
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)));
 #endif
 
   if (parsed_command_line().HasSwitch(switches::kMakeDefaultBrowser)) {

chrome/browser/chrome_select_file_dialog_factory_win.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chrome_select_file_dialog_factory_win.h"
+
+#include <Windows.h>
+#include <commdlg.h>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/metrics/field_trial.h"
+#include "base/strings/string16.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/win/metro.h"
+#include "chrome/common/chrome_utility_messages.h"
+#include "content/public/browser/utility_process_host.h"
+#include "content/public/browser/utility_process_host_client.h"
+#include "ipc/ipc_message_macros.h"
+#include "ui/base/win/open_file_name_win.h"
+#include "ui/shell_dialogs/select_file_dialog_win.h"
+
+namespace {
+
+// Receives the GetOpenFileName result from the utility process.
+class GetOpenFileNameClient : public content::UtilityProcessHostClient {
+ public:
+  GetOpenFileNameClient();
+
+  // Blocks until the GetOpenFileName result is received (including failure to
+  // launch or a crash of the utility process).
+  void WaitForCompletion();
+
+  // Returns the selected directory.
+  const base::FilePath& directory() const { return directory_; }
+
+  // Returns the list of selected filenames. Each should be interpreted as a
+  // child of directory().
+  const std::vector<base::FilePath>& filenames() const { return filenames_; }
+
+  // UtilityProcessHostClient implementation
+  virtual void OnProcessCrashed(int exit_code) OVERRIDE;
+  virtual void OnProcessLaunchFailed() OVERRIDE;
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
+ protected:
+  virtual ~GetOpenFileNameClient();
+
+ private:
+  void OnResult(const base::FilePath& directory,
+                const std::vector<base::FilePath>& filenames);
+  void OnFailure();
+
+  base::FilePath directory_;
+  std::vector<base::FilePath> filenames_;
+  base::WaitableEvent event_;
+
+  DISALLOW_COPY_AND_ASSIGN(GetOpenFileNameClient);
+};
+
+GetOpenFileNameClient::GetOpenFileNameClient() : event_(true, false) {
+}
+
+void GetOpenFileNameClient::WaitForCompletion() {
+  event_.Wait();
+}
+
+void GetOpenFileNameClient::OnProcessCrashed(int exit_code) {
+  event_.Signal();
+}
+
+void GetOpenFileNameClient::OnProcessLaunchFailed() {
+  event_.Signal();
+}
+
+bool GetOpenFileNameClient::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(GetOpenFileNameClient, message)
+    IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_GetOpenFileName_Failed,
+                        OnFailure)
+    IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_GetOpenFileName_Result,
+                        OnResult)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+GetOpenFileNameClient::~GetOpenFileNameClient() {}
+
+void GetOpenFileNameClient::OnResult(
+    const base::FilePath& directory,
+    const std::vector<base::FilePath>& filenames) {
+  directory_ = directory;
+  filenames_ = filenames;
+  event_.Signal();
+}
+
+void GetOpenFileNameClient::OnFailure() {
+  event_.Signal();
+}
+
+// Initiates IPC with a new utility process using |client|. Instructs the
+// utility process to call GetOpenFileName with |ofn|. |current_task_runner|
+// must be the currently executing task runner.
+void DoInvokeGetOpenFileName(
+    OPENFILENAME* ofn,
+    scoped_refptr<GetOpenFileNameClient> client,
+    const scoped_refptr<base::SequencedTaskRunner>& current_task_runner) {
+  DCHECK(current_task_runner->RunsTasksOnCurrentThread());
+
+  base::WeakPtr<content::UtilityProcessHost> utility_process_host(
+      content::UtilityProcessHost::Create(client, current_task_runner)
+      ->AsWeakPtr());
+  utility_process_host->DisableSandbox();
+  utility_process_host->Send(new ChromeUtilityMsg_GetOpenFileName(
+      ofn->hwndOwner,
+      ofn->Flags,
+      ui::win::OpenFileName::GetFilters(ofn),
+      base::FilePath(ofn->lpstrInitialDir ? ofn->lpstrInitialDir
+                                          : base::string16()),
+      base::FilePath(ofn->lpstrFile)));
+}
+
+// Invokes GetOpenFileName in a utility process. Blocks until the result is
+// received. Uses |blocking_task_runner| for IPC.
+bool GetOpenFileNameInUtilityProcess(
+    const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
+    OPENFILENAME* ofn) {
+  scoped_refptr<GetOpenFileNameClient> client(new GetOpenFileNameClient);
+  blocking_task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&DoInvokeGetOpenFileName,
+                 base::Unretained(ofn), client, blocking_task_runner));
+  client->WaitForCompletion();
+
+  if (!client->filenames().size())
+    return false;
+
+  ui::win::OpenFileName::SetResult(
+      client->directory(), client->filenames(), ofn);
+  return true;
+}
+
+// Implements GetOpenFileName for CreateWinSelectFileDialog by delegating either
+// to Metro or a utility process.
+bool GetOpenFileNameImpl(
+    const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
+    OPENFILENAME* ofn) {
+  HMODULE metro_module = base::win::GetMetroModule();
+  if (metro_module != NULL) {
+    typedef BOOL (*MetroGetOpenFileName)(OPENFILENAME*);
+    MetroGetOpenFileName metro_get_open_file_name =
+        reinterpret_cast<MetroGetOpenFileName>(
+            ::GetProcAddress(metro_module, "MetroGetOpenFileName"));
+    if (metro_get_open_file_name == NULL) {
+      NOTREACHED();
+      return false;
+    }
+
+    return metro_get_open_file_name(ofn) == TRUE;
+  }
+
+  if (base::FieldTrialList::FindFullName("IsolateShellOperations") ==
+      "Enabled") {
+    return GetOpenFileNameInUtilityProcess(blocking_task_runner, ofn);
+  }
+
+  return ::GetOpenFileName(ofn) == TRUE;
+}
+
+}  // namespace
+
+ChromeSelectFileDialogFactory::ChromeSelectFileDialogFactory(
+    const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner)
+    : blocking_task_runner_(blocking_task_runner) {
+}
+
+ChromeSelectFileDialogFactory::~ChromeSelectFileDialogFactory() {}
+
+ui::SelectFileDialog* ChromeSelectFileDialogFactory::Create(
+    ui::SelectFileDialog::Listener* listener,
+    ui::SelectFilePolicy* policy) {
+  return ui::CreateWinSelectFileDialog(
+      listener,
+      policy,
+      base::Bind(GetOpenFileNameImpl, blocking_task_runner_));
+}

chrome/browser/chrome_select_file_dialog_factory_win.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_CHROME_SELECT_FILE_DIALOG_FACTORY_WIN_H_
+#define CHROME_BROWSER_CHROME_SELECT_FILE_DIALOG_FACTORY_WIN_H_
+
+#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "ui/shell_dialogs/select_file_dialog_factory.h"
+
+namespace base {
+class SequencedTaskRunner;
+}  // namespace base
+
+// Implements a Select File dialog that delegates to a Metro file picker on
+// Metro and to a utility process otherwise. The utility process is used in
+// order to isolate the Chrome browser process from potential instability
+// caused by Shell extension modules loaded by GetOpenFileName.
+class ChromeSelectFileDialogFactory : public ui::SelectFileDialogFactory {
+ public:
+  // Uses |blocking_task_runner| to perform IPC with the utility process.
+  explicit ChromeSelectFileDialogFactory(
+      const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner);
+  virtual ~ChromeSelectFileDialogFactory();
+
+  // ui::SelectFileDialogFactory implementation
+  virtual ui::SelectFileDialog* Create(ui::SelectFileDialog::Listener* listener,
+                                       ui::SelectFilePolicy* policy) OVERRIDE;
+
+ private:
+  scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeSelectFileDialogFactory);
+};
+
+#endif  // CHROME_BROWSER_CHROME_SELECT_FILE_DIALOG_FACTORY_WIN_H_

chrome/chrome_browser.gypi

@@ -260,6 +260,8 @@
       'browser/chrome_page_zoom.h',
       'browser/chrome_page_zoom_constants.cc',
       'browser/chrome_page_zoom_constants.h',
+      'browser/chrome_select_file_dialog_factory_win.cc',
+      'browser/chrome_select_file_dialog_factory_win.h',
       'browser/chrome_quota_permission_context.cc',
       'browser/chrome_quota_permission_context.h',
       'browser/command_observer.h',

chrome/chrome_utility.gypi

@@ -17,6 +17,8 @@
       'utility/local_discovery/service_discovery_message_handler.h',
       'utility/printing_handler.cc',
       'utility/printing_handler.h',
+      'utility/shell_handler_win.cc',
+      'utility/shell_handler_win.h',
       'utility/utility_message_handler.h',
       'utility/web_resource_unpacker.cc',
       'utility/web_resource_unpacker.h',

chrome/common/chrome_utility_messages.h

@@ -4,6 +4,10 @@
 
 // Multiply-included message file, so no include guard.
 
+#if defined(OS_WIN)
+#include <Windows.h>
+#endif  // defined(OS_WIN)
+
 #include <string>
 #include <vector>
 
@@ -86,6 +90,28 @@ IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_AnalyzeZipFileForDownloadProtection,
                      IPC::PlatformFileForTransit /* zip_file */)
 #endif
 
+#if defined(OS_WIN)
+// A vector of filters, each being a Tuple2a display string (i.e. "Text Files")
+// and a filter pattern (i.e. "*.txt")..
+typedef std::vector<Tuple2<base::string16, base::string16> >
+    GetOpenFileNameFilter;
+
+// Instructs the utility process to invoke GetOpenFileName. |owner| is the
+// parent of the modal dialog, |flags| are OFN_* flags. |filter| constrains the
+// user's file choices. |initial_directory| and |filename| select the directory
+// to be displayed and the file to be initially selected.
+//
+// Either ChromeUtilityHostMsg_GetOpenFileName_Failed or
+// ChromeUtilityHostMsg_GetOpenFileName_Result will be returned when the
+// operation completes whether due to error or user action.
+IPC_MESSAGE_CONTROL5(ChromeUtilityMsg_GetOpenFileName,
+                     HWND /* owner */,
+                     DWORD /* flags */,
+                     GetOpenFileNameFilter /* filter */,
+                     base::FilePath /* initial_directory */,
+                     base::FilePath /* filename */)
+#endif  // defined(OS_WIN)
+
 //------------------------------------------------------------------------------
 // Utility process host messages:
 // These are messages from the utility process to the browser.
@@ -128,3 +154,10 @@ IPC_MESSAGE_CONTROL1(
     ChromeUtilityHostMsg_AnalyzeZipFileForDownloadProtection_Finished,
     safe_browsing::zip_analyzer::Results)
 #endif
+
+#if defined(OS_WIN)
+IPC_MESSAGE_CONTROL0(ChromeUtilityHostMsg_GetOpenFileName_Failed)
+IPC_MESSAGE_CONTROL2(ChromeUtilityHostMsg_GetOpenFileName_Result,
+                     base::FilePath /* directory */,
+                     std::vector<base::FilePath> /* filenames */)
+#endif  // defined(OS_WIN)

chrome/utility/chrome_content_utility_client.cc

@@ -27,6 +27,10 @@
 #include "chrome/utility/profile_import_handler.h"
 #endif
 
+#if defined(OS_WIN)
+#include "chrome/utility/shell_handler_win.h"
+#endif
+
 #if defined(ENABLE_EXTENSIONS)
 #include "chrome/common/extensions/chrome_utility_extensions_messages.h"
 #include "chrome/utility/extensions/extensions_handler.h"
@@ -87,6 +91,10 @@ ChromeContentUtilityClient::ChromeContentUtilityClient()
     handlers_.push_back(new local_discovery::ServiceDiscoveryMessageHandler());
   }
 #endif
+
+#if defined(OS_WIN)
+  handlers_.push_back(new ShellHandler());
+#endif
 }
 
 ChromeContentUtilityClient::~ChromeContentUtilityClient() {

chrome/utility/shell_handler_win.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/utility/shell_handler_win.h"
+
+#include <commdlg.h>
+
+#include "base/files/file_path.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/common/chrome_utility_messages.h"
+#include "content/public/utility/utility_thread.h"
+#include "ui/base/win/open_file_name_win.h"
+
+ShellHandler::ShellHandler() {}
+ShellHandler::~ShellHandler() {}
+
+bool ShellHandler::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(ShellHandler, message)
+    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_GetOpenFileName,
+                        OnGetOpenFileName)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void ShellHandler::OnGetOpenFileName(
+    HWND owner,
+    DWORD flags,
+    const GetOpenFileNameFilter& filter,
+    const base::FilePath& initial_directory,
+    const base::FilePath& filename) {
+  ui::win::OpenFileName open_file_name(owner, flags);
+  open_file_name.SetInitialSelection(initial_directory, filename);
+  open_file_name.SetFilters(filter);
+
+  base::FilePath directory;
+  std::vector<base::FilePath> filenames;
+
+  if (::GetOpenFileName(open_file_name.GetOPENFILENAME()))
+    open_file_name.GetResult(&directory, &filenames);
+
+  if (filenames.size()) {
+    content::UtilityThread::Get()->Send(
+        new ChromeUtilityHostMsg_GetOpenFileName_Result(directory, filenames));
+  } else {
+    content::UtilityThread::Get()->Send(
+        new ChromeUtilityHostMsg_GetOpenFileName_Failed());
+  }
+}

chrome/utility/shell_handler_win.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_UTILITY_SHELL_HANDLER_WIN_H_
+#define CHROME_UTILITY_SHELL_HANDLER_WIN_H_
+
+#include <Windows.h>
+
+#include <vector>
+
+#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "base/strings/string16.h"
+#include "base/tuple.h"
+#include "chrome/utility/utility_message_handler.h"
+
+namespace base {
+class FilePath;
+}  // namespace base
+
+typedef std::vector<Tuple2<base::string16, base::string16> >
+    GetOpenFileNameFilter;
+
+// Handles requests to execute shell operations. Used to protect the browser
+// process from instability due to 3rd-party shell extensions. Must be invoked
+// in a non-sandboxed utility process.
+class ShellHandler : public UtilityMessageHandler {
+ public:
+  ShellHandler();
+  virtual ~ShellHandler();
+
+  // IPC::Listener implementation
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
+ private:
+  void OnGetOpenFileName(
+    HWND owner,
+    DWORD flags,
+    const GetOpenFileNameFilter& filter,
+    const base::FilePath& initial_directory,
+    const base::FilePath& filename);
+
+  DISALLOW_COPY_AND_ASSIGN(ShellHandler);
+};
+
+#endif  // CHROME_UTILITY_SHELL_HANDLER_WIN_H_

ui/base/win/open_file_name_win.cc

@@ -27,6 +27,25 @@ OpenFileName::OpenFileName(HWND parent_window, DWORD flags) {
 OpenFileName::~OpenFileName() {
 }
 
+void OpenFileName::SetFilters(
+    const std::vector<Tuple2<base::string16, base::string16> >& filters) {
+  openfilename_.lpstrFilter = NULL;
+  filter_buffer_.clear();
+  if (filters.empty())
+    return;
+  for (std::vector<Tuple2<base::string16, base::string16> >::const_iterator
+           it = filters.begin();
+       it != filters.end();
+       ++it) {
+    filter_buffer_.append(it->a);
+    filter_buffer_.push_back(0);
+    filter_buffer_.append(it->b);
+    filter_buffer_.push_back(0);
+  }
+  filter_buffer_.push_back(0);
+  openfilename_.lpstrFilter = filter_buffer_.c_str();
+}
+
 void OpenFileName::SetInitialSelection(const base::FilePath& initial_directory,
                                        const base::FilePath& initial_filename) {
   // First reset to the default case.
@@ -83,5 +102,58 @@ void OpenFileName::GetResult(base::FilePath* directory,
   }
 }
 
+// static
+void OpenFileName::SetResult(const base::FilePath& directory,
+                             const std::vector<base::FilePath>& filenames,
+                             OPENFILENAME* openfilename) {
+  base::string16 filename_value;
+  if (filenames.size() == 1) {
+    filename_value = directory.Append(filenames[0]).value();
+  } else {
+    filename_value = directory.value();
+    filename_value.push_back(0);
+    for (std::vector<base::FilePath>::const_iterator it = filenames.begin();
+         it != filenames.end();
+         ++it) {
+      filename_value.append(it->value());
+      filename_value.push_back(0);
+    }
+  }
+  if (filename_value.size() + 1 < openfilename->nMaxFile) {
+    // Because the result has embedded nulls, we must memcpy.
+    memcpy(openfilename->lpstrFile,
+           filename_value.c_str(),
+           (filename_value.size() + 1) * sizeof(filename_value[0]));
+  } else if (openfilename->nMaxFile) {
+    openfilename->lpstrFile[0] = 0;
+  }
+}
+
+// static
+std::vector<Tuple2<base::string16, base::string16> > OpenFileName::GetFilters(
+    const OPENFILENAME* openfilename) {
+  std::vector<Tuple2<base::string16, base::string16> > filters;
+
+  const base::char16* display_string = openfilename->lpstrFilter;
+  if (!display_string)
+    return filters;
+
+  while (*display_string) {
+    const base::char16* display_string_end = display_string;
+    while (*display_string_end)
+      ++display_string_end;
+    const base::char16* pattern = display_string_end + 1;
+    const base::char16* pattern_end = pattern;
+    while (*pattern_end)
+      ++pattern_end;
+    filters.push_back(
+        MakeTuple(base::string16(display_string, display_string_end),
+                  base::string16(pattern, pattern_end)));
+    display_string = pattern_end + 1;
+  }
+
+  return filters;
+}
+
 }  // namespace win
 }  // namespace ui

ui/base/win/open_file_name_win.h

@@ -32,6 +32,10 @@ class UI_BASE_EXPORT OpenFileName {
   OpenFileName(HWND parent_window, DWORD flags);
   ~OpenFileName();
 
+  // Initializes |lpstrFilter| from the label/pattern pairs in |filters|.
+  void SetFilters(
+      const std::vector<Tuple2<base::string16, base::string16> >& filters);
+
   // Sets |lpstrInitialDir| and |lpstrFile|.
   void SetInitialSelection(const base::FilePath& initial_directory,
                            const base::FilePath& initial_filename);
@@ -47,10 +51,25 @@ class UI_BASE_EXPORT OpenFileName {
   // Returns the OPENFILENAME structure.
   OPENFILENAME* GetOPENFILENAME() { return &openfilename_; }
 
+  // Returns the OPENFILENAME structure.
+  const OPENFILENAME* GetOPENFILENAME() const { return &openfilename_; }
+
+  // Stores directory and filenames in the buffer pointed to by
+  // |openfilename->lpstrFile| and sized |openfilename->nMaxFile|.
+  static void SetResult(const base::FilePath& directory,
+                        const std::vector<base::FilePath>& filenames,
+                        OPENFILENAME* openfilename);
+
+  // Returns a vector of label/pattern pairs built from
+  // |openfilename->lpstrFilter|.
+  static std::vector<Tuple2<base::string16, base::string16> > GetFilters(
+      const OPENFILENAME* openfilename);
+
  private:
   OPENFILENAME openfilename_;
   base::string16 initial_directory_buffer_;
   wchar_t filename_buffer_[UNICODE_STRING_MAX_CHARS];
+  base::string16 filter_buffer_;
 
   DISALLOW_COPY_AND_ASSIGN(OpenFileName);
 };

ui/base/win/open_file_name_win_unittest.cc

@@ -12,16 +12,64 @@ const HWND kHwnd = reinterpret_cast<HWND>(0xDEADBEEF);
 const DWORD kFlags = OFN_OVERWRITEPROMPT | OFN_EXPLORER | OFN_ENABLESIZING;
 
 void SetResult(const base::string16& result, ui::win::OpenFileName* ofn) {
-  EXPECT_GT(ofn->GetOPENFILENAME()->nMaxFile, result.size());
-  if (ofn->GetOPENFILENAME()->nMaxFile > result.size()) {
-    if (!result.size()) {
-      ofn->GetOPENFILENAME()->lpstrFile[0] = 0;
-    } else {
-      // Because the result has embedded nulls, we must memcpy.
-      memcpy(ofn->GetOPENFILENAME()->lpstrFile,
-             result.c_str(),
-             (result.size() + 1) * sizeof(result[0]));
-    }
+  if (ofn->GetOPENFILENAME()->nMaxFile <= result.size()) {
+    ADD_FAILURE() << "filename buffer insufficient.";
+    return;
+  }
+  if (!result.size()) {
+    ofn->GetOPENFILENAME()->lpstrFile[0] = 0;
+  } else {
+    // Because the result has embedded nulls, we must memcpy.
+    memcpy(ofn->GetOPENFILENAME()->lpstrFile,
+           result.c_str(),
+           (result.size() + 1) * sizeof(result[0]));
+  }
+}
+
+void CheckFilters(
+    const std::vector<Tuple2<base::string16, base::string16> >& expected,
+    const std::vector<Tuple2<base::string16, base::string16> >& actual) {
+  if (expected.size() != actual.size()) {
+    ADD_FAILURE() << "filter count mismatch. Got " << actual.size()
+                  << " expected " << expected.size() << ".";
+    return;
+  }
+
+  for (size_t i = 0; i < expected.size(); ++i) {
+    EXPECT_EQ(expected[i].a, actual[i].a) << "Mismatch at index " << i;
+    EXPECT_EQ(expected[i].b, actual[i].b) << "Mismatch at index " << i;
+  }
+}
+
+void CheckFilterString(const base::string16& expected,
+                       const ui::win::OpenFileName& ofn) {
+  if (!ofn.GetOPENFILENAME()->lpstrFilter) {
+    ADD_FAILURE() << "Filter string is NULL.";
+    return;
+  }
+  if (expected.size() == 0) {
+    EXPECT_EQ(0, ofn.GetOPENFILENAME()->lpstrFilter[0]);
+  } else {
+    EXPECT_EQ(0,
+              memcmp(expected.c_str(),
+                     ofn.GetOPENFILENAME()->lpstrFilter,
+                     expected.size() + 1 * sizeof(expected[0])));
+  }
+}
+
+void CheckResult(const base::string16& expected,
+                 const ui::win::OpenFileName& ofn) {
+  if (!ofn.GetOPENFILENAME()->lpstrFile) {
+    ADD_FAILURE() << "File string is NULL.";
+    return;
+  }
+  if (expected.size() == 0) {
+    EXPECT_EQ(0, ofn.GetOPENFILENAME()->lpstrFile[0]);
+  } else {
+    EXPECT_EQ(0,
+              memcmp(expected.c_str(),
+                     ofn.GetOPENFILENAME()->lpstrFile,
+                     expected.size() + 1 * sizeof(expected[0])));
   }
 }
 
@@ -135,3 +183,54 @@ TEST(OpenFileNameTest, GetResult) {
   EXPECT_EQ(base::FilePath(), directory);
   ASSERT_EQ(0u, filenames.size());
 }
+
+TEST(OpenFileNameTest, SetAndGetFilters) {
+  const base::string16 kNull(L"\0", 1);
+
+  ui::win::OpenFileName ofn(kHwnd, kFlags);
+  std::vector<Tuple2<base::string16, base::string16> > filters;
+  ofn.SetFilters(filters);
+  EXPECT_FALSE(ofn.GetOPENFILENAME()->lpstrFilter);
+  CheckFilters(filters,
+               ui::win::OpenFileName::GetFilters(ofn.GetOPENFILENAME()));
+
+  filters.push_back(MakeTuple(base::string16(L"a"), base::string16(L"b")));
+  ofn.SetFilters(filters);
+  CheckFilterString(L"a" + kNull + L"b" + kNull, ofn);
+  CheckFilters(filters,
+               ui::win::OpenFileName::GetFilters(ofn.GetOPENFILENAME()));
+
+  filters.push_back(MakeTuple(base::string16(L"X"), base::string16(L"Y")));
+  ofn.SetFilters(filters);
+  CheckFilterString(L"a" + kNull + L"b" + kNull + L"X" + kNull + L"Y" + kNull,
+                    ofn);
+  CheckFilters(filters,
+               ui::win::OpenFileName::GetFilters(ofn.GetOPENFILENAME()));
+}
+
+TEST(OpenFileNameTest, SetResult) {
+  const base::string16 kNull(L"\0", 1);
+
+  ui::win::OpenFileName ofn(kHwnd, kFlags);
+  base::FilePath directory;
+  std::vector<base::FilePath> filenames;
+
+  ui::win::OpenFileName::SetResult(directory, filenames, ofn.GetOPENFILENAME());
+  CheckResult(L"", ofn);
+
+  directory = base::FilePath(L"C:\\dir");
+  filenames.push_back(base::FilePath(L"file"));
+  ui::win::OpenFileName::SetResult(directory, filenames, ofn.GetOPENFILENAME());
+  CheckResult(L"C:\\dir\\file" + kNull, ofn);
+
+  filenames.push_back(base::FilePath(L"otherfile"));
+  ui::win::OpenFileName::SetResult(directory, filenames, ofn.GetOPENFILENAME());
+  CheckResult(L"C:\\dir" + kNull + L"file" + kNull + L"otherfile" + kNull, ofn);
+
+  base::char16 short_buffer[10] = L"";
+
+  ofn.GetOPENFILENAME()->lpstrFile = short_buffer;
+  ofn.GetOPENFILENAME()->nMaxFile = arraysize(short_buffer);
+  ui::win::OpenFileName::SetResult(directory, filenames, ofn.GetOPENFILENAME());
+  CheckResult(L"", ofn);
+}

ui/shell_dialogs/select_file_dialog.cc

@@ -84,7 +84,7 @@ scoped_refptr<SelectFileDialog> SelectFileDialog::Create(
 #if defined(OS_WIN)
   // TODO(ananta)
   // Fix this for Chrome ASH on Windows.
-  return CreateWinSelectFileDialog(listener, policy);
+  return CreateDefaultWinSelectFileDialog(listener, policy);
 #elif defined(OS_MACOSX) && !defined(USE_AURA)
   return CreateMacSelectFileDialog(listener, policy);
 #elif defined(OS_ANDROID)

ui/shell_dialogs/select_file_dialog_win.cc

@@ -4,8 +4,6 @@
 
 #include "ui/shell_dialogs/select_file_dialog_win.h"
 
-#include <windows.h>
-#include <commdlg.h>
 #include <shlobj.h>
 
 #include <algorithm>
@@ -38,31 +36,16 @@
 
 namespace {
 
+bool CallBuiltinGetOpenFileName(OPENFILENAME* ofn) {
+  return ::GetOpenFileName(ofn) == TRUE;
+}
+
 // Given |extension|, if it's not empty, then remove the leading dot.
 std::wstring GetExtensionWithoutLeadingDot(const std::wstring& extension) {
   DCHECK(extension.empty() || extension[0] == L'.');
   return extension.empty() ? extension : extension.substr(1);
 }
 
-// Diverts to a metro-specific implementation as appropriate.
-bool CallGetOpenFileName(OPENFILENAME* ofn) {
-  HMODULE metro_module = base::win::GetMetroModule();
-  if (metro_module != NULL) {
-    typedef BOOL (*MetroGetOpenFileName)(OPENFILENAME*);
-    MetroGetOpenFileName metro_get_open_file_name =
-        reinterpret_cast<MetroGetOpenFileName>(
-            ::GetProcAddress(metro_module, "MetroGetOpenFileName"));
-    if (metro_get_open_file_name == NULL) {
-      NOTREACHED();
-      return false;
-    }
-
-    return metro_get_open_file_name(ofn) == TRUE;
-  } else {
-    return GetOpenFileName(ofn) == TRUE;
-  }
-}
-
 // Diverts to a metro-specific implementation as appropriate.
 bool CallGetSaveFileName(OPENFILENAME* ofn) {
   HMODULE metro_module = base::win::GetMetroModule();
@@ -410,8 +393,10 @@ bool SaveFileAs(HWND owner,
 class SelectFileDialogImpl : public ui::SelectFileDialog,
                              public ui::BaseShellDialogImpl {
  public:
-  explicit SelectFileDialogImpl(Listener* listener,
-                                ui::SelectFilePolicy* policy);
+  SelectFileDialogImpl(
+      Listener* listener,
+      ui::SelectFilePolicy* policy,
+      const base::Callback<bool(OPENFILENAME*)>& get_open_file_name_impl);
 
   // BaseShellDialog implementation:
   virtual bool IsRunning(gfx::NativeWindow owning_window) const OVERRIDE;
@@ -520,15 +505,19 @@ class SelectFileDialogImpl : public ui::SelectFileDialog,
   base::string16 GetFilterForFileTypes(const FileTypeInfo* file_types);
 
   bool has_multiple_file_type_choices_;
+  base::Callback<bool(OPENFILENAME*)> get_open_file_name_impl_;
 
   DISALLOW_COPY_AND_ASSIGN(SelectFileDialogImpl);
 };
 
-SelectFileDialogImpl::SelectFileDialogImpl(Listener* listener,
-                                           ui::SelectFilePolicy* policy)
+SelectFileDialogImpl::SelectFileDialogImpl(
+    Listener* listener,
+    ui::SelectFilePolicy* policy,
+    const base::Callback<bool(OPENFILENAME*)>& get_open_file_name_impl)
     : SelectFileDialog(listener, policy),
       BaseShellDialogImpl(),
-      has_multiple_file_type_choices_(false) {
+      has_multiple_file_type_choices_(false),
+      get_open_file_name_impl_(get_open_file_name_impl) {
 }
 
 SelectFileDialogImpl::~SelectFileDialogImpl() {
@@ -786,7 +775,8 @@ bool SelectFileDialogImpl::RunOpenFileDialog(
 
   if (!filter.empty())
     ofn.GetOPENFILENAME()->lpstrFilter = filter.c_str();
-  bool success = CallGetOpenFileName(ofn.GetOPENFILENAME());
+
+  bool success = get_open_file_name_impl_.Run(ofn.GetOPENFILENAME());
   DisableOwner(owner);
   if (success)
     *path = ofn.GetSingleResult();
@@ -811,8 +801,9 @@ bool SelectFileDialogImpl::RunOpenMultiFileDialog(
   base::FilePath directory;
   std::vector<base::FilePath> filenames;
 
-  if (CallGetOpenFileName(ofn.GetOPENFILENAME()))
+  if (get_open_file_name_impl_.Run(ofn.GetOPENFILENAME()))
     ofn.GetResult(&directory, &filenames);
+
   DisableOwner(owner);
 
   for (std::vector<base::FilePath>::iterator it = filenames.begin();
@@ -893,9 +884,17 @@ std::wstring AppendExtensionIfNeeded(
 }
 
 SelectFileDialog* CreateWinSelectFileDialog(
+    SelectFileDialog::Listener* listener,
+    SelectFilePolicy* policy,
+    const base::Callback<bool(OPENFILENAME* ofn)>& get_open_file_name_impl) {
+  return new SelectFileDialogImpl(listener, policy, get_open_file_name_impl);
+}
+
+SelectFileDialog* CreateDefaultWinSelectFileDialog(
     SelectFileDialog::Listener* listener,
     SelectFilePolicy* policy) {
-  return new SelectFileDialogImpl(listener, policy);
+  return CreateWinSelectFileDialog(
+      listener, policy, base::Bind(&CallBuiltinGetOpenFileName));
 }
 
 }  // namespace ui

ui/shell_dialogs/select_file_dialog_win.h

@@ -5,6 +5,10 @@
 #ifndef UI_SHELL_DIALOGS_SELECT_FILE_DIALOG_WIN_H_
 #define UI_SHELL_DIALOGS_SELECT_FILE_DIALOG_WIN_H_
 
+#include <Windows.h>
+#include <commdlg.h>
+
+#include "base/callback_forward.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
 #include "ui/shell_dialogs/shell_dialogs_export.h"
@@ -18,9 +22,12 @@ SHELL_DIALOGS_EXPORT std::wstring AppendExtensionIfNeeded(
     const std::wstring& filter_selected,
     const std::wstring& suggested_ext);
 
-// Intentionally not exported. Implementation detail of SelectFileDialog. Use
-// SelectFileDialog::Create() instead.
-SelectFileDialog* CreateWinSelectFileDialog(
+SHELL_DIALOGS_EXPORT SelectFileDialog* CreateWinSelectFileDialog(
+    SelectFileDialog::Listener* listener,
+    SelectFilePolicy* policy,
+    const base::Callback<bool(OPENFILENAME* ofn)>& get_open_file_name_impl);
+
+SelectFileDialog* CreateDefaultWinSelectFileDialog(
     SelectFileDialog::Listener* listener,
     SelectFilePolicy* policy);