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

Issue 1812693002: Fix how Save-Page-As responds to web requests blocked by extensions. (Closed)

Created:
3 years, 11 months ago by Łukasz Anforowicz
Modified:
3 years, 11 months ago
CC:
asanka, chromium-reviews, darin-cc_chromium.org, jam, rginda+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix how Save-Page-As responds to web requests blocked by extensions. AdBlocker extensions exercise (via chrome.webRequest API) a code path where ResourceHandler (and in particular SaveFileResourceHandler) reports a failure via OnResponseCompleted without first calling OnResponseStarted. This code path is also executed when the request fails before HTTP headers are received and parsed (i.e. if the host name could not be resolved by DNS). In this code path, SaveFileManager::SaveFinished is called before SaveFileManager::StartSave got called. This means that when SaveFinished calls LookupSaveFile, it won't find anything. This behavior has regressed in https://crrev.com/1484093002, which removed the "else" clause in SaveFinished method, incorrectly thinking that save_item_id-based lookup will always succeed because of the other changes made in the CL. This CL fixes the problem by always calling OnSaveFinished (even if LookupSaveFile failed). To make this work, we need to make sure that SavePackage lookup in SaveFinished succeeds, even if OnStartSave was not called (this is where |package_| map was populated before the fix). To accomplish this, the fix moves populating the |package_| map (SaveItemId to SavePackage* map) earlier - from OnStartSave into SaveURL method. BUG=595345 Committed: https://crrev.com/2813991c65e09a65de095074c867e88af557088e Cr-Commit-Position: refs/heads/master@{#382577}

Patch Set 1 #

Patch Set 2 : Added a working regression test. #

Patch Set 3 : Self-review (comment tweaks + no need for extra OnSaveFinishedBeforeStarting method). #

Patch Set 4 : Fixing grammar in a comment. #

Patch Set 5 : Removed new test (to help with merges into Beta and Stable branches). #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -13 lines) Patch
M content/browser/download/save_file_manager.h View 1 2 1 chunk +4 lines, -2 lines 0 comments Download
M content/browser/download/save_file_manager.cc View 1 2 3 chunks +19 lines, -9 lines 0 comments Download
M content/browser/download/save_package.cc View 1 2 1 chunk +8 lines, -2 lines 0 comments Download

Messages

Total messages: 21 (10 generated)
Łukasz Anforowicz
Asanka, could you please take a look?
3 years, 11 months ago (2016-03-17 20:39:33 UTC) #4
Randy Smith (Not in Mondays)
Two high level comments: * While as a reviewer I'm very glad you did the ...
3 years, 11 months ago (2016-03-21 01:19:42 UTC) #6
Randy Smith (Not in Mondays)
LGTM with above notes (and if you want to leave the CL description as is, ...
3 years, 11 months ago (2016-03-21 20:15:43 UTC) #7
asanka
lgtm
3 years, 11 months ago (2016-03-21 20:48:58 UTC) #8
Łukasz Anforowicz
Thanks for reviewing. I've followed-up by two changes: 1. I've removed the test changes - ...
3 years, 11 months ago (2016-03-21 21:26:01 UTC) #10
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812693002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812693002/80001
3 years, 11 months ago (2016-03-21 23:50:40 UTC) #12
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 11 months ago (2016-03-22 00:52:22 UTC) #14
Randy Smith (Not in Mondays)
On 2016/03/21 21:26:01, Łukasz Anforowicz wrote: > Thanks for reviewing. I've followed-up by two changes: ...
3 years, 11 months ago (2016-03-22 14:34:50 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812693002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812693002/80001
3 years, 11 months ago (2016-03-22 15:50:29 UTC) #18
commit-bot: I haz the power
Committed patchset #5 (id:80001)
3 years, 11 months ago (2016-03-22 15:55:02 UTC) #19
commit-bot: I haz the power
3 years, 11 months ago (2016-03-22 15:56:19 UTC) #21
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/2813991c65e09a65de095074c867e88af557088e
Cr-Commit-Position: refs/heads/master@{#382577}

Powered by Google App Engine
This is Rietveld 408576698