Uploading a blob with network service is very much broken |
||||
Issue descriptionAs it turns out we don't have any test that try to submit a blob through a form. While writing such a test I discovered that (as I already suspected) this is completely and utterly broken with network service enabled because of only half implemented support for DataElement::TYPE_DATA_PIPE (or any of the other recently added DataElement types for that matter).
,
Mar 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/97b0b8a42a2f7d9c205a042f4133867e2adeb6ac commit 97b0b8a42a2f7d9c205a042f4133867e2adeb6ac Author: Marijn Kruisselbrink <mek@chromium.org> Date: Wed Mar 14 21:05:31 2018 Support TYPE_DATA_PIPE in CanReadRequestBody. 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). Bug: 821878 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I33812ac556029066196b3fcbdb54d6bdbc041a86 Reviewed-on: https://chromium-review.googlesource.com/963007 Reviewed-by: Tom Sepez <tsepez@chromium.org> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/master@{#543188} [modify] https://crrev.com/97b0b8a42a2f7d9c205a042f4133867e2adeb6ac/content/browser/child_process_security_policy_impl.cc
,
Mar 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0689f9414a616a94069ffbd3fef2d5f588960c8e commit 0689f9414a616a94069ffbd3fef2d5f588960c8e Author: Marijn Kruisselbrink <mek@chromium.org> Date: Wed Mar 14 21:45:04 2018 Check types before checking body size in InspectorNetworkAgent code. EncodedFormData::SizeInBytes doesn't support all possible types (in particular it DCHECKs on kDataPipe), so by swapping the order of checks we make sure things actually work correctly. On of several changes towards making uploading blobs with network service work. Bug: 821878 Change-Id: I64313bcc390756de6ad0be091d0ab32e9b78ae44 Reviewed-on: https://chromium-review.googlesource.com/962833 Reviewed-by: Andrey Kosyakov <caseq@chromium.org> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/master@{#543207} [modify] https://crrev.com/0689f9414a616a94069ffbd3fef2d5f588960c8e/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
,
Mar 15 2018
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
,
Mar 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1214a9117eabec916388f6ff0ac95886d0b174c4 commit 1214a9117eabec916388f6ff0ac95886d0b174c4 Author: Marijn Kruisselbrink <mek@chromium.org> Date: Fri Mar 16 06:22:50 2018 Change DataElement to hold DataPipeGetterPtrInfo instead of Ptr. Holding an InterfacePtr makes the struct weirdly sequence bound. Also get rid of const casts that invalidate a const DataElement and replace them with cloning the DataPipeGetter. This is needed because the same ResourceRequestBody is sent to multiple mojo methods, and thus the param traits should not modify the DataElements. 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). Bug: 821878 , 761117 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: Ic1d63e6f0f7112cf37c75526c561c524a9c15201 Reviewed-on: https://chromium-review.googlesource.com/963085 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#543639} [modify] https://crrev.com/1214a9117eabec916388f6ff0ac95886d0b174c4/content/common/service_worker/service_worker_loader_helpers.cc [modify] https://crrev.com/1214a9117eabec916388f6ff0ac95886d0b174c4/content/renderer/loader/web_url_request_util.cc [modify] https://crrev.com/1214a9117eabec916388f6ff0ac95886d0b174c4/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc [modify] https://crrev.com/1214a9117eabec916388f6ff0ac95886d0b174c4/services/network/public/cpp/data_element.cc [modify] https://crrev.com/1214a9117eabec916388f6ff0ac95886d0b174c4/services/network/public/cpp/data_element.h [modify] https://crrev.com/1214a9117eabec916388f6ff0ac95886d0b174c4/services/network/public/cpp/network_param_ipc_traits.cc [modify] https://crrev.com/1214a9117eabec916388f6ff0ac95886d0b174c4/services/network/public/cpp/simple_url_loader_unittest.cc [modify] https://crrev.com/1214a9117eabec916388f6ff0ac95886d0b174c4/services/network/url_loader.cc
,
Mar 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43681139ff03dfbcab20b1a79ddb0ac5a9a992ae commit 43681139ff03dfbcab20b1a79ddb0ac5a9a992ae Author: Marijn Kruisselbrink <mek@chromium.org> Date: Fri Mar 16 19:41:44 2018 Remove NOTREACHED() from EncodedFormData::SizeInBytes. The comment incorrectly asserted that the method was only called by PingLoader. In fact the method is called for every request by devtools. So instead just ignore data pipes in the size calculation for now, just like blobs without blob data handle are ignored. If we do want actual size information for these elements at some point we'd probably just want to add an explicit size field along with the DataPipeGetter, just like network::DataElement already does for almost every other type of element. Bug: 821878 Change-Id: I6a64aaeffbcf0e22bf90bcf44c38367bf682cf3e Reviewed-on: https://chromium-review.googlesource.com/964612 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/master@{#543784} [modify] https://crrev.com/43681139ff03dfbcab20b1a79ddb0ac5a9a992ae/third_party/WebKit/Source/platform/network/EncodedFormData.cpp
,
Mar 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4128665dd79d4333640aca24354dc6f457f1ba2f commit 4128665dd79d4333640aca24354dc6f457f1ba2f Author: Marijn Kruisselbrink <mek@chromium.org> Date: Fri Mar 16 19:56:33 2018 Use existing BlobPtr when possible when converting blob to DataPipeGetter. EncodedFormData already (optionally) contains a blink::BlobDataHandle, so rather than looking up the same blob again by uuid just use that handle if we have it. This should ensure no races are possible between a blob being dereferenced (in blink) and the blob being converted into a DataPipeGetter (as looking up the blob by UUID is an async operation). Bug: 740744, 821878 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: Ic8464b778ec91d10fdbd7688e2b680b9464fe1dc Reviewed-on: https://chromium-review.googlesource.com/964716 Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#543794} [modify] https://crrev.com/4128665dd79d4333640aca24354dc6f457f1ba2f/content/renderer/loader/web_url_request_util.cc [modify] https://crrev.com/4128665dd79d4333640aca24354dc6f457f1ba2f/third_party/WebKit/Source/platform/exported/WebHTTPBody.cpp [modify] https://crrev.com/4128665dd79d4333640aca24354dc6f457f1ba2f/third_party/WebKit/public/platform/WebHTTPBody.h
,
Mar 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d3e3a388e54f0163a0c02838b0028f491142f797 commit d3e3a388e54f0163a0c02838b0028f491142f797 Author: Marijn Kruisselbrink <mek@chromium.org> Date: Fri Mar 16 20:28:36 2018 [Blobs] Add AsDataPipeGetter method to Blob mojo interface. This makes it simpler to create a DataPipeGetter out of a Blob, and also ensures that a blob that is used as the body of a request can still be read even after the process that created the DataPipeGetter (i.e. the renderer that initiated the fetch) dies. Bug: 821878 , 761117 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I519a6061900f4cb29b786b8445dc14f034959e27 Reviewed-on: https://chromium-review.googlesource.com/963088 Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#543807} [modify] https://crrev.com/d3e3a388e54f0163a0c02838b0028f491142f797/content/renderer/loader/web_url_request_util.cc [modify] https://crrev.com/d3e3a388e54f0163a0c02838b0028f491142f797/storage/browser/blob/blob_impl.cc [modify] https://crrev.com/d3e3a388e54f0163a0c02838b0028f491142f797/storage/browser/blob/blob_impl.h [modify] https://crrev.com/d3e3a388e54f0163a0c02838b0028f491142f797/storage/browser/blob/blob_registry_impl_unittest.cc [modify] https://crrev.com/d3e3a388e54f0163a0c02838b0028f491142f797/third_party/WebKit/Source/platform/blob/testing/FakeBlob.cpp [modify] https://crrev.com/d3e3a388e54f0163a0c02838b0028f491142f797/third_party/WebKit/Source/platform/blob/testing/FakeBlob.h [modify] https://crrev.com/d3e3a388e54f0163a0c02838b0028f491142f797/third_party/WebKit/public/mojom/BUILD.gn [modify] https://crrev.com/d3e3a388e54f0163a0c02838b0028f491142f797/third_party/WebKit/public/mojom/blob/blob.mojom
,
Mar 16 2018
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
,
May 15 2018
,
May 18 2018
,
May 31 2018
Marking this as fixed. There are still some open questions (for example around how file uploads are treated with respect to the blob system, and what to do about history serialization of upload data), but those are really separate questions, and aren't exactly new issues that don't also exist in different forms without network service. |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Mar 14 2018