Fix the open file dialog on Windows

Fix the open file dialog on Windows

Currently even if a valid file is selected the OpenFileDialog is likely
to return an emtpy FilePath, depending on what is in memory after the
sole string returned by the Windows OFD. The code as written assumes
the Windows OFD will always return a null-terminated list of
null-terminated strings when returning successfully from the call to
the OFD. However if you check the MSDN documentation at:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms646839(v=vs.85).aspx

You will find this is not the case, and the return value depends on the
presence of OFN_ALLOWMULTISELECT in the Flags argument. If that value
is not set, which it is not in the case of single-file selection, then
there is only a single string in |openfilename_.lpstrFile|, and the
code within OpenFileName::GetResult() blows past that single NULL and
into unitialized memory, which ::GetSingleResult then assumes means
an invalid return filename and returns the empty FilePath.

This CL repairs that situation so that GetSingleResult now DCHECKS
that the OFN_ALLOWMULTISELECT is not there and returns the string
directly, and ::GetResult() asserts that the flag is present and
continues to do its multi-value parsing that it was doing before.

BUG=418760
TEST=Launch Chrome, hit CTRL+O to open a file, chose a valid test html
file, observe that the browser doesn't crash and successfully opens that
file.

Committed: https://crrev.com/702fdfed602988a1d404ae4c862c252d40209995
Cr-Commit-Position: refs/heads/master@{#297489}

[luken, 2014-09-29 22:19:08]

luken@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2014-09-29 23:53:02]

sky@chromium.org changed reviewers:
+ erikwright@chromium.org

[sky, 2014-09-29 23:53:03]

https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
File ui/base/win/open_file_name_win.cc (right):

https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
ui/base/win/open_file_name_win.cc:141: base::FilePath
OpenFileName::GetSingleResult() {
I would rather we always do the right thing here, and not assume GetSingleResult
is only called if allow multi-select was false.

https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
ui/base/win/open_file_name_win.cc:155: // The return value of
|openfilename_.lpstrFile| is dependent on the
nit: don't duplicate almost the whole comment. Do something like, see comment in
GetSingleResult for details.

[erikwright (departed), 2014-09-30 00:14:13]

On 2014/09/29 23:53:03, sky wrote:
>
https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
> File ui/base/win/open_file_name_win.cc (right):
> 
>
https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
> ui/base/win/open_file_name_win.cc:141: base::FilePath
> OpenFileName::GetSingleResult() {
> I would rather we always do the right thing here, and not assume
GetSingleResult
> is only called if allow multi-select was false.
> 
>
https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
> ui/base/win/open_file_name_win.cc:155: // The return value of
> |openfilename_.lpstrFile| is dependent on the
> nit: don't duplicate almost the whole comment. Do something like, see comment
in
> GetSingleResult for details.

This file has unit tests that I added, if I remember correctly. Please add a
regression test for this.

[luken, 2014-09-30 17:25:07]

I repaired the two unit tests that were not setting OFN_MULTISELECT but were
providing test values assuming it was set, and added another test that doesn't
set the flag, to make sure all configurations are tested.

https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
File ui/base/win/open_file_name_win.cc (right):

https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
ui/base/win/open_file_name_win.cc:141: base::FilePath
OpenFileName::GetSingleResult() {
On 2014/09/29 23:53:03, sky wrote:
> I would rather we always do the right thing here, and not assume
GetSingleResult
> is only called if allow multi-select was false.

Agreed, your idea is better. I've refactored to just extract the values
correctly in GetResult().

https://codereview.chromium.org/618503002/diff/1/ui/base/win/open_file_name_w...
ui/base/win/open_file_name_win.cc:155: // The return value of
|openfilename_.lpstrFile| is dependent on the
On 2014/09/29 23:53:03, sky wrote:
> nit: don't duplicate almost the whole comment. Do something like, see comment
in
> GetSingleResult for details.

Done.

[erikwright (departed), 2014-09-30 17:34:15]

LGTM. Thank you for digging into and fixing this!

[sky, 2014-09-30 19:19:28]

LGTM

[luken, 2014-09-30 19:20:24]

The CQ bit was checked by luken@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:22:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/618503002/20001

[commit-bot: I haz the power, 2014-09-30 20:15:32]

Committed patchset #2 (id:20001) as c690c791cdf65683d52ba49902a66e9327188b58

[commit-bot: I haz the power, 2014-09-30 20:16:10]

Patchset 2 (id:??) landed as
https://crrev.com/702fdfed602988a1d404ae4c862c252d40209995
Cr-Commit-Position: refs/heads/master@{#297489}