Experimentally isolate GetOpenFileName in a utility process.

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289625

[erikwright (departed), 2014-07-31 17:03:13]

sky: PTAL.

[sky, 2014-07-31 20:47:47]

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
File chrome/browser/chrome_select_file_dialog_factory_win.cc (right):

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:38: const
base::FilePath& directory() { return directory_; }
make const.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:42: const
std::vector<base::FilePath>& filenames() { return filenames_; }
make const

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:67:
GetOpenFileNameClient::~GetOpenFileNameClient() {}
declaration/definition order should match.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:94: const
std::vector<base::FilePath> &
wrapping here is weird. & should be next to >

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:122:
base::FilePath(ofn->lpstrInitialDir ? ofn->lpstrInitialDir : L""),
""->std::wstring or string16.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:163: } else if
(base::FieldTrialList::FindFullName("IsolateShellOperations") ==
nit: no else after return here and 166.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
File chrome/browser/chrome_select_file_dialog_factory_win.h (right):

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.h:22: public:
nit: spacing is off.

https://codereview.chromium.org/419523006/diff/130001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/419523006/diff/130001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:99: DWORD /* flags */,
Better document what these correspond to.

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
File ui/base/win/open_file_name_win.cc (right):

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
ui/base/win/open_file_name_win.cc:110: return &openfilename_;
nit: this is trivial enough that I would inline it.

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
ui/base/win/open_file_name_win.cc:130: if (filename_value.size() + 1 <
openfilename->nMaxFile) {
What about the else case here?

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
ui/base/win/open_file_name_win.cc:143: const base::char16* name =
openfilename->lpstrFilter;
Assigning 'name' to 'filter' is rather confusing.

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
ui/base/win/open_file_name_win.cc:149: while (*name_end) {
nit: no {} here and 154

https://codereview.chromium.org/419523006/diff/130001/ui/shell_dialogs/select...
File ui/shell_dialogs/select_file_dialog_win.cc (right):

https://codereview.chromium.org/419523006/diff/130001/ui/shell_dialogs/select...
ui/shell_dialogs/select_file_dialog_win.cc:39: bool
CallBuiltinGetOpenFileName(OPENFILENAME* ofn) {
nit: don't indent this (same as 44 below).

[erikwright (departed), 2014-08-01 13:53:16]

sky: PTAL.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
File chrome/browser/chrome_select_file_dialog_factory_win.cc (right):

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:38: const
base::FilePath& directory() { return directory_; }
On 2014/07/31 20:47:46, sky wrote:
> make const.

Done.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:42: const
std::vector<base::FilePath>& filenames() { return filenames_; }
On 2014/07/31 20:47:46, sky wrote:
> make const

Done.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:67:
GetOpenFileNameClient::~GetOpenFileNameClient() {}
On 2014/07/31 20:47:46, sky wrote:
> declaration/definition order should match.

Done.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:94: const
std::vector<base::FilePath> &
On 2014/07/31 20:47:46, sky wrote:
> wrapping here is weird. & should be next to >

Done.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:122:
base::FilePath(ofn->lpstrInitialDir ? ofn->lpstrInitialDir : L""),
On 2014/07/31 20:47:46, sky wrote:
> ""->std::wstring or string16.

Done.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.cc:163: } else if
(base::FieldTrialList::FindFullName("IsolateShellOperations") ==
On 2014/07/31 20:47:46, sky wrote:
> nit: no else after return here and 166.

Done.

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
File chrome/browser/chrome_select_file_dialog_factory_win.h (right):

https://codereview.chromium.org/419523006/diff/130001/chrome/browser/chrome_s...
chrome/browser/chrome_select_file_dialog_factory_win.h:22: public:
On 2014/07/31 20:47:46, sky wrote:
> nit: spacing is off.

Done.

https://codereview.chromium.org/419523006/diff/130001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/419523006/diff/130001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:99: DWORD /* flags */,
On 2014/07/31 20:47:46, sky wrote:
> Better document what these correspond to.

Done.

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
File ui/base/win/open_file_name_win.cc (right):

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
ui/base/win/open_file_name_win.cc:110: return &openfilename_;
On 2014/07/31 20:47:46, sky wrote:
> nit: this is trivial enough that I would inline it.

Done.

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
ui/base/win/open_file_name_win.cc:130: if (filename_value.size() + 1 <
openfilename->nMaxFile) {
On 2014/07/31 20:47:46, sky wrote:
> What about the else case here?

Done. Also added a test case.

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
ui/base/win/open_file_name_win.cc:143: const base::char16* name =
openfilename->lpstrFilter;
On 2014/07/31 20:47:46, sky wrote:
> Assigning 'name' to 'filter' is rather confusing.

Done.

https://codereview.chromium.org/419523006/diff/130001/ui/base/win/open_file_n...
ui/base/win/open_file_name_win.cc:149: while (*name_end) {
On 2014/07/31 20:47:46, sky wrote:
> nit: no {} here and 154

Done.

https://codereview.chromium.org/419523006/diff/130001/ui/shell_dialogs/select...
File ui/shell_dialogs/select_file_dialog_win.cc (right):

https://codereview.chromium.org/419523006/diff/130001/ui/shell_dialogs/select...
ui/shell_dialogs/select_file_dialog_win.cc:39: bool
CallBuiltinGetOpenFileName(OPENFILENAME* ofn) {
On 2014/07/31 20:47:46, sky wrote:
> nit: don't indent this (same as 44 below).

Done.

[erikwright (departed), 2014-08-01 13:54:41]

cpalmer: PTAL for chrome_utility_messages.h.

[sky, 2014-08-04 18:40:26]

LGTM

[erikwright (departed), 2014-08-12 13:20:53]

palmer: PTAL for chrome_utility_messags.h

[-cpalmer, +palmer]

[palmer, 2014-08-12 21:40:09]

I think jschuh of wfh might be better reviewers for this. I'm not confident I
know what's expected on Windows.

[erikwright (departed), 2014-08-13 17:17:23]

jschuh: ptal for new IPC messages. These are being sent from the browser to an
unsandboxed utility process. The work being done is identical to work previously
done in the browser, but by putting it in a separate process we avoid loading
3rd-party modules into the browser.

[jschuh, 2014-08-14 15:12:33]

Weird, but is it's unsandboxed <-> unsandboxed, so lgtm for ipc security.

[erikwright (departed), 2014-08-14 15:14:36]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-08-14 15:15:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/419523006/190001

[commit-bot: I haz the power, 2014-08-14 17:14:53]

Committed patchset #11 (190001) as 289625