Exclude files from FileSelectChooser if they can't convert to WebStrings.

Exclude files from FileSelectChooser if they can't convert to WebStrings.

BUG=703303
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2811533002
Cr-Commit-Position: refs/heads/master@{#466118}
Committed: https://chromium.googlesource.com/chromium/src/+/e30abe9ad8527585fc6783d8abd24842b07f67b6

[Charlie Reis, 2017-04-09 15:56:15]

Description was changed from

==========
Exclude files from FileSelectChooser if they can't convert to WebStrings.

BUG=703303
TEST=See bug for repro steps.
==========

to

==========
Exclude files from FileSelectChooser if they can't convert to WebStrings.

BUG=703303
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-04-09 15:56:20]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-09 15:56:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-09 17:14:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-09 17:14:44]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Charlie Reis, 2017-04-10 17:39:24]

creis@chromium.org changed reviewers:
+ asanka@chromium.org, dcheng@chromium.org

[Charlie Reis, 2017-04-10 17:39:25]

dcheng@, I'm curious if you have thoughts on how to handle this FilePath UTF8
conversion bug, since I'm not thrilled with this approach.  Today, the browser
can send down a FilePath with invalid encoding (apparently only on GTK3's file
chooser), which the renderer converts to an empty WebString.  That breaks the
upload, plus it gets the renderer killed (since it doesn't have access to the
empty file path).

I've confirmed that this CL works locally, at least.  It ignores the path if it
won't convert cleanly.  I'm not sure it's an equivalent check to WebString or
the best thing to be doing, though.  Any thoughts?  I'll try to find a way to
test once we decide.

asanka@: I also noticed that you added a
FileSelectHelperTest.GetSanitizedFileName test in
https://codereview.chromium.org/1409003002, which had the following comment:
  // The only thing we are testing is that if the source filename was non-empty,
  // the resulting filename is also not empty. Invalid encoded filenames can
  // cause conversions to fail. Such failures shouldn't cause the resulting
  // filename to disappear.

That sounds like the opposite of what I'm doing, though strangely my change
passes your test.  Am I breaking an invariant you were expecting?  Do you have
thoughts on what we should do here?

[dcheng, 2017-04-10 20:36:52]

LGTM

I don't have any better ideas... =/

[asanka, 2017-04-11 00:43:31]

LGTM. No good ideas here either unless we are open to plumbing a base::FilePath
all the way through. I guess that's not in our cards yet. :-)

[asanka, 2017-04-11 00:47:15]

On 2017/04/10 17:39:25, Charlie Reis wrote:
> dcheng@, I'm curious if you have thoughts on how to handle this FilePath UTF8
> conversion bug, since I'm not thrilled with this approach.  Today, the browser
> can send down a FilePath with invalid encoding (apparently only on GTK3's file
> chooser), which the renderer converts to an empty WebString.  That breaks the
> upload, plus it gets the renderer killed (since it doesn't have access to the
> empty file path).
> 
> I've confirmed that this CL works locally, at least.  It ignores the path if
it
> won't convert cleanly.  I'm not sure it's an equivalent check to WebString or
> the best thing to be doing, though.  Any thoughts?  I'll try to find a way to
> test once we decide.
> 
> asanka@: I also noticed that you added a
> FileSelectHelperTest.GetSanitizedFileName test in
> https://codereview.chromium.org/1409003002, which had the following comment:
>   // The only thing we are testing is that if the source filename was
non-empty,
>   // the resulting filename is also not empty. Invalid encoded filenames can
>   // cause conversions to fail. Such failures shouldn't cause the resulting
>   // filename to disappear.
> 
> That sounds like the opposite of what I'm doing, though strangely my change
> passes your test.  Am I breaking an invariant you were expecting?  Do you have
> thoughts on what we should do here?

This test is for the "Save as" mode for the file chooser where the browser
receives a potentially unsafe string as a filename. The browser sanitizes it
before using it in a file chooser. If the filename is completely invalid (e.g.
consists entirely of invalid characters or had an invalid encoding), then the
browser replaces the name entirely with something known to be safe (e.g.
"download").

The code path you are modifying happens *after* the file chooser is displayed.

[Charlie Reis, 2017-04-13 16:40:19]

Ok, thanks for the thoughts!

I'm having some trouble finding a way to test this.  The FileSelectHelper unit
test doesn't really exercise NotifyRenderFrameHostAndEnd (which needs an RFH),
and most browser tests of file selection seem to mock out FileSelectHelper.

I suppose I could move the fix into RenderFrameHostImpl directly and test it
without the FileSelectHelper, though that would require making a copy of the
array without the problematic files before passing it on in the IPC.  Any
thoughts on whether that's worth it, or if I should just land as is?

[asanka, 2017-04-13 17:21:23]

https://codereview.chromium.org/2811533002/diff/1/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2811533002/diff/1/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:325: if
(file.local_path.AsUTF8Unsafe().empty())
Shall we move this logic to a static function and call it from
NotifyRenderFrameHostAndEndAfterConversion()?

This way:
* That function can be unit tested separately.
* Filtering logic will be applied to ChromeOS's file chooser results as well.
Technically those filenames should always be valid UTF-8. But we should apply
filtering consistently.

For an end-to-end test, we could rely on something like what we did in
ppapi_filechooser_browsertest.cc where we switch out the SelectFileDialogFactory
and replace it with something that'll act like the user selected some hardcoded
filename.

It'll be trivial to add  a test to ppapi_filechooser_browsertest to test the
logic being added here, but that'd violate the principle of least surprise for
what's tested where :-)

There *has* to be an end-to-end test where we exercise FileSelectHelper based on
a renderer initiated "open" request other than PPAPI. But I couldn't find one.

[Charlie Reis, 2017-04-14 19:23:42]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 19:24:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2017-04-14 19:28:15]

Thanks!  PTAL.

https://codereview.chromium.org/2811533002/diff/1/chrome/browser/file_select_...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2811533002/diff/1/chrome/browser/file_select_...
chrome/browser/file_select_helper.cc:325: if
(file.local_path.AsUTF8Unsafe().empty())
On 2017/04/13 17:21:22, asanka wrote:
> Shall we move this logic to a static function and call it from
> NotifyRenderFrameHostAndEndAfterConversion()?
> 
> This way:
> * That function can be unit tested separately.
> * Filtering logic will be applied to ChromeOS's file chooser results as well.
> Technically those filenames should always be valid UTF-8. But we should apply
> filtering consistently.
> 
> For an end-to-end test, we could rely on something like what we did in
> ppapi_filechooser_browsertest.cc where we switch out the
SelectFileDialogFactory
> and replace it with something that'll act like the user selected some
hardcoded
> filename.
> 
> It'll be trivial to add  a test to ppapi_filechooser_browsertest to test the
> logic being added here, but that'd violate the principle of least surprise for
> what's tested where :-)
> 
> There *has* to be an end-to-end test where we exercise FileSelectHelper based
on
> a renderer initiated "open" request other than PPAPI. But I couldn't find one.

Thanks for the suggestion!  If we're going to be filtering the vector of files
after we build it, though, I might as well do it in RenderFrameHostImpl.  That
covers even more cases (including content_browsertests that mock out
FileSelectHelper), and it keeps both the fix and test closer to the code that
does the renderer kill.

I've updated the CL to do that-- can you take another look?

https://codereview.chromium.org/2811533002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2811533002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1944:
DontGrantAccessToInvalidFilePaths) {
This is based mostly on the following RestoreFileAccessForHistoryNavigation
test, which already mocks out the file chooser.  I've confirmed it causes the
renderer kill and fails without my fix, and passes with my fix.

[commit-bot: I haz the power, 2017-04-14 19:50:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 19:50:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Reis, 2017-04-14 20:00:26]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 20:00:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2017-04-14 20:06:04]

Well this is interesting.  The new test in Patch Set 2 is failing on at least
ChromeOS, cast_shell_linux and Mac, in a way that looks like AsUTF8Unsafe()
might be returning a real string but WebString still converts to an empty
string.  :(

I don't think the bug repros on those platforms because of their FileChooser
behavior, but I'd have to double check.

I wonder if we should try to better match WebString's behavior, vs just run the
test on Linux (but not ChromeOS).

[commit-bot: I haz the power, 2017-04-14 20:27:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 20:27:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Reis, 2017-04-17 18:32:50]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 18:33:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 19:00:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 19:00:50]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Reis, 2017-04-19 03:25:31]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 03:25:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 04:21:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 04:21:50]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-04-19 16:06:46]

asanka@: Can you take another look?  I think the new approach is a bit better.

I found the tests were failing on Mac/ChromeOS/Chromecast/Android because
AsUTF8Unsafe() returned non-empty but WebString::FromUTF8() returned empty, so
we'd kill the renderer anyway.  (Windows was a bit different, where even
WebString could handle the path and no kill happened in the first place.)

I decided to go ahead and grant access to the file in the browser process
regardless (since the user actually did select it), and just check in the
renderer if the WebString conversion worked.  This avoids having to
"approximate" WebString's behavior with AsUTF8Unsafe() in the browser process. 
All try jobs seem happy now, and I feel like it's a more accurate check.

[asanka, 2017-04-20 03:05:11]

LGTM. Comments are minor nits and questions. Feel free to ignore.

https://codereview.chromium.org/2811533002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2811533002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1950: // to
a WebString (on all platforms but Windows).
Interesting. WebString allows invalid UTF-16?

It's possible to create a file on NTFS that's invalid UTF-16.

https://codereview.chromium.org/2811533002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2811533002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5773:
blink::FilePathToWebString(filtered_files[i].file_path);
Nit: Why not skip test with selected_file.path.IsEmpty() here and skip?

[Charlie Reis, 2017-04-20 18:59:28]

Patchset #7 (id:120001) has been deleted

[Charlie Reis, 2017-04-20 19:00:03]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[Charlie Reis, 2017-04-20 19:00:37]

creis@chromium.org changed reviewers:
+ nick@chromium.org

[Charlie Reis, 2017-04-20 19:00:38]

Thanks!  Adding Nick, who suggested the optimization below.

https://codereview.chromium.org/2811533002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2811533002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1950: // to
a WebString (on all platforms but Windows).
On 2017/04/20 03:05:11, asanka wrote:
> Interesting. WebString allows invalid UTF-16?
> 
> It's possible to create a file on NTFS that's invalid UTF-16.

I was pretty surprised myself that WebString sent the string back up intact, but
yes, everything seems to work there without a kill.

I don't have an actual file on Windows to test with-- do you know how to create
such a file?  (I think it's probably fine to proceed with this either way, but
I'm curious to try it.)

https://codereview.chromium.org/2811533002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2811533002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5773:
blink::FilePathToWebString(filtered_files[i].file_path);
On 2017/04/20 03:05:11, asanka wrote:
> Nit: Why not skip test with selected_file.path.IsEmpty() here and skip?

Heh, I did try that at first.  :)  AFAICT, WebVector doesn't have any way to
resize or build up arrays of unknown size.  You have to declare its size in
advance and any gaps left over have default constructed objects.

However, Nick pointed out that we could only do an extra pass in the rare case
that we didn't copy over every extra element.  I've switched to that approach. 
(Thanks!)

[commit-bot: I haz the power, 2017-04-20 19:00:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 20:23:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 20:23:46]

Dry run: This issue passed the CQ dry run.

[asanka, 2017-04-20 20:24:06]

lgtm

https://codereview.chromium.org/2811533002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2811533002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1950: // to
a WebString (on all platforms but Windows).
On 2017/04/20 19:00:38, Charlie Reis wrote:
> On 2017/04/20 03:05:11, asanka wrote:
> > Interesting. WebString allows invalid UTF-16?
> > 
> > It's possible to create a file on NTFS that's invalid UTF-16.
> 
> I was pretty surprised myself that WebString sent the string back up intact,
but
> yes, everything seems to work there without a kill.
> 
> I don't have an actual file on Windows to test with-- do you know how to
create
> such a file?  (I think it's probably fine to proceed with this either way, but
> I'm curious to try it.)

------snip-----
#include<windows.h>

int main() {
  wchar_t filename[] = {
    'h', 'e', 'l', 'l', 'o', 0xd810, 0xd810, 0
  };
  HANDLE file = CreateFileW(filename, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS,
FILE_ATTRIBUTE_NORMAL, NULL);
  if (file == INVALID_HANDLE_VALUE)
    return 1;
  CloseHandle(file);
  return 0;
}
------------------

Build and run it on Windows. That should create a file with an invalid filename.

[Charlie Reis, 2017-04-20 20:36:15]

On 2017/04/20 20:24:06, asanka wrote:
> lgtm
> 
>
https://codereview.chromium.org/2811533002/diff/100001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_manager_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2811533002/diff/100001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_manager_browsertest.cc:1950: //
to
> a WebString (on all platforms but Windows).
> On 2017/04/20 19:00:38, Charlie Reis wrote:
> > On 2017/04/20 03:05:11, asanka wrote:
> > > Interesting. WebString allows invalid UTF-16?
> > > 
> > > It's possible to create a file on NTFS that's invalid UTF-16.
> > 
> > I was pretty surprised myself that WebString sent the string back up intact,
> but
> > yes, everything seems to work there without a kill.
> > 
> > I don't have an actual file on Windows to test with-- do you know how to
> create
> > such a file?  (I think it's probably fine to proceed with this either way,
but
> > I'm curious to try it.)
> 
> ------snip-----
> #include<windows.h>
> 
> int main() {
>   wchar_t filename[] = {
>     'h', 'e', 'l', 'l', 'o', 0xd810, 0xd810, 0
>   };
>   HANDLE file = CreateFileW(filename, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS,
> FILE_ATTRIBUTE_NORMAL, NULL);
>   if (file == INVALID_HANDLE_VALUE)
>     return 1;
>   CloseHandle(file);
>   return 0;
> }
> ------------------
> 
> Build and run it on Windows. That should create a file with an invalid
filename.

Thanks!  And yep, seems to work on Windows before and after my fix.

[Charlie Reis, 2017-04-20 20:36:56]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2017-04-20 20:36:56]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2811533002/#ps140001
(title: "Optimize")

[commit-bot: I haz the power, 2017-04-20 20:37:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 20:45:29]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492720616347220,
"parent_rev": "60ed9879a5363d0413f70b40569a14c7b11f42f3", "commit_rev":
"e30abe9ad8527585fc6783d8abd24842b07f67b6"}

[commit-bot: I haz the power, 2017-04-20 20:45:36]

Description was changed from

==========
Exclude files from FileSelectChooser if they can't convert to WebStrings.

BUG=703303
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Exclude files from FileSelectChooser if they can't convert to WebStrings.

BUG=703303
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2811533002
Cr-Commit-Position: refs/heads/master@{#466118}
Committed:
https://chromium.googlesource.com/chromium/src/+/e30abe9ad8527585fc6783d8abd2...
==========

[commit-bot: I haz the power, 2017-04-20 20:45:38]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/e30abe9ad8527585fc6783d8abd2...