|
|
Created:
4 years, 9 months ago by Łukasz Anforowicz Modified:
4 years, 9 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. |
DescriptionFix 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). #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Fix how Save-Page-As responds to web requests blocked by extensions. BUG=595345 ========== to ========== Fix how Save-Page-As responds to web requests blocked by extensions. Description of the bug and the cause of the regression ====================================================== 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. Fix options and the chosen fix ============================== One way to fix the regression would be to go to the older code (to the extent possible) and find SavePackage based on GetSavePackageFromRenderIds. This is problematic because the routing ids are not available in some code paths (i.e. when SaveFinished is called from SaveLocalFile). I think a better way to fix the problem is to always call 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 -> SavePackage* map) earlier - from OnStartSave into SaveURL method. Below I try to explain why this is okay. Note that the size of the wall of text below is not necessarily indicative of the complexity of the fix - I think the fix is relatively simple, but I wanted to exhaustively analyse old behavior and its potential interactions with the fix to avoid the kind of problems I managed to introduce in the old CL from December. SavePackage* is guaranteed to stay alive until RemoveSaveFile call ------------------------------------------------------------------ Before SavePackage dies, its constructor will end up calling SavePackage::Stop which will call RemoveSaveFile for all SaveItems currently in progress if |!finished && !cancelled()|. If |cancelled()|, then SavePackage::Stop was already called. If |!finished| then there are no SaveItems in progress (either because SavePackage::Stop was called earlier or because SavePackage::Finish was called [indicating that all SaveItems have been completed and therefore already removed via RemoveSaveFile - lack of in-progress items is checked by the very first "if" inside SavePackage::CheckFinish]). Only removing in-progress items is fine - SaveItem::state_ is only set to IN_PROGRESS in SaveItem::Start which is called from SavePackage::SaveNextFile next to the call to SaveFileManager::SaveURL (which is what populates the SaveFileManager::package_ map after this CL). |package_| lookups will succeed if they succeeded before this CL ---------------------------------------------------------------- Previously |package_| was populated in OnStartSave. SaveURL is *always* called before OnStartSave as shown by analyzing callers of StartSave: 1. SaveFileResourceHandler::OnResponseStarted is kicked off via SaveURL / OnSaveURL / ResourceDispatcherHostImpl::BeginSaveFile 2. StartSave call is posted directly by SaveURL for non-net-based saves (this cover both file-based and html/dom-frame-dumping saves) Therefore, by moving adding of |package_| entry from OnStartSave to SaveURL we are not introducing any situations when |package_| has been populated, but won't be populated after the changes. Another way to look at this, is to see where |package_| lookups happen and indeed - they happen *after* SaveURL (and interestingly not always after OnStartSave - see the last item about cancellation below): - UpdateSaveProgress A. called from SaveFileResourceHandler::OnReadCompleted (SaveURL would be called by SavePackage::SaveNextFile called with |process_all_remaining_items| set to false). B. called from SavePackage::OnSerializedHtmlWithLocalLinksResponse (SaveURL would be called by SavePackage::SaveNextFile called with |process_all_remaining_items| set to true). - OnSaveFinished - called from SaveFileResourceHandler::OnResponseCompleted (same reasoning as for case A above) - called from SavePackage::OnSerializedHtmlWithLocalLinksResponse (same reasoning as for case B above) - called from SavePackage::GetSerializedHtmlWithLocalLinks (also same reasoning as for case B above) - RemoveSaveFile - called from SavePackage::SaveFinished - called from SaveFileManager::OnSaveFinished (so reasoning from whole previous item applies) - called from SavePackage::StartSave upon error (so reasoning about OnStartSave / SaveURL ordering from first paragraph in this section applies) - called from SavePackage::SaveCancelled which gets called via SaveItem::Cancel which gets called via SavePackage::Stop. In this case we get better guarantees that lookup will succeed than before the CL (because before the CL we were racing with StartSave call posted from SaveFileResourceHandler::OnResponseStarted onto FILE thread). Okay if RemoveSaveFile is called before a |package_| lookup ----------------------------------------------------------- |package_| lookups happen in: - OnUpdateSaveProgress - OnSaveFinished - RemoveSaveFile All lookups are failsafe in case the entry has already been removed (to handle the race between IO/FILE threads and cancellation on the UI thread). Therefore it is okay if RemoveSaveFile is called (from SavePackage::SaveCanceled) before any of the three lookups listed above. Other notes =========== The bug also highlights that when calculating local paths, we need to skip SaveItems that encountered failures (in SavePackage::GetSerializedHtmlWithLocalLinksForFrame). BUG=595345 ==========
Description was changed from ========== Fix how Save-Page-As responds to web requests blocked by extensions. Description of the bug and the cause of the regression ====================================================== 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. Fix options and the chosen fix ============================== One way to fix the regression would be to go to the older code (to the extent possible) and find SavePackage based on GetSavePackageFromRenderIds. This is problematic because the routing ids are not available in some code paths (i.e. when SaveFinished is called from SaveLocalFile). I think a better way to fix the problem is to always call 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 -> SavePackage* map) earlier - from OnStartSave into SaveURL method. Below I try to explain why this is okay. Note that the size of the wall of text below is not necessarily indicative of the complexity of the fix - I think the fix is relatively simple, but I wanted to exhaustively analyse old behavior and its potential interactions with the fix to avoid the kind of problems I managed to introduce in the old CL from December. SavePackage* is guaranteed to stay alive until RemoveSaveFile call ------------------------------------------------------------------ Before SavePackage dies, its constructor will end up calling SavePackage::Stop which will call RemoveSaveFile for all SaveItems currently in progress if |!finished && !cancelled()|. If |cancelled()|, then SavePackage::Stop was already called. If |!finished| then there are no SaveItems in progress (either because SavePackage::Stop was called earlier or because SavePackage::Finish was called [indicating that all SaveItems have been completed and therefore already removed via RemoveSaveFile - lack of in-progress items is checked by the very first "if" inside SavePackage::CheckFinish]). Only removing in-progress items is fine - SaveItem::state_ is only set to IN_PROGRESS in SaveItem::Start which is called from SavePackage::SaveNextFile next to the call to SaveFileManager::SaveURL (which is what populates the SaveFileManager::package_ map after this CL). |package_| lookups will succeed if they succeeded before this CL ---------------------------------------------------------------- Previously |package_| was populated in OnStartSave. SaveURL is *always* called before OnStartSave as shown by analyzing callers of StartSave: 1. SaveFileResourceHandler::OnResponseStarted is kicked off via SaveURL / OnSaveURL / ResourceDispatcherHostImpl::BeginSaveFile 2. StartSave call is posted directly by SaveURL for non-net-based saves (this cover both file-based and html/dom-frame-dumping saves) Therefore, by moving adding of |package_| entry from OnStartSave to SaveURL we are not introducing any situations when |package_| has been populated, but won't be populated after the changes. Another way to look at this, is to see where |package_| lookups happen and indeed - they happen *after* SaveURL (and interestingly not always after OnStartSave - see the last item about cancellation below): - UpdateSaveProgress A. called from SaveFileResourceHandler::OnReadCompleted (SaveURL would be called by SavePackage::SaveNextFile called with |process_all_remaining_items| set to false). B. called from SavePackage::OnSerializedHtmlWithLocalLinksResponse (SaveURL would be called by SavePackage::SaveNextFile called with |process_all_remaining_items| set to true). - OnSaveFinished - called from SaveFileResourceHandler::OnResponseCompleted (same reasoning as for case A above) - called from SavePackage::OnSerializedHtmlWithLocalLinksResponse (same reasoning as for case B above) - called from SavePackage::GetSerializedHtmlWithLocalLinks (also same reasoning as for case B above) - RemoveSaveFile - called from SavePackage::SaveFinished - called from SaveFileManager::OnSaveFinished (so reasoning from whole previous item applies) - called from SavePackage::StartSave upon error (so reasoning about OnStartSave / SaveURL ordering from first paragraph in this section applies) - called from SavePackage::SaveCancelled which gets called via SaveItem::Cancel which gets called via SavePackage::Stop. In this case we get better guarantees that lookup will succeed than before the CL (because before the CL we were racing with StartSave call posted from SaveFileResourceHandler::OnResponseStarted onto FILE thread). Okay if RemoveSaveFile is called before a |package_| lookup ----------------------------------------------------------- |package_| lookups happen in: - OnUpdateSaveProgress - OnSaveFinished - RemoveSaveFile All lookups are failsafe in case the entry has already been removed (to handle the race between IO/FILE threads and cancellation on the UI thread). Therefore it is okay if RemoveSaveFile is called (from SavePackage::SaveCanceled) before any of the three lookups listed above. Other notes =========== The bug also highlights that when calculating local paths, we need to skip SaveItems that encountered failures (in SavePackage::GetSerializedHtmlWithLocalLinksForFrame). BUG=595345 ========== to ========== Fix how Save-Page-As responds to web requests blocked by extensions. Description of the bug and the cause of the regression ====================================================== 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. Fix options and the chosen fix ============================== One way to fix the regression would be to go to the older code (to the extent possible) and find SavePackage based on GetSavePackageFromRenderIds. This is problematic because the routing ids are not available in some code paths (i.e. when SaveFinished is called from SaveLocalFile). I think a better way to fix the problem is to always call 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 -> SavePackage* map) earlier - from OnStartSave into SaveURL method. Below I try to explain why this is okay. Note that the size of the wall of text below is not necessarily indicative of the complexity of the fix - I think the fix is relatively simple, but I wanted to exhaustively analyse old behavior and its potential interactions with the fix to avoid the kind of problems I managed to introduce in the old CL from December. SavePackage* is guaranteed to stay alive until RemoveSaveFile call ------------------------------------------------------------------ Before SavePackage dies, its constructor will end up calling SavePackage::Stop which will call RemoveSaveFile for all SaveItems currently in progress if |!finished && !cancelled()|. If |cancelled()|, then SavePackage::Stop was already called. If |!finished| then there are no SaveItems in progress (either because SavePackage::Stop was called earlier or because SavePackage::Finish was called [indicating that all SaveItems have been completed and therefore already removed via RemoveSaveFile - lack of in-progress items is checked by the very first "if" inside SavePackage::CheckFinish]). Only removing in-progress items is fine - SaveItem::state_ is only set to IN_PROGRESS in SaveItem::Start which is called from SavePackage::SaveNextFile next to the call to SaveFileManager::SaveURL (which is what populates the SaveFileManager::package_ map after this CL). |package_| lookups will succeed if they succeeded before this CL ---------------------------------------------------------------- Previously |package_| was populated in OnStartSave. SaveURL is *always* called before OnStartSave as shown by analyzing callers of StartSave: 1. SaveFileResourceHandler::OnResponseStarted is kicked off via SaveURL / OnSaveURL / ResourceDispatcherHostImpl::BeginSaveFile 2. StartSave call is posted directly by SaveURL for non-net-based saves (this cover both file-based and html/dom-frame-dumping saves) Therefore, by moving adding of |package_| entry from OnStartSave to SaveURL we are not introducing any situations when |package_| has been populated, but won't be populated after the changes. Another way to look at this, is to see where |package_| lookups happen and indeed - they happen *after* SaveURL (and interestingly not always after OnStartSave - see the last item about cancellation below): - UpdateSaveProgress A. called from SaveFileResourceHandler::OnReadCompleted (SaveURL would be called by SavePackage::SaveNextFile called with |process_all_remaining_items| set to false). B. called from SavePackage::OnSerializedHtmlWithLocalLinksResponse (SaveURL would be called by SavePackage::SaveNextFile called with |process_all_remaining_items| set to true). - OnSaveFinished - called from SaveFileResourceHandler::OnResponseCompleted (same reasoning as for case A above) - called from SavePackage::OnSerializedHtmlWithLocalLinksResponse (same reasoning as for case B above) - called from SavePackage::GetSerializedHtmlWithLocalLinks (also same reasoning as for case B above) - RemoveSaveFile - called from SavePackage::SaveFinished - called from SaveFileManager::OnSaveFinished (so reasoning from whole previous item applies) - called from SavePackage::StartSave upon error (so reasoning about OnStartSave / SaveURL ordering from first paragraph in this section applies) - called from SavePackage::SaveCancelled which gets called via SaveItem::Cancel which gets called via SavePackage::Stop. In this case we get better guarantees that lookup will succeed than before the CL (because before the CL we were racing with StartSave call posted from SaveFileResourceHandler::OnResponseStarted onto FILE thread). Okay if RemoveSaveFile is called before a |package_| lookup ----------------------------------------------------------- |package_| lookups happen in: - OnUpdateSaveProgress - OnSaveFinished - RemoveSaveFile All lookups are failsafe in case the entry has already been removed (to handle the race between IO/FILE threads and cancellation on the UI thread). Therefore it is okay if RemoveSaveFile is called (from SavePackage::SaveCanceled) before any of the three lookups listed above. Other notes =========== The bug also highlights that when calculating local paths, we need to skip SaveItems that encountered failures (in SavePackage::GetSerializedHtmlWithLocalLinksForFrame). BUG=595345 ==========
lukasza@chromium.org changed reviewers: + asanka@chromium.org
Asanka, could you please take a look?
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
Two high level comments: * While as a reviewer I'm very glad you did the due diligence in the CL description and shared it with me, I think it's too detailed for a CL description; better is just saying what the CL is doing and why (reference to regression in previous CL is fine), without going into design alternatives and careful evaluation of control flows. I think that would be most simply documented as a comment on the CL; if you want to respond to this comment with a cut&paste of the CL description and then edit the description down, that would be fine by me. * If you do keep the full description, I think (please confirm?) that the sentence starting "If |!finished| then there are no SaveItems in progress (either because SavePackage::Stop was called earlier...." should actually be |finished| (i.e. without the !). I'm going to try and validate what I see as the key section on SavePackage being guaranteed to live for the expanded window (SaveURL->OnStartURL), then take a look at the code. (I.e. not blocked on you, just giving you initial feedback.)
LGTM with above notes (and if you want to leave the CL description as is, I could be convinced, I just want to hear your reasoning).
lgtm
Description was changed from ========== Fix how Save-Page-As responds to web requests blocked by extensions. Description of the bug and the cause of the regression ====================================================== 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. Fix options and the chosen fix ============================== One way to fix the regression would be to go to the older code (to the extent possible) and find SavePackage based on GetSavePackageFromRenderIds. This is problematic because the routing ids are not available in some code paths (i.e. when SaveFinished is called from SaveLocalFile). I think a better way to fix the problem is to always call 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 -> SavePackage* map) earlier - from OnStartSave into SaveURL method. Below I try to explain why this is okay. Note that the size of the wall of text below is not necessarily indicative of the complexity of the fix - I think the fix is relatively simple, but I wanted to exhaustively analyse old behavior and its potential interactions with the fix to avoid the kind of problems I managed to introduce in the old CL from December. SavePackage* is guaranteed to stay alive until RemoveSaveFile call ------------------------------------------------------------------ Before SavePackage dies, its constructor will end up calling SavePackage::Stop which will call RemoveSaveFile for all SaveItems currently in progress if |!finished && !cancelled()|. If |cancelled()|, then SavePackage::Stop was already called. If |!finished| then there are no SaveItems in progress (either because SavePackage::Stop was called earlier or because SavePackage::Finish was called [indicating that all SaveItems have been completed and therefore already removed via RemoveSaveFile - lack of in-progress items is checked by the very first "if" inside SavePackage::CheckFinish]). Only removing in-progress items is fine - SaveItem::state_ is only set to IN_PROGRESS in SaveItem::Start which is called from SavePackage::SaveNextFile next to the call to SaveFileManager::SaveURL (which is what populates the SaveFileManager::package_ map after this CL). |package_| lookups will succeed if they succeeded before this CL ---------------------------------------------------------------- Previously |package_| was populated in OnStartSave. SaveURL is *always* called before OnStartSave as shown by analyzing callers of StartSave: 1. SaveFileResourceHandler::OnResponseStarted is kicked off via SaveURL / OnSaveURL / ResourceDispatcherHostImpl::BeginSaveFile 2. StartSave call is posted directly by SaveURL for non-net-based saves (this cover both file-based and html/dom-frame-dumping saves) Therefore, by moving adding of |package_| entry from OnStartSave to SaveURL we are not introducing any situations when |package_| has been populated, but won't be populated after the changes. Another way to look at this, is to see where |package_| lookups happen and indeed - they happen *after* SaveURL (and interestingly not always after OnStartSave - see the last item about cancellation below): - UpdateSaveProgress A. called from SaveFileResourceHandler::OnReadCompleted (SaveURL would be called by SavePackage::SaveNextFile called with |process_all_remaining_items| set to false). B. called from SavePackage::OnSerializedHtmlWithLocalLinksResponse (SaveURL would be called by SavePackage::SaveNextFile called with |process_all_remaining_items| set to true). - OnSaveFinished - called from SaveFileResourceHandler::OnResponseCompleted (same reasoning as for case A above) - called from SavePackage::OnSerializedHtmlWithLocalLinksResponse (same reasoning as for case B above) - called from SavePackage::GetSerializedHtmlWithLocalLinks (also same reasoning as for case B above) - RemoveSaveFile - called from SavePackage::SaveFinished - called from SaveFileManager::OnSaveFinished (so reasoning from whole previous item applies) - called from SavePackage::StartSave upon error (so reasoning about OnStartSave / SaveURL ordering from first paragraph in this section applies) - called from SavePackage::SaveCancelled which gets called via SaveItem::Cancel which gets called via SavePackage::Stop. In this case we get better guarantees that lookup will succeed than before the CL (because before the CL we were racing with StartSave call posted from SaveFileResourceHandler::OnResponseStarted onto FILE thread). Okay if RemoveSaveFile is called before a |package_| lookup ----------------------------------------------------------- |package_| lookups happen in: - OnUpdateSaveProgress - OnSaveFinished - RemoveSaveFile All lookups are failsafe in case the entry has already been removed (to handle the race between IO/FILE threads and cancellation on the UI thread). Therefore it is okay if RemoveSaveFile is called (from SavePackage::SaveCanceled) before any of the three lookups listed above. Other notes =========== The bug also highlights that when calculating local paths, we need to skip SaveItems that encountered failures (in SavePackage::GetSerializedHtmlWithLocalLinksForFrame). BUG=595345 ========== to ========== 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 ==========
Thanks for reviewing. I've followed-up by two changes: 1. I've removed the test changes - I'll land them in a separate CL. This will help with merging the fix into Beta and Stable branches (chrome/test/data/save_page/broken-image.htm was added ~3 weeks ago and doesn't exist Stable branch). 2. I've shortened the CL description. The longer description stays for the record on Rietveld (at https://codereview.chromium.org/1812693002/#msg9). Randy - can you ack if the new CL description looks ok?
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/21 21:26:01, Łukasz Anforowicz wrote: > Thanks for reviewing. I've followed-up by two changes: > > 1. I've removed the test changes - I'll land them in a separate CL. This will > help with merging the fix into Beta and Stable branches > (chrome/test/data/save_page/broken-image.htm was added ~3 weeks ago and doesn't > exist Stable branch). > > 2. I've shortened the CL description. The longer description stays for the > record on Rietveld (at https://codereview.chromium.org/1812693002/#msg9). > > > Randy - can you ack if the new CL description looks ok? LGTM
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1812693002/#ps80001 (title: "Removed new test (to help with merges into Beta and Stable branches).")
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
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2813991c65e09a65de095074c867e88af557088e Cr-Commit-Position: refs/heads/master@{#382577} |