Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(175)

Issue 2503813002: Fix and refactor downloaded file handling in the loading stack (Closed)

Created:
3 years, 9 months ago by tzik
Modified:
3 years, 8 months 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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -19 lines) Patch
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/loader/DEPS View 1 2 3 4 5 6 7 3 chunks +9 lines, -0 lines 0 comments Download
A content/browser/loader/downloaded_temp_file_impl.h View 1 2 3 4 5 6 7 8 9 1 chunk +35 lines, -0 lines 0 comments Download
A content/browser/loader/downloaded_temp_file_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +30 lines, -0 lines 0 comments Download
M content/browser/loader/mojo_async_resource_handler.cc View 1 2 3 4 5 6 7 2 chunks +12 lines, -1 line 0 comments Download
M content/browser/loader/resource_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +53 lines, -1 line 0 comments Download
M content/browser/loader/test_url_loader_client.h View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/loader/test_url_loader_client.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_fetch_dispatcher.cc View 1 2 3 4 5 1 chunk +4 lines, -2 lines 0 comments Download
M content/child/resource_dispatcher.cc View 1 2 3 4 5 2 chunks +5 lines, -1 line 0 comments Download
M content/common/url_loader.mojom View 1 2 3 4 5 6 7 8 1 chunk +11 lines, -2 lines 0 comments Download
M content/renderer/service_worker/service_worker_context_client.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -9 lines 0 comments Download

Messages

Total messages: 69 (50 generated)
tzik
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/
3 years, 9 months ago (2016-11-15 11:39:59 UTC) #18
yhirano
https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/downloaded_temp_file_impl.cc File content/browser/loader/downloaded_temp_file_impl.cc (right): https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/downloaded_temp_file_impl.cc#newcode32 content/browser/loader/downloaded_temp_file_impl.cc:32: DownloadedTempFileImpl::CreateInterfacePtrAndBind() { How about setting an connection_error_handler calling Release()? ...
3 years, 9 months ago (2016-11-15 13:15:26 UTC) #19
mmenke
What does this CL do? I'm unable to figure it out from the description. Is ...
3 years, 9 months ago (2016-11-15 15:59:06 UTC) #20
Randy Smith (Not in Mondays)
CCing Asanka FHI, though based on the filenames, I don't think this has to do ...
3 years, 9 months ago (2016-11-15 16:09:29 UTC) #21
nhiroki
service_worker lgtm Can you elaborate on what this CL will fix and/or update the BUG= ...
3 years, 9 months ago (2016-11-16 08:10:14 UTC) #22
tzik
https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/downloaded_temp_file_impl.cc File content/browser/loader/downloaded_temp_file_impl.cc (right): https://codereview.chromium.org/2503813002/diff/80001/content/browser/loader/downloaded_temp_file_impl.cc#newcode32 content/browser/loader/downloaded_temp_file_impl.cc:32: DownloadedTempFileImpl::CreateInterfacePtrAndBind() { On 2016/11/15 13:15:26, yhirano wrote: > How ...
3 years, 9 months ago (2016-11-16 09:14:56 UTC) #26
yhirano
lgtm https://codereview.chromium.org/2503813002/diff/100001/content/browser/loader/resource_dispatcher_host_unittest.cc File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2503813002/diff/100001/content/browser/loader/resource_dispatcher_host_unittest.cc#newcode3344 content/browser/loader/resource_dispatcher_host_unittest.cc:3344: downloaded_file_ptr = nullptr; It may be good to ...
3 years, 8 months ago (2016-11-16 10:43:40 UTC) #27
tzik
https://codereview.chromium.org/2503813002/diff/100001/content/browser/loader/resource_dispatcher_host_unittest.cc File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2503813002/diff/100001/content/browser/loader/resource_dispatcher_host_unittest.cc#newcode3344 content/browser/loader/resource_dispatcher_host_unittest.cc:3344: downloaded_file_ptr = nullptr; On 2016/11/16 10:43:39, yhirano wrote: > ...
3 years, 8 months ago (2016-11-17 08:42:50 UTC) #36
dcheng
mojom lgtm with comments addressed https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader/downloaded_temp_file_impl.h File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader/downloaded_temp_file_impl.h#newcode25 content/browser/loader/downloaded_temp_file_impl.h:25: // outlives URLLoader until ...
3 years, 8 months ago (2016-11-17 11:16:02 UTC) #39
mmenke
https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader/downloaded_temp_file_impl.h File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader/downloaded_temp_file_impl.h#newcode25 content/browser/loader/downloaded_temp_file_impl.h:25: // outlives URLLoader until the client destroyes the interface. ...
3 years, 8 months ago (2016-11-17 16:59:33 UTC) #40
tzik
https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader/downloaded_temp_file_impl.h File content/browser/loader/downloaded_temp_file_impl.h (right): https://codereview.chromium.org/2503813002/diff/140001/content/browser/loader/downloaded_temp_file_impl.h#newcode25 content/browser/loader/downloaded_temp_file_impl.h:25: // outlives URLLoader until the client destroyes the interface. ...
3 years, 8 months ago (2016-11-22 13:54:45 UTC) #47
mmenke
Looks good! Just a couple comments. https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader/downloaded_temp_file_impl.cc File content/browser/loader/downloaded_temp_file_impl.cc (right): https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader/downloaded_temp_file_impl.cc#newcode13 content/browser/loader/downloaded_temp_file_impl.cc:13: void OnConnectionError(int child_id, ...
3 years, 8 months ago (2016-11-22 16:19:34 UTC) #50
tzik
https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader/downloaded_temp_file_impl.cc File content/browser/loader/downloaded_temp_file_impl.cc (right): https://codereview.chromium.org/2503813002/diff/180001/content/browser/loader/downloaded_temp_file_impl.cc#newcode13 content/browser/loader/downloaded_temp_file_impl.cc:13: void OnConnectionError(int child_id, int request_id) { On 2016/11/22 16:19:33, ...
3 years, 8 months ago (2016-11-24 14:06:40 UTC) #51
mmenke
loader/ LGTM!
3 years, 8 months ago (2016-11-28 18:51:01 UTC) #56
tzik
+avi as a //content OWNER. PTAL.
3 years, 8 months ago (2016-11-30 06:25:25 UTC) #58
Avi (use Gerrit)
lgtm stamp You really need a BUG=
3 years, 8 months ago (2016-11-30 17:27:19 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2503813002/200001
3 years, 8 months ago (2016-12-01 02:29:44 UTC) #64
commit-bot: I haz the power
Committed patchset #10 (id:200001)
3 years, 8 months ago (2016-12-01 04:17:19 UTC) #67
commit-bot: I haz the power
3 years, 8 months ago (2016-12-01 04:20:23 UTC) #69
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/47a9f9bc844919d1bcdbf27a2a28256b1f40dd06
Cr-Commit-Position: refs/heads/master@{#435551}

Powered by Google App Engine
This is Rietveld 408576698