This can be tested with the SBNavigationObserverBrowserTest.DownloadViaHTML5FileApi test.

It looks like FileSystemBrowserTest.CreateTest is the test that *should* catch this failure, but does not. This should fail:


```
out/Debug/content_browsertests --enable-features=NetworkService --gtest_filter=':FileSystemBrowserTest.CreateTest:'
```

but it passes with the network service enabled *and* disabled. However if run by Chrome as so:

```
out/Debug/chrome --enable-features=NetworkService http://localhost:8000/content/test/data/fileapi/create_test.html
```

Then the test hangs.

Hmm, this overrides a bunch of unusual delegate methods, but perhaps LOAD_DO_NOT_SEND_AUTH_DATA will cover most of those?

morlovich@ I think it isn't correct to make this change and that I was wrong in my understanding of FileWriterDelegate. I was focused on it's use of net::URLRequest which is now generally used in the network service. However I think that it is correct to use it in the browser process for filesystem requests when the network service is enabled. It's still a bit confusing to me, but I do think it's correct.

Oh, I see what you mean. Yeah, network service isn't going to handle file:// URLs --- it's supposed to eventually be sandboxed, after all --- and it's using URLRequest, but not URLFetcher.... 

It does seem to be using a URLRequestContext, which is then passed to BlobProtocolHandler:
https://cs.chromium.org/chromium/src/storage/browser/fileapi/file_system_operation_runner.cc?rcl=d72db906acff2772870b45f08f5425a8220ed6c7&l=269
and that would probably break down. 

I think there is a BlobURLLoaderFactory or something, though? 

In this bug FileSystem* refers to the filesystem:// scheme and not the file:// scheme. But both are still handled in the browser process with the network service enabled. Well, when I land http://crrev.com/c/1127148 then filesystem:// will be properly handled. I don't know the Blob code well enough to comment on your last question. In the browser process requests for file://, filesystem://, and blob:// schemes still go through net::URLRequest, which confused me initially - hence this bug. mek@ and dmurph@ know more about blobs than I do.

I did a bit more research into this and here's what I've learned. net::URLRequestContext is the old (pre network service) implementation which works but is being phased out. Most request types using it would completely fail when the network service is enabled, but because the request for, and the request handling of, blobs are done all within the same browser process they still work even though they use net::URLRequest.

FileSystem reading/writing (with the network service enabled) works in the content_browsertests (where the BrowserContext is a ShellBrowserContext), but does not work in the browser_tests (where the BrowserContext is a Profile), or within Chrome itself.

At present FileAPIMessageFilter is given a net::URLRequestContext which is used for all blob requests. This should either be switched to a BlobURLLoaderFactory, or a SimpleURLLoaderFactory.

Note that ideally this shouldn't use any kind of URLLoader/URLLoaderFactory, but should just be reading the blob directly (preferably through a BlobPtr and its mojom interface, but there are other alternatives that could work as well).

Also there is a very similar case in indexeddb (where IndexedDBBackingStore::WriteBlobFile also tries to read a blob by creating a blob URL and then using a net::URLRequest to read that URL).

Filed 868462 for the IDB case

I just looked through this code.

If there are architectural improvements (i.e. not using URLFetcher/SimpleURLLoader for this load) then I'd prefer that if they're not small efforts (e.g. can be fixed in next two weeks) and done by the storage team, they're worked on orthogonally.

For this specific issue, it seems that yes it can be converted from net::URLFetcher to network::SimpleURLLoader. While the network service itself doesn't know about blobs, we have a URLLoaderFactory implementation for blobs in storage/browser/blob/blob_url_loader_factory.h.

The proper fix (or at least something better than using SimpleURLLoader) should really be no extra work, so unless somebody objects I'll just go ahead and do that.

Thanks!

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

commit a780469e2f2de600512c592c36041002dd697a33
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Thu Aug 09 04:59:33 2018

[FileSystem]: Rewrite FileWriterDelegate using BlobReader rather than URLRequest.

Not perfect, since this really should be going through the mojo interface
for dealing with blobs, but a lot better than going through blob URLs.

Tbr: reillyg@chromium.org
Bug:  859594 ,  868462 , 804546
Change-Id: Ifd7cd11b7bf4432eff41bdef00edd079d31e49c6
Reviewed-on: https://chromium-review.googlesource.com/1164523
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581790}
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/chrome/browser/extensions/app_data_migrator_unittest.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/chrome/browser/sync_file_system/local/canned_syncable_file_system.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/chrome/browser/sync_file_system/local/canned_syncable_file_system.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/chrome/browser/sync_file_system/local/local_file_change_tracker_unittest.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/chrome/browser/sync_file_system/local/syncable_file_operation_runner_unittest.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/chrome/browser/sync_file_system/local/syncable_file_system_operation.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/chrome/browser/sync_file_system/local/syncable_file_system_operation.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/fileapi/fileapi_message_filter.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/fileapi/fileapi_message_filter.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/fileapi/fileapi_message_filter_unittest.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/database_impl.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_backing_store.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_backing_store.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_backing_store_unittest.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_blob_info.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_blob_info.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_callbacks.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_database.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_database.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_database_unittest.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_fake_backing_store.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/indexed_db/indexed_db_fake_backing_store.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_system_operation.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_system_operation_impl.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_system_operation_impl.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_system_operation_impl_write_unittest.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_system_operation_runner.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_system_operation_runner.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_writer_delegate.cc
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_writer_delegate.h
[modify] https://crrev.com/a780469e2f2de600512c592c36041002dd697a33/storage/browser/fileapi/file_writer_delegate_unittest.cc

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

commit 4c999b6344a3a2d1bd8dbdd09f83d4499453d336
Author: John Abd-El-Malek <jam@chromium.org>
Date: Fri Aug 10 20:58:37 2018

Enable some more passing browser tests with network service.

Bug:  797292 ,  859594 

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Id4e90f0ebdf5ac172b12c92d80b54348125bc883
Reviewed-on: https://chromium-review.googlesource.com/1171556
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582328}
[modify] https://crrev.com/4c999b6344a3a2d1bd8dbdd09f83d4499453d336/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

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

commit e0066b0344ad7db43a2c4c2f7629c49e47ce26a6
Author: John Abd-El-Malek <jam@chromium.org>
Date: Wed Aug 15 17:18:12 2018

Reenable filesystem extensions tests with network service.

Bug:  859594 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I5026f51b6bc005215827de6d8d4a15b870b42cf4
Reviewed-on: https://chromium-review.googlesource.com/1175981
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583295}
[modify] https://crrev.com/e0066b0344ad7db43a2c4c2f7629c49e47ce26a6/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

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

commit aa4eef324f4f5959f39c02f5a837601d2db2005b
Author: Matt Mueller <mattm@chromium.org>
Date: Tue Aug 21 17:26:39 2018

re-enable SBNavigationObserverBrowserTest.DownloadViaHTML5FileApi on network_browser_tests

It was accidentally re-disabled in a bad rebase in
https://chromium.googlesource.com/chromium/src/+/3787780199f8ef7bfa5e977aab8180766b10ddad

Bug:  859594 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I2dd6b15dba5639657ddc4deadc4afe6468cd529a
Reviewed-on: https://chromium-review.googlesource.com/1182309
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584817}
[modify] https://crrev.com/aa4eef324f4f5959f39c02f5a837601d2db2005b/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter