Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2508503002:
Fix an issue that temp files are left permanently on storage after chrome crash (Closed)
DescriptionFix an issue that temp files are left permanently on storage after chrome crash
When download path is determined, Chrome will commit that information to history.
However, the information is not committed immediately.
As a result, if Chrome crashes before the next commit, the path information is lost.
When resuming the download later, Chrome will generate a new target path.
And the old temp file is left permanently on external storage.
This might not be a big issue for desktop, as Chrome rarely gets killed and there are ample storage.
However, this has a great impact on android.
This CL requests the path information to be committed immediately to address the issue.
BUG=664677
Committed: https://crrev.com/64558cd6c0e060c1f127f8f36f692529c8087aae
Cr-Commit-Position: refs/heads/master@{#434738}
Patch Set 1 #
Total comments: 6
Patch Set 2 : addressing comments and fix test #
Total comments: 6
Patch Set 3 : addressing comments #Patch Set 4 : rebase #
Messages
Total messages: 45 (26 generated)
The CQ bit was checked by qinmin@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...)
qinmin@chromium.org changed reviewers: + asanka@chromium.org, sky@chromium.org
PTAL
Patchset #2 (id:20001) has been deleted
Nit is optional but could you address the other two? LGTM with those addressed. https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_history.cc (right): https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_history.cc:167: const history::DownloadRow& current) { Minor nit: You could fold this into ShouldUpdateHistory above. It can return a 3-state instead of a bool. I.e. NO, UPDATE, UPDATE_AND_COMMIT_IMMEDIATELY or somesuch. https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_history.cc:168: #if defined(OS_ANDROID) Let's make this work on all platforms. https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_history_unittest.cc (right): https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_history_unittest.cc:95: update_download_ = info; Store should_commit_immediately bit and test that it is set as expected when changing the current path.
https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_history.cc (right): https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_history.cc:167: const history::DownloadRow& current) { On 2016/11/16 23:15:57, asanka wrote: > Minor nit: You could fold this into ShouldUpdateHistory above. It can return a > 3-state instead of a bool. I.e. NO, UPDATE, UPDATE_AND_COMMIT_IMMEDIATELY or > somesuch. Done. https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_history.cc:168: #if defined(OS_ANDROID) On 2016/11/16 23:15:57, asanka wrote: > Let's make this work on all platforms. Done. https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_history_unittest.cc (right): https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... chrome/browser/download/download_history_unittest.cc:95: update_download_ = info; On 2016/11/16 23:15:57, asanka wrote: > Store should_commit_immediately bit and test that it is set as expected when > changing the current path. Done.
The CQ bit was checked by qinmin@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 #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
New patchset? On 2016/11/17 at 00:46:30, qinmin wrote: > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > File chrome/browser/download/download_history.cc (right): > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > chrome/browser/download/download_history.cc:167: const history::DownloadRow& current) { > On 2016/11/16 23:15:57, asanka wrote: > > Minor nit: You could fold this into ShouldUpdateHistory above. It can return a > > 3-state instead of a bool. I.e. NO, UPDATE, UPDATE_AND_COMMIT_IMMEDIATELY or > > somesuch. > > Done. > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > chrome/browser/download/download_history.cc:168: #if defined(OS_ANDROID) > On 2016/11/16 23:15:57, asanka wrote: > > Let's make this work on all platforms. > > Done. > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > File chrome/browser/download/download_history_unittest.cc (right): > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > chrome/browser/download/download_history_unittest.cc:95: update_download_ = info; > On 2016/11/16 23:15:57, asanka wrote: > > Store should_commit_immediately bit and test that it is set as expected when > > changing the current path. > > Done.
Sorry, patchset is now uploaded. Found an issue with the current DownloadHistoryTest while uploading the patchset earlier. In InitBasicItem(), we always use content::DownloadItem::COMPLETE as the state. However, when download state is content::DownloadItem::COMPLETE, Chrome will not set the DownloadRow in DownloadHistoryData. As a result, when running DownloadHistoryTest_Update, we are always hitting the case that the previous DownloadRow is null. So we are not actually comparing the new value and the old value. This makes the test almost useless. The new patchset sets the download state to IN_PROGRESS to fix the above issue. On 2016/11/17 15:08:55, asanka wrote: > New patchset? > > On 2016/11/17 at 00:46:30, qinmin wrote: > > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > > File chrome/browser/download/download_history.cc (right): > > > > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > > chrome/browser/download/download_history.cc:167: const history::DownloadRow& > current) { > > On 2016/11/16 23:15:57, asanka wrote: > > > Minor nit: You could fold this into ShouldUpdateHistory above. It can return > a > > > 3-state instead of a bool. I.e. NO, UPDATE, UPDATE_AND_COMMIT_IMMEDIATELY or > > > somesuch. > > > > Done. > > > > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > > chrome/browser/download/download_history.cc:168: #if defined(OS_ANDROID) > > On 2016/11/16 23:15:57, asanka wrote: > > > Let's make this work on all platforms. > > > > Done. > > > > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > > File chrome/browser/download/download_history_unittest.cc (right): > > > > > https://codereview.chromium.org/2508503002/diff/1/chrome/browser/download/dow... > > chrome/browser/download/download_history_unittest.cc:95: update_download_ = > info; > > On 2016/11/16 23:15:57, asanka wrote: > > > Store should_commit_immediately bit and test that it is set as expected when > > > changing the current path. > > > > Done.
The CQ bit was checked by qinmin@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.
ping, asanka@, sky@, would you please take another look? Thanks
On 2016/11/21 17:46:46, qinmin wrote: > ping, asanka@, sky@, would you please take another look? Thanks Isn't there still a window of time between when the download starts *and* the task is processed on history? If you need to truly ensure history processes the task shouldn't you block creation of the download until that happens?
On 2016/11/21 18:17:09, sky wrote: > On 2016/11/21 17:46:46, qinmin wrote: > > ping, asanka@, sky@, would you please take another look? Thanks > > Isn't there still a window of time between when the download starts *and* the > task is processed on history? If you need to truly ensure history processes the > task shouldn't you block creation of the download until that happens? When download starts, we already requests the history to immediately commit the entry: https://cs.chromium.org/chromium/src/components/history/core/browser/history_... However, there could be still some timings on posting the task to history thread, but I don't think that is a big issue. Imagine the use case: user click a download, and chrome got immediately killed before the commit is posted on to history thread. In such case, there is no guarantee that the download will start and can resume, either because slow network request, or because the history is not written. This CL, on the other hand, deals with the issue that after download starts, we should not lost the temporary file information or otherwise those files will be permanently left on the storage. That's a more serious issue. I have hundreds of megabytes temporary download files on my phone, and there is no easy way to visualize those temporary files on Android.
Can we update both of these things at once, and then continue the download? On Mon, Nov 21, 2016 at 10:46 AM, <qinmin@chromium.org> wrote: > On 2016/11/21 18:17:09, sky wrote: >> On 2016/11/21 17:46:46, qinmin wrote: >> > ping, asanka@, sky@, would you please take another look? Thanks >> >> Isn't there still a window of time between when the download starts *and* >> the >> task is processed on history? If you need to truly ensure history >> processes > the >> task shouldn't you block creation of the download until that happens? > > When download starts, we already requests the history to immediately commit > the > entry: > https://cs.chromium.org/chromium/src/components/history/core/browser/history_... > However, there could be still some timings on posting the task to history > thread, but I don't think that is a big issue. Imagine the use case: user > click > a download, and chrome got immediately killed before the commit is posted on > to > history thread. In such case, there is no guarantee that the download will > start > and can resume, either because slow network request, or because the history > is > not written. > > This CL, on the other hand, deals with the issue that after download starts, > we > should not lost the temporary file information or otherwise those files will > be > permanently left on the storage. That's a more serious issue. I have > hundreds of > megabytes temporary download files on my phone, and there is no easy way to > visualize those temporary files on Android. > > > https://codereview.chromium.org/2508503002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM with % comments. sky@ is correctly pointing out that there's a window during which the temporary file exists on disk, but the history update task hasn't run yet. If Chrome were to be killed during this window, then once again Chrome won't be able to associate the file on disk with the download entry. The window is even larger than that. The |DownloadItem::GetCurrentPath| only returns a non-empty path after the download target determination phase has completed successfully, including the time spent prompting the user. The temporary file on disk is created as soon as Chrome starts receiving response body data from the server which happens *before* target determination. So the order of tasks is (roughly): 1. (IO thread) Response headers processed for network request. Download process starts. 2. (IO thread) Reads response body and queues up to a max of 100 segments until step 5 below. 3. (UI thread) DownloadManager starts setting up UI thread state for download. 3.1 (UI -> FILE) DM posts task to handle DownloadFile construction. 4. (FILE thread) DownloadFileImpl created, which creates a temporary file on disk. 5. (FILE thread) DownloadFileImpl writes queued data to temporary file. Hereafter, data queued from IO thread is flushed ASAP on FILE thread to download file. 5.1 (FILE -> UI) DFI posts task to update DownloadItem with DownloadFile construction result. 6. (UI thread) DOwnloadItemImpl initiates download target determination sequence. 6.1 ... lots of stuff happens on multiple threads including FILE and DB threads. 7. (UI thread) DownloadItemImpl finalizes download target determination. 8. (UI -> FILE) DownloadItemImpl posts a task to rename intermediate file to something closer to the target filename. 9. (FILE thread) DownloadFileImpl renames intermediate file. 10. (FILE -> UI) DFI posts task to update DownloadItemImpl with rename result. 11. (UI thread) DownloadItemImpl receives name of intermediate file. 12. (UI thread) DownloadItemImpl notifies observers of update to DownloadItem which includes the new current path. 13. (UI thread) DownloadHistory calls into history:: to update download record in download DB. 14. (UI -> history thread) HistoryService schedules a task to perform DB operation. 15. (history thread) HistoryBackend runs query to update DB record. 16. (history thread) *after this CL* commits pending transaction. So the delay between the file landing on disk and the history DB being current is the time between step 4 and step 16, which includes lots of thread hops and user interaction. This CL ameliorates the problem by narrowing the window by ~5s, but falls short of fix it. Additionally we'd need to: * Account for stray .crdownload files in the download directory. Only Chrome creates those, and at this point .crdownload is a well known extension (search GitHub for .crdownload). So we can assume with some confidence that any such file on the download directory is likely Chrome's doing. Unfortunately, this doesn't easily hold true for folks who run multiple instances of Chrome/Chromium where it's possible that what looks like a stray .crdownload file may be accounted for by a different instance of Chrome. * Make step 5.1 communicate the temporary filename to DownloadItemImpl. That narrows the window further and eliminates the delay caused by download target determination. The latter is important since that also removes the delay due to any necessary user prompting. (you probably want to copy and paste this into the bug so you can keep track of what else needs to happen). https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... File chrome/browser/download/download_history.cc (right): https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... chrome/browser/download/download_history.cc:146: enum ShouldUpdateHistoryResult { use 'enum class' instead. Regular enums pollute the namespace and names like 'NO' and 'UPDATE' are likely to collide. https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... chrome/browser/download/download_history.cc:156: return UPDATE; Should return UPDATE_IMMEDIATELY here. If this condition is true, we can't determine whether the current path was modified and we won't be issuing any more updates unless something else changes with the download. https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... File chrome/browser/download/download_history_unittest.cc (right): https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... chrome/browser/download/download_history_unittest.cc:674: ExpectDownloadUpdated(info, true); There's currently no test for the case where ShouldUpdateDownload is called on a download with no stored previous state. I won't block this CL on adding that test in this CL, but we should add one.
As this is the second attempt at fixing this, is there a way we could truly address the bug? Maybe an ack from history before actually writing to disk? -Scott On Mon, Nov 21, 2016 at 12:17 PM, <asanka@chromium.org> wrote: > LGTM with % comments. > > sky@ is correctly pointing out that there's a window during which the > temporary > file exists on disk, but the history update task hasn't run yet. If Chrome > were > to be killed during this window, then once again Chrome won't be able to > associate the file on disk with the download entry. > > The window is even larger than that. The |DownloadItem::GetCurrentPath| only > returns a non-empty path after the download target determination phase has > completed successfully, including the time spent prompting the user. The > temporary file on disk is created as soon as Chrome starts receiving > response > body data from the server which happens *before* target determination. > > So the order of tasks is (roughly): > > 1. (IO thread) Response headers processed for network request. Download > process starts. > 2. (IO thread) Reads response body and queues up to a max of 100 segments > until step 5 below. > 3. (UI thread) DownloadManager starts setting up UI thread state for > download. > 3.1 (UI -> FILE) DM posts task to handle DownloadFile construction. > 4. (FILE thread) DownloadFileImpl created, which creates a temporary file on > disk. > 5. (FILE thread) DownloadFileImpl writes queued data to temporary file. > Hereafter, data queued from IO thread is flushed ASAP on FILE thread to > download > file. > 5.1 (FILE -> UI) DFI posts task to update DownloadItem with DownloadFile > construction result. > 6. (UI thread) DOwnloadItemImpl initiates download target determination > sequence. > 6.1 ... lots of stuff happens on multiple threads including FILE and DB > threads. > 7. (UI thread) DownloadItemImpl finalizes download target determination. > 8. (UI -> FILE) DownloadItemImpl posts a task to rename intermediate file to > something closer to the target filename. > 9. (FILE thread) DownloadFileImpl renames intermediate file. > 10. (FILE -> UI) DFI posts task to update DownloadItemImpl with rename > result. > 11. (UI thread) DownloadItemImpl receives name of intermediate file. > 12. (UI thread) DownloadItemImpl notifies observers of update to > DownloadItem > which includes the new current path. > 13. (UI thread) DownloadHistory calls into history:: to update download > record > in download DB. > 14. (UI -> history thread) HistoryService schedules a task to perform DB > operation. > 15. (history thread) HistoryBackend runs query to update DB record. > 16. (history thread) *after this CL* commits pending transaction. > > So the delay between the file landing on disk and the history DB being > current > is the time between step 4 and step 16, which includes lots of thread hops > and > user interaction. > > This CL ameliorates the problem by narrowing the window by ~5s, but falls > short > of fix it. Additionally we'd need to: > > * Account for stray .crdownload files in the download directory. Only Chrome > creates those, and at this point .crdownload is a well known extension > (search > GitHub for .crdownload). So we can assume with some confidence that any such > file on the download directory is likely Chrome's doing. Unfortunately, this > doesn't easily hold true for folks who run multiple instances of > Chrome/Chromium > where it's possible that what looks like a stray .crdownload file may be > accounted for by a different instance of Chrome. > > * Make step 5.1 communicate the temporary filename to DownloadItemImpl. That > narrows the window further and eliminates the delay caused by download > target > determination. The latter is important since that also removes the delay due > to > any necessary user prompting. > > (you probably want to copy and paste this into the bug so you can keep track > of > what else needs to happen). > > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > File chrome/browser/download/download_history.cc (right): > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > chrome/browser/download/download_history.cc:146: enum > ShouldUpdateHistoryResult { > use 'enum class' instead. Regular enums pollute the namespace and names > like 'NO' and 'UPDATE' are likely to collide. > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > chrome/browser/download/download_history.cc:156: return UPDATE; > Should return UPDATE_IMMEDIATELY here. If this condition is true, we > can't determine whether the current path was modified and we won't be > issuing any more updates unless something else changes with the > download. > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > File chrome/browser/download/download_history_unittest.cc (right): > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > chrome/browser/download/download_history_unittest.cc:674: > ExpectDownloadUpdated(info, true); > There's currently no test for the case where ShouldUpdateDownload is > called on a download with no stored previous state. > > I won't block this CL on adding that test in this CL, but we should add > one. > > https://codereview.chromium.org/2508503002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Here are my 2 cents. So there are 2 things we need to commit immediately to the history: 1. Chrome writes to a temporary file on HTTP response. 2. Chrome determines the target path, so the temporary file is renamed. This CL tries to solve the issue in 2, but with some thread hopping delays, so it could be unfortunate if Chrome gets killed during the thread hopping. Delay renaming the temporary file until history commit will not resolve the problem. Because the history db will then hold a different path from the temporary file, and Chrome may get killed before doing the file renaming. To truely solve the problem, we need to get rid of the renaming process. Chrome need to wait until the target path is determined, and commits to the history db, before writing to the disk. However, that seems to be a non-trivial change and we are not sure about the side effects of buffering all the received data. Can we submit this change first and then thinking about how to either solve 1 or solve both problem altogether. I know this is not a perfect fix, but it addresses most of the issues user will experience assuming there is no file name prompt. On 2016/11/21 22:23:47, sky wrote: > As this is the second attempt at fixing this, is there a way we could > truly address the bug? Maybe an ack from history before actually > writing to disk? > > -Scott > > On Mon, Nov 21, 2016 at 12:17 PM, <mailto:asanka@chromium.org> wrote: > > LGTM with % comments. > > > > sky@ is correctly pointing out that there's a window during which the > > temporary > > file exists on disk, but the history update task hasn't run yet. If Chrome > > were > > to be killed during this window, then once again Chrome won't be able to > > associate the file on disk with the download entry. > > > > The window is even larger than that. The |DownloadItem::GetCurrentPath| only > > returns a non-empty path after the download target determination phase has > > completed successfully, including the time spent prompting the user. The > > temporary file on disk is created as soon as Chrome starts receiving > > response > > body data from the server which happens *before* target determination. > > > > So the order of tasks is (roughly): > > > > 1. (IO thread) Response headers processed for network request. Download > > process starts. > > 2. (IO thread) Reads response body and queues up to a max of 100 segments > > until step 5 below. > > 3. (UI thread) DownloadManager starts setting up UI thread state for > > download. > > 3.1 (UI -> FILE) DM posts task to handle DownloadFile construction. > > 4. (FILE thread) DownloadFileImpl created, which creates a temporary file on > > disk. > > 5. (FILE thread) DownloadFileImpl writes queued data to temporary file. > > Hereafter, data queued from IO thread is flushed ASAP on FILE thread to > > download > > file. > > 5.1 (FILE -> UI) DFI posts task to update DownloadItem with DownloadFile > > construction result. > > 6. (UI thread) DOwnloadItemImpl initiates download target determination > > sequence. > > 6.1 ... lots of stuff happens on multiple threads including FILE and DB > > threads. > > 7. (UI thread) DownloadItemImpl finalizes download target determination. > > 8. (UI -> FILE) DownloadItemImpl posts a task to rename intermediate file to > > something closer to the target filename. > > 9. (FILE thread) DownloadFileImpl renames intermediate file. > > 10. (FILE -> UI) DFI posts task to update DownloadItemImpl with rename > > result. > > 11. (UI thread) DownloadItemImpl receives name of intermediate file. > > 12. (UI thread) DownloadItemImpl notifies observers of update to > > DownloadItem > > which includes the new current path. > > 13. (UI thread) DownloadHistory calls into history:: to update download > > record > > in download DB. > > 14. (UI -> history thread) HistoryService schedules a task to perform DB > > operation. > > 15. (history thread) HistoryBackend runs query to update DB record. > > 16. (history thread) *after this CL* commits pending transaction. > > > > So the delay between the file landing on disk and the history DB being > > current > > is the time between step 4 and step 16, which includes lots of thread hops > > and > > user interaction. > > > > This CL ameliorates the problem by narrowing the window by ~5s, but falls > > short > > of fix it. Additionally we'd need to: > > > > * Account for stray .crdownload files in the download directory. Only Chrome > > creates those, and at this point .crdownload is a well known extension > > (search > > GitHub for .crdownload). So we can assume with some confidence that any such > > file on the download directory is likely Chrome's doing. Unfortunately, this > > doesn't easily hold true for folks who run multiple instances of > > Chrome/Chromium > > where it's possible that what looks like a stray .crdownload file may be > > accounted for by a different instance of Chrome. > > > > * Make step 5.1 communicate the temporary filename to DownloadItemImpl. That > > narrows the window further and eliminates the delay caused by download > > target > > determination. The latter is important since that also removes the delay due > > to > > any necessary user prompting. > > > > (you probably want to copy and paste this into the bug so you can keep track > > of > > what else needs to happen). > > > > > > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > > File chrome/browser/download/download_history.cc (right): > > > > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > > chrome/browser/download/download_history.cc:146: enum > > ShouldUpdateHistoryResult { > > use 'enum class' instead. Regular enums pollute the namespace and names > > like 'NO' and 'UPDATE' are likely to collide. > > > > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > > chrome/browser/download/download_history.cc:156: return UPDATE; > > Should return UPDATE_IMMEDIATELY here. If this condition is true, we > > can't determine whether the current path was modified and we won't be > > issuing any more updates unless something else changes with the > > download. > > > > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > > File chrome/browser/download/download_history_unittest.cc (right): > > > > > https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... > > chrome/browser/download/download_history_unittest.cc:674: > > ExpectDownloadUpdated(info, true); > > There's currently no test for the case where ShouldUpdateDownload is > > called on a download with no stored previous state. > > > > I won't block this CL on adding that test in this CL, but we should add > > one. > > > > https://codereview.chromium.org/2508503002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
Ok, LGTM
Also added TODO and linked to the crbug for the remaining issue https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... File chrome/browser/download/download_history.cc (right): https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... chrome/browser/download/download_history.cc:146: enum ShouldUpdateHistoryResult { On 2016/11/21 20:17:57, asanka wrote: > use 'enum class' instead. Regular enums pollute the namespace and names like > 'NO' and 'UPDATE' are likely to collide. Done. https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... chrome/browser/download/download_history.cc:156: return UPDATE; On 2016/11/21 20:17:57, asanka wrote: > Should return UPDATE_IMMEDIATELY here. If this condition is true, we can't > determine whether the current path was modified and we won't be issuing any more > updates unless something else changes with the download. Done. https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... File chrome/browser/download/download_history_unittest.cc (right): https://codereview.chromium.org/2508503002/diff/60001/chrome/browser/download... chrome/browser/download/download_history_unittest.cc:674: ExpectDownloadUpdated(info, true); On 2016/11/21 20:17:57, asanka wrote: > There's currently no test for the case where ShouldUpdateDownload is called on a > download with no stored previous state. > > I won't block this CL on adding that test in this CL, but we should add one. Actually, we set the state INTERRUPTED in this test. That will cause the stored state to get cleared. I added some new code to change the state back to IN_PROGRESS, and that will hit the no stored state case, and also restore the state so that the code after that can do the actual comparison.
The CQ bit was checked by qinmin@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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2508503002/#ps80001 (title: "addressing comments")
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
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_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2508503002/#ps100001 (title: "rebase")
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": 100001, "attempt_start_ts": 1480363811892330,
"parent_rev": "c94d4d95c8971ed7c1513fb4ca0bce08001ea890", "commit_rev":
"44f7203450a85494189355b78792deaff9f2b34a"}
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix an issue that temp files are left permanently on storage after chrome crash When download path is determined, Chrome will commit that information to history. However, the information is not committed immediately. As a result, if Chrome crashes before the next commit, the path information is lost. When resuming the download later, Chrome will generate a new target path. And the old temp file is left permanently on external storage. This might not be a big issue for desktop, as Chrome rarely gets killed and there are ample storage. However, this has a great impact on android. This CL requests the path information to be committed immediately to address the issue. BUG=664677 ========== to ========== Fix an issue that temp files are left permanently on storage after chrome crash When download path is determined, Chrome will commit that information to history. However, the information is not committed immediately. As a result, if Chrome crashes before the next commit, the path information is lost. When resuming the download later, Chrome will generate a new target path. And the old temp file is left permanently on external storage. This might not be a big issue for desktop, as Chrome rarely gets killed and there are ample storage. However, this has a great impact on android. This CL requests the path information to be committed immediately to address the issue. BUG=664677 Committed: https://crrev.com/64558cd6c0e060c1f127f8f36f692529c8087aae Cr-Commit-Position: refs/heads/master@{#434738} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/64558cd6c0e060c1f127f8f36f692529c8087aae Cr-Commit-Position: refs/heads/master@{#434738} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
