Just for the record, I plan to add support for passing in base::File's to the network service - I don't currently plan on tackling the problem of hooking that up to the renderer.

ok, no worries I can take care of the renderer upload case once you're done

Apologies, applied the wrong component in bulk.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df42ea8147622d84866bb69572a0e612db2150a4

commit df42ea8147622d84866bb69572a0e612db2150a4
Author: Matt Menke <mmenke@chromium.org>
Date: Sat Nov 18 00:04:37 2017

Make UploadFileElementReader able to take in a base::File.

It currently only takes in paths. Taking in an already opened file will
allow it to be used in a sandboxed process with limited ability to open
files itself.

Previously, when an upload was retried, the old FileStream would be
deleted, and a new one created and the file re-opened. This won't work
with the aforementioned sandbox, so instead, rewind the file.

This also means that when there's an operation in progress when we try
to re-initialize the UploadFileElementReader, it has to wait for the
previous operation to complete. In order to achieve this, this CL
switches UploadFileElementReader to using a DoLoop pattern.

Bug:  777879 
Change-Id: I6b1f9f9ec727c588ca99427b949bc7410068b314
Reviewed-on: https://chromium-review.googlesource.com/772978
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517624}
[modify] https://crrev.com/df42ea8147622d84866bb69572a0e612db2150a4/net/base/file_stream.cc
[modify] https://crrev.com/df42ea8147622d84866bb69572a0e612db2150a4/net/base/file_stream.h
[modify] https://crrev.com/df42ea8147622d84866bb69572a0e612db2150a4/net/base/file_stream_context.cc
[modify] https://crrev.com/df42ea8147622d84866bb69572a0e612db2150a4/net/base/file_stream_context.h
[modify] https://crrev.com/df42ea8147622d84866bb69572a0e612db2150a4/net/base/upload_file_element_reader.cc
[modify] https://crrev.com/df42ea8147622d84866bb69572a0e612db2150a4/net/base/upload_file_element_reader.h
[modify] https://crrev.com/df42ea8147622d84866bb69572a0e612db2150a4/net/base/upload_file_element_reader_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aba8000cecf82449db33d5acd87b98e3cc72f357

commit aba8000cecf82449db33d5acd87b98e3cc72f357
Author: Matt Menke <mmenke@chromium.org>
Date: Tue Nov 21 21:05:12 2017

Add a DataElement::TYPE_RAW_FILE type that contains a file handle.

This, or something like it, will eventually need to replace TYPE_FILE
(Which just contains a path) in order to sandbox the network service
process.

Bug:  777879 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I8c67e828288443cbfc15aa774c1b97aa2f22ef29
Reviewed-on: https://chromium-review.googlesource.com/773238
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518390}
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/browser/loader/upload_data_stream_builder.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/common/page_state_serialization.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/common/resource_messages.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/network/DEPS
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/network/url_loader.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/network/url_loader_unittest.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/public/common/resource_request_body.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/public/common/resource_request_body.h
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/content/renderer/blob_storage/blob_transport_controller.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/storage/browser/blob/blob_data_builder.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/storage/browser/blob/blob_reader.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/storage/browser/blob/blob_storage_context.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/storage/browser/blob/view_blob_internals_job.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/storage/common/data_element.cc
[modify] https://crrev.com/aba8000cecf82449db33d5acd87b98e3cc72f357/storage/common/data_element.h

Unassigning from myself, as I've done what I planned to do.  Unclear if this will work for blobs, since it may require the file being opened for a longer period of time.  Regardless, the first CL I landed, which allows net/ to accept an open base::File as part of an upload, will probably still be useful here.

New WPT tests were added that are failing that I think are related to this bug:

### external/wpt/FileAPI/file/
NEEDBUG external/wpt/FileAPI/file//send-file-form-iso-2022-jp.tentative.html [ Crash ]
NEEDBUG external/wpt/FileAPI/file//send-file-form-utf-8.html [ Crash ]
NEEDBUG external/wpt/FileAPI/file//send-file-form-windows-1252.tentative.html [ Crash ]
NEEDBUG external/wpt/FileAPI/file//send-file-form-x-user-defined.tentative.html [ Crash ]
NEEDBUG external/wpt/FileAPI/file//send-file-form.html [ Crash ]

They are crashing at NOTREACHED here:

bool ChildProcessSecurityPolicyImpl::CanReadRequestBody(
  ...
      case ResourceRequestBody::Element::TYPE_UNKNOWN:
      default:
        // Fail safe - deny access.
        NOTREACHED();

Bot: https://ci.chromium.org/buildbot/chromium.fyi/Mojo%20Linux/7996

Output:
[31767:31767:1206/181548.095309:FATAL:child_process_security_policy_impl.cc(841)] Check failed: false. 
#0 0x00000303292c base::debug::StackTrace::StackTrace()
#1 0x00000305101c logging::LogMessage::~LogMessage()
#2 0x0000028e8ced content::ChildProcessSecurityPolicyImpl::CanReadRequestBody()
#3 0x0000028e8e8c content::ChildProcessSecurityPolicyImpl::CanReadRequestBody()
#4 0x000002a12b81 content::RenderFrameHostImpl::BeginNavigation()
#5 0x000001886fc9 content::mojom::FrameHostStubDispatch::Accept()
#6 0x0000032268d7 mojo::InterfaceEndpointClient::HandleValidatedMessage()

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/730a07d08cdc21f6e9678efb095469ff12dc7d09

commit 730a07d08cdc21f6e9678efb095469ff12dc7d09
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Dec 07 02:58:00 2017

Gardening: FileAPI WPT tests fail in NetworkService.

These were added in r522197 (from r522122) and are failing.

Bug:  777879 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ia969cea09329d027b88d6eacbd3c50f322bbe52e
NOTRY: true
TBR: kinuko
Reviewed-on: https://chromium-review.googlesource.com/812586
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522318}
[modify] https://crrev.com/730a07d08cdc21f6e9678efb095469ff12dc7d09/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

An easy option that wouldn't require the renderer to do anything would be to stop treating file uploads as special, and just do the same as it does for regular blobs. The benefit of that would be that you get more security checks than what ChildProcessSecurityPolicy does (for one, a File is supposed to be a snapshot; so the blob system records the last modified time in the browser process when a File is first created, and refuses to read the blob/file if the file changes. Current file upload code bypasses those checks).

But if you do want to treat single-file-backed-blobs separately like the current code I guess the API that gives you the file object would probably have to be on the Blob mojom interface. That way we can still make sure to do all the right checks.

(I'm guessing there is some sort of performance reason for doing all this in the first place? I.e. we don't want to be reading a file into a datapipe in one process and streaming it out onto the network in another if we could just read the file directly from disk to the network?)

Not sure what others think, but I'd be fine with streaming a file through a data pipe - I've been considering that approach for uploading files through the SimpleURLLoader interface as well.

We'd probably want to do perf testing before going with that approach, though.  It could theoretically even improve performance, though it seems unlikely (The file upload code is serialized:  Read data -> send data -> read data -> send data etc.  A mojo pipe implicitly gives us an in-memory buffer).

Erm, in memory-buffering, and potentially simultaneous reads/writes, that is.

Also because of similar issues like issue 631877 just getting the file path and then trying to reason about being allowed to upload it or not is problematic, since we can't reliably keep track of which renderers can access which files. The blob system is really the only thing that can keep track of if a renderer can access a certain file (by the renderer demonstrating that permission by being able to send messages over a Blob mojo pipe associated with that file).

That doesn't prevent is from passing a file handle, or passing a blob object that can return a file handle, though, instead of streaming the data over a Mojo pipe.

Sure, that doesn't really effect network service indeed, since it's pretty much already decided not to use the file path anyway.

Do you know if there are any perf tests around uploading files already? Just treating files as any other blobs would be really easy to implement, since it is just deleting a bunch of code, so if we already have tests it should be easy to measure the impact of such a change.

[+kapishnikov]:  Didn't you investigate QUIC upload performance?  Do you know if we have any perf tests in that area?  More interested in integrationy upload tests here, and doesn't matter if they use QUIC or not.

There are download performance tests for
iOS [QUIC/H2/Platform]: https://cs.chromium.org/chromium/src/components/cronet/ios/test/cronet_performance_test.mm
Android [H2/QUIC]: https://cs.chromium.org/chromium/src/components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java

The Android one also supports uploads. The instuctions are located here: https://cs.chromium.org/chromium/src/components/cronet/android/test/javaperftests/run.py

I mostly worked on the download path optimization on a device. ckrasic@ and ianswett@ should know more about measuring the upload performance.

I'm not sure I follow why we wouldn't want to pass the file handle to the network service? Extra copying of files seems like something we should avoid unless there's a big reason not to; it's not clear to me that we've faced that hurdle yet.

Regarding files changing from when user select to when we upload; chrome has behaved like this since launch, so I don't think this is a reason to do the extra copying.

I don't think chrome having had a security bug (and being non-spec-compliant) since launch is any argument not to fix that bug.

But yes, we can definitely pass a file handle to the network service, we'd just need a round-trip to the browser process to get that file handle first.

No extra copies are technically necessary, if pass the IPC memory buffer around (Though we aren't doing that, currently)

The reason I was considering it for SimpleURLLoader was because sending streams open for async reading and passing them between processes is a pain to get working.  I don't want to dig into all the NaCl failures I got when I tried, and async reads currently don't work with retries, either, because Windows is weird, which would require more workarounds.

Switching to sync IO is also doable, and I don't think it has the windows problems on retries that async IO has, but still seemed like a fair bit of effort.  Sounds like issue  631877 adds more prerequisites for going the file handle path as well.

@mek: can you share more info on why this is a security bug? And I didn't know this isn't spec compliant, can you share more background? FWIW I tested this in IE and Safari, and both uploaded the latest version (i.e. after I selected file but before I uploaded). Lastly, note we can still fix this even if we use file handles, i.e. we pass a handle + timestamp.

#mmenke sorry I'm not following what you wrote due to to my unfamiliarity in this area. Happy to chat in person when I'm in CAM in 2 weeks, or perhaps we can all VC (and add Tom for security pov)?

I'm in no rush here - I've been putting off dealing with file uploads for now (SimpleURLLoader provides an API to upload files, but it uses the provide-a-path DataElement API, so consumers can upload files, and we can worry about making it sandboxable at our leisure)

@jam: From https://w3c.github.io/FileAPI/#file-section "If a File object is a reference to a byte sequence originating from a file on disk, then its snapshot state should be set to the state of the file on disk at the time the File object is created." and in https://w3c.github.io/FileAPI/#dfn-error-codes the description of NotReadableError (although to be fair the reading algorithms still need cleaning up to actually properly deal with snapshot state).

Issue 710190 is a very similar problem (marked as a security bug, so not sure you can access that), where the issue is that FileReader lets you reload a file after it changes. But not much point in fixing that if you can still upload the modified file at will (as is currently the case).

I guess however this is solved, a related problem is what to do with PageState serialization (where I believe for back/forward navigation the whole page state, including ResourceRequestBody is serialized into a string, that string passed from renderer to browser, and then later the files/data to upload are parsed back out of the string).

That codepath I believe is already broken with network service if somebody somehow manages to get a non-file backed blob as body for a navigation (fortunately that is really hard to get), as WriteResourceRequestBody (in content/common/page_state_serialization.cc) would just DCHECK on the unknown DataElement types that would be the result of that. But doing anything like what's being discussed here for files would make that even more broken (I'm not sure what exactly there is supposed/expected to work though, I'm not really sure if/when we'd actually try to re-upload files on history navigation).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be8ffb1b1c877bf1ef29b8a0044cc13b81b130c5

commit be8ffb1b1c877bf1ef29b8a0044cc13b81b130c5
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Thu Mar 15 17:12:22 2018

Don't crash if attempting to serialize TYPE_DATA_PIPE bodies.

Actually somehow serializing these is going to be more difficult, but
this will at least help to not crash for tests that hit this codepath.

On of several changes towards making uploading blobs with network service work.

Tested in a separate CL because multiple unrelated bugfixes are needed to make this
all work and testable (https://chromium-review.googlesource.com/c/chromium/src/+/963008).

(see also some of the discussion in https://groups.google.com/a/chromium.org/d/msg/network-service-dev/Jby2gkdDhi8/3GURbFqRAAAJ
where the problem of serializing uploaded blobs in history was mentioned)

Bug:  821878 ,  777879 ,  761117 
Change-Id: Id60791d26bc2f53c79290305cd25f7a95f247a85
Reviewed-on: https://chromium-review.googlesource.com/963015
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543413}
[modify] https://crrev.com/be8ffb1b1c877bf1ef29b8a0044cc13b81b130c5/content/common/page_state_serialization.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afec04f86cd29b51c3a92e4af85e7f8db7e12324

commit afec04f86cd29b51c3a92e4af85e7f8db7e12324
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Fri Mar 16 20:36:20 2018

Reenable some layout tests that are fixed after recent changes.

Bug:  777879 ,  821878 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I1bbc4db1fc665ab1844780c4d689d7a8ab0d044c
Reviewed-on: https://chromium-review.googlesource.com/967111
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543812}
[modify] https://crrev.com/afec04f86cd29b51c3a92e4af85e7f8db7e12324/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService
[modify] https://crrev.com/afec04f86cd29b51c3a92e4af85e7f8db7e12324/third_party/WebKit/LayoutTests/TestExpectations

this related to sandboxing and not blocking canary.

John:  This is a duplicate of  issue 845612 , right?

Yep, thanks.