Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503813002:
Fix and refactor downloaded file handling in the loading stack (Closed)
|
Created:
4 years, 1 month ago by tzik Modified:
4 years ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, kinuko+watch, jsbell+serviceworker_chromium.org, tzik, jam, abarth-chromium, darin-cc_chromium.org, loading-reviews_chromium.org, blink-worker-reviews_chromium.org, mlamouri+watch-content_chromium.org, Randy Smith (Not in Mondays), nhiroki, michaeln, shimazu+serviceworker_chromium.org, serviceworker-reviews, Aaron Boodman, kinuko+serviceworker, horo+watch_chromium.org, darin (slow to review), mmenke, asanka Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix and refactor downloaded file handling in the loading stack
In download_to_file mode of resource loading, the resource handler needs
to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the
file is created, and needs to call UnregisterDownloadedTempFile after the
client handles the file. Otherwise, the file is deleted before the client
uses it.
However, MojoAsyncResourceHandler doesn't call them, and that causes
layout test failure around XHR with responseType = 'blob'. This CL fixes
that by adding the RDTF and UDTF pair, and refactors similar code in
AsyncResourceHandler to share the same lifetime management of the
downloaded file in ResourceDispatcherHostImpl.
BUG=603396, 659917
Committed: https://crrev.com/47a9f9bc844919d1bcdbf27a2a28256b1f40dd06
Cr-Commit-Position: refs/heads/master@{#435551}
Patch Set 1 #Patch Set 2 : +test case #Patch Set 3 : update test expectation #Patch Set 4 : revert an unrelated change #
Total comments: 8
Patch Set 5 : -Release #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : +RunUntilIdle #
Total comments: 15
Patch Set 8 : Binding -> StrongBinding #Patch Set 9 : . #
Total comments: 6
Patch Set 10 : move to dtor #Messages
Total messages: 69 (50 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix and refactor downloaded file handling in the loading stack BUG= ========== to ========== Fix and refactor downloaded file handling in the loading stack This CL refactors downloaded file handling in ResourceDispatcherHostImpl and AsyncResourceHandler, then fixes its handling in MojoAsyncResourceHandler. BUG= ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Description was changed from ========== Fix and refactor downloaded file handling in the loading stack This CL refactors downloaded file handling in ResourceDispatcherHostImpl and AsyncResourceHandler, then fixes its handling in MojoAsyncResourceHandler. BUG= ========== to ========== Fix and refactor downloaded file handling in the loading stack This CL refactors downloaded file handling in ResourceDispatcherHostImpl and AsyncResourceHandler, then fixes its handling in MojoAsyncResourceHandler. BUG= ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tzik@chromium.org changed reviewers: + mmenke@chromium.org
tzik@chromium.org changed reviewers: + nhiroki@chromium.org
tzik@chromium.org changed reviewers: + dcheng@chromium.org
tzik@chromium.org changed reviewers: + yhirano@chromium.org
PTAL to:
yhirano: overall
nhiroki: //content/{browser,renderer}/service_worker/
mmenke: //content/browser/loader/
dcheng: //content/common/url_loader.mojom
avi: //content/browser/BUILD.gn, //content/child/
https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... File content/browser/loader/downloaded_temp_file_impl.cc (right): https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... content/browser/loader/downloaded_temp_file_impl.cc:32: DownloadedTempFileImpl::CreateInterfacePtrAndBind() { How about setting an connection_error_handler calling Release()? Is it possible to remove Release() from the mojom interface if we rely on the mechanism? https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... content/browser/loader/downloaded_temp_file_impl.h:39: const base::FilePath& path() const { return path_; } Not used https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... content/browser/loader/downloaded_temp_file_impl.h:50: base::FilePath path_; ditto
What does this CL do? I'm unable to figure it out from the description. Is this an alternative for RedirectToFileImpl, or is this changing how actual downloads work? Please update the description. https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... content/browser/loader/downloaded_temp_file_impl.h:23: class CONTENT_EXPORT DownloadedTempFileImpl final Docs?
CCing Asanka FHI, though based on the filenames, I don't think this has to do with what I think of as the downloads system.
service_worker lgtm Can you elaborate on what this CL will fix and/or update the BUG= line in the CL description?
Description was changed from ========== Fix and refactor downloaded file handling in the loading stack This CL refactors downloaded file handling in ResourceDispatcherHostImpl and AsyncResourceHandler, then fixes its handling in MojoAsyncResourceHandler. BUG= ========== to ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG= ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... File content/browser/loader/downloaded_temp_file_impl.cc (right): https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... content/browser/loader/downloaded_temp_file_impl.cc:32: DownloadedTempFileImpl::CreateInterfacePtrAndBind() { On 2016/11/15 13:15:26, yhirano wrote: > How about setting an connection_error_handler calling Release()? Is it possible > to remove Release() from the mojom interface if we rely on the mechanism? SG. Updated. https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... content/browser/loader/downloaded_temp_file_impl.h:23: class CONTENT_EXPORT DownloadedTempFileImpl final On 2016/11/15 15:59:05, mmenke wrote: > Docs? Done. https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... content/browser/loader/downloaded_temp_file_impl.h:39: const base::FilePath& path() const { return path_; } On 2016/11/15 13:15:26, yhirano wrote: > Not used Done. https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/... content/browser/loader/downloaded_temp_file_impl.h:50: base::FilePath path_; On 2016/11/15 13:15:26, yhirano wrote: > ditto Done.
lgtm https://codereview.chromium.org/2503813002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2503813002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:3344: downloaded_file_ptr = nullptr; It may be good to call base::RunLoop().RunUntilIdle() here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2503813002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2503813002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:3344: downloaded_file_ptr = nullptr; On 2016/11/16 10:43:39, yhirano wrote: > It may be good to call base::RunLoop().RunUntilIdle() here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojom lgtm with comments addressed https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/downloaded_temp_file_impl.h:25: // outlives URLLoader until the client destroyes the interface. Nit: destroys https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1776: scoped_refptr<storage::ShareableFileReference> reference = Are lines 1774-1776 still needed? https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:3447: deletable_file = NULL; Nit: nullptr https://codereview.chromium.org/2503813002/diff/140001/content/common/url_loa... File content/common/url_loader.mojom (right): https://codereview.chromium.org/2503813002/diff/140001/content/common/url_loa... content/common/url_loader.mojom:28: // The downloaded file keeps alive until the instance is destroyed. Empty interfaces are a bit unusual. My main feedback is it would be useful for the interface to explicitly describe why it's empty (it's an opaque handle), which way the handle is passed (browser -> child), and explicitly what happens when we close the pipe (the temporary file is deleted... I think?). Strawman suggestion: // Opaque handle passed from the browser process to a child // process to manage the lifetime of temporary files used for // download_to_file resource loading. When the message pipe // for this interface is closed, the browser process will // clean up the corresponding temporary file. https://codereview.chromium.org/2503813002/diff/140001/content/common/url_loa... content/common/url_loader.mojom:33: // Called when the response head is received. |downloaded_file| is non-null on Nit: on => in
https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/downloaded_temp_file_impl.h:25: // outlives URLLoader until the client destroyes the interface. "outlives until" isn't grammatically correct. Maybe ".... outlives URLLoader, and is torn down when client destroys" or "and lives until the client destroys the interface, slightly after the URLLoader is destroyed." https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/downloaded_temp_file_impl.h:25: // outlives URLLoader until the client destroyes the interface. "URLLoader"? This is also used on the non-Mojo path, which doesn't use a URLLoader. https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/downloaded_temp_file_impl.h:38: int request_id() const { return request_id_; } Not actually used? (Well, it's used in RDH, but nothing is done with it) https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1773: std::unique_ptr<DownloadedTempFileImpl> downloaded_file) { Do we even need to change all this? Would it be better to just make DownloadedTempFileImpl call ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFile itself, and rely on the connection_error_handler to ensure teardown (Or just let the Mojo object in the other process own it directly), and leave the old code alone? Then when we switch over to mojo, we can remove all this stuff from the RDH as well.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/downloaded_temp_file_impl.h:25: // outlives URLLoader until the client destroyes the interface. On 2016/11/17 16:59:33, mmenke wrote: > "outlives until" isn't grammatically correct. Maybe ".... outlives URLLoader, > and is torn down when client destroys" or "and lives until the client destroys > the interface, slightly after the URLLoader is destroyed." Done. https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1773: std::unique_ptr<DownloadedTempFileImpl> downloaded_file) { On 2016/11/17 16:59:33, mmenke wrote: > Do we even need to change all this? Would it be better to just make > DownloadedTempFileImpl call > ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFile itself, and rely on > the connection_error_handler to ensure teardown (Or just let the Mojo object in > the other process own it directly), and leave the old code alone? Then when we > switch over to mojo, we can remove all this stuff from the RDH as well. OK, let me take the second: Let the Mojo object in the other process own it. https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1776: scoped_refptr<storage::ShareableFileReference> reference = On 2016/11/17 11:16:02, dcheng wrote: > Are lines 1774-1776 still needed? Reverted. https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:3447: deletable_file = NULL; On 2016/11/17 11:16:02, dcheng wrote: > Nit: nullptr Done. https://codereview.chromium.org/2503813002/diff/140001/content/common/url_loa... File content/common/url_loader.mojom (right): https://codereview.chromium.org/2503813002/diff/140001/content/common/url_loa... content/common/url_loader.mojom:28: // The downloaded file keeps alive until the instance is destroyed. On 2016/11/17 11:16:02, dcheng wrote: > Empty interfaces are a bit unusual. My main feedback is it would be useful for > the interface to explicitly describe why it's empty (it's an opaque handle), > which way the handle is passed (browser -> child), and explicitly what happens > when we close the pipe (the temporary file is deleted... I think?). Strawman > suggestion: > > // Opaque handle passed from the browser process to a child > // process to manage the lifetime of temporary files used for > // download_to_file resource loading. When the message pipe > // for this interface is closed, the browser process will > // clean up the corresponding temporary file. Done. https://codereview.chromium.org/2503813002/diff/140001/content/common/url_loa... content/common/url_loader.mojom:33: // Called when the response head is received. |downloaded_file| is non-null on On 2016/11/17 11:16:02, dcheng wrote: > Nit: on => in Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good! Just a couple comments. https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... File content/browser/loader/downloaded_temp_file_impl.cc (right): https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... content/browser/loader/downloaded_temp_file_impl.cc:13: void OnConnectionError(int child_id, int request_id) { Can we just make this a member of DownloadedTempFileImpl? Seems weird to have it here...Or could we just move it to the destructor instead? IF it's a strong binding, we'll be destroyed on connection error, anyways, right? https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:3439: // may take additional blob references.) I don't see an API for a child process to create additional DownloadedTempFilePtrs for the same file? https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:3452: EXPECT_FALSE(base::PathExists(file_path)); Add an EXPECT_TRUE(base::PathExists(file_path)); when the file is acutally created?
https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... File content/browser/loader/downloaded_temp_file_impl.cc (right): https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... content/browser/loader/downloaded_temp_file_impl.cc:13: void OnConnectionError(int child_id, int request_id) { On 2016/11/22 16:19:33, mmenke wrote: > Can we just make this a member of DownloadedTempFileImpl? Seems weird to have > it here...Or could we just move it to the destructor instead? IF it's a strong > binding, we'll be destroyed on connection error, anyways, right? Done. https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:3439: // may take additional blob references.) On 2016/11/22 16:19:34, mmenke wrote: > I don't see an API for a child process to create additional > DownloadedTempFilePtrs for the same file? "another reference to the file" means a ShareableFileReference rather than a DownloadedTempFilePtr. That's a bit far from these code, but the renderer may take a ShareableFileReference by creating a blob from the responding file path. E.g. XMLHttpRequest::createBlobDataHandleFromResponse() makes a blob from the file path, and that eventually calls BlobDataBuilder::AppendFile(), that holds a ShareableFileReference in a BlobDataItem. https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:3452: EXPECT_FALSE(base::PathExists(file_path)); On 2016/11/22 16:19:34, mmenke wrote: > Add an EXPECT_TRUE(base::PathExists(file_path)); when the file is acutally > created? Done.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
loader/ LGTM!
tzik@chromium.org changed reviewers: + avi@chromium.org
+avi as a //content OWNER. PTAL.
lgtm stamp You really need a BUG=
Description was changed from ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG= ========== to ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG=603396 ==========
Description was changed from ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG=603396 ========== to ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG=603396, 659917 ==========
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org, yhirano@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2503813002/#ps200001 (title: "move to dtor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480559364210100,
"parent_rev": "3b940ec97956a830f2c1dfae2e851090e60d09f6", "commit_rev":
"a4ee1079c1b4dbe9b902339a573d4821b8d514fa"}
Message was sent while issue was closed.
Description was changed from ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG=603396, 659917 ========== to ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG=603396, 659917 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG=603396, 659917 ========== to ========== Fix and refactor downloaded file handling in the loading stack In download_to_file mode of resource loading, the resource handler needs to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the file is created, and needs to call UnregisterDownloadedTempFile after the client handles the file. Otherwise, the file is deleted before the client uses it. However, MojoAsyncResourceHandler doesn't call them, and that causes layout test failure around XHR with responseType = 'blob'. This CL fixes that by adding the RDTF and UDTF pair, and refactors similar code in AsyncResourceHandler to share the same lifetime management of the downloaded file in ResourceDispatcherHostImpl. BUG=603396, 659917 Committed: https://crrev.com/47a9f9bc844919d1bcdbf27a2a28256b1f40dd06 Cr-Commit-Position: refs/heads/master@{#435551} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/47a9f9bc844919d1bcdbf27a2a28256b1f40dd06 Cr-Commit-Position: refs/heads/master@{#435551} |
