Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(733)

Issue 998663002: Flush the extension files to disk before finalize installation. (Closed)

Created:
5 years, 9 months ago by xc
Modified:
5 years, 9 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Flush the extension files to disk before finalize installation. At least on Chrome OS devices, we have experienced extension installation with corrupted files. Flushing the files to disk before renaming the directory should minimize (potentially eliminate) the possiblity of corrupted installs. BUG=463987 TEST=unit tests Committed: https://crrev.com/0b1f9920ac6dd13d4987e41b745bae8601324498 Cr-Commit-Position: refs/heads/master@{#321006}

Patch Set 1 #

Total comments: 4

Patch Set 2 : update comments #

Total comments: 4

Patch Set 3 : flush again after move #

Total comments: 3

Patch Set 4 : refector out a local helper method for dir flush #

Total comments: 4

Patch Set 5 : respond to comments and add an experiment guard. #

Total comments: 6

Patch Set 6 : add space #

Patch Set 7 : add histogram #

Total comments: 4

Patch Set 8 : add webstore download time metric #

Total comments: 4

Patch Set 9 : handle clock going backward #

Total comments: 2

Patch Set 10 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -0 lines) Patch
M chrome/browser/extensions/webstore_installer.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M extensions/common/file_util.cc View 1 2 3 4 5 6 3 chunks +60 lines, -0 lines 0 comments Download
M extensions/common/file_util_unittest.cc View 2 chunks +5 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 2 chunks +18 lines, -0 lines 0 comments Download

Messages

Total messages: 39 (5 generated)
xc
I will add the owner once you guys give lgtm.
5 years, 9 months ago (2015-03-10 18:15:18 UTC) #2
Thiemo Nagel
The code changes lgtm but I've some suggestions to improve the comments. And in the ...
5 years, 9 months ago (2015-03-11 13:32:51 UTC) #3
xiyuan
+asargent LGTM but please also wait for Antony.
5 years, 9 months ago (2015-03-11 15:22:24 UTC) #5
xc
https://codereview.chromium.org/998663002/diff/1/extensions/common/file_util.cc File extensions/common/file_util.cc (right): https://codereview.chromium.org/998663002/diff/1/extensions/common/file_util.cc#newcode107 extensions/common/file_util.cc:107: // extension to be in a corrupted state. On ...
5 years, 9 months ago (2015-03-11 18:32:17 UTC) #6
Thiemo Nagel
https://codereview.chromium.org/998663002/diff/20001/extensions/common/file_util.cc File extensions/common/file_util.cc (right): https://codereview.chromium.org/998663002/diff/20001/extensions/common/file_util.cc#newcode108 extensions/common/file_util.cc:108: // may still be loss. s/loss/lost/ https://codereview.chromium.org/998663002/diff/20001/extensions/common/file_util.cc#newcode123 extensions/common/file_util.cc:123: // ...
5 years, 9 months ago (2015-03-12 13:22:35 UTC) #7
xc
https://codereview.chromium.org/998663002/diff/20001/extensions/common/file_util.cc File extensions/common/file_util.cc (right): https://codereview.chromium.org/998663002/diff/20001/extensions/common/file_util.cc#newcode108 extensions/common/file_util.cc:108: // may still be loss. On 2015/03/12 13:22:35, Thiemo ...
5 years, 9 months ago (2015-03-12 17:38:43 UTC) #8
Thiemo Nagel
> Done. I thought of the same thing last night :) Great! LGTM!
5 years, 9 months ago (2015-03-12 18:05:53 UTC) #9
xc
Ping Antony, please take a look.
5 years, 9 months ago (2015-03-12 22:57:10 UTC) #10
asargent_no_longer_on_chrome
Sorry for latency. A couple of high level questions: 1) In the bug entry, xiyuan ...
5 years, 9 months ago (2015-03-12 23:47:26 UTC) #11
Thiemo Nagel
> [...] unless running the fsync on the files in the new > location accomplishes ...
5 years, 9 months ago (2015-03-13 10:45:51 UTC) #12
xc
Thanks Antony for the review. Do you think the cl is commitable after adding the ...
5 years, 9 months ago (2015-03-13 17:16:03 UTC) #13
asargent_no_longer_on_chrome
I don't think you need to wait to get the helper code into base/ - ...
5 years, 9 months ago (2015-03-13 18:40:22 UTC) #14
Thiemo Nagel
https://codereview.chromium.org/998663002/diff/60001/extensions/common/file_util.cc File extensions/common/file_util.cc (right): https://codereview.chromium.org/998663002/diff/60001/extensions/common/file_util.cc#newcode61 extensions/common/file_util.cc:61: for(base::FilePath current = temp_traversal.Next(); !current.empty(); Nit: please insert blank ...
5 years, 9 months ago (2015-03-16 16:07:03 UTC) #15
xc
https://codereview.chromium.org/998663002/diff/60001/extensions/common/file_util.cc File extensions/common/file_util.cc (right): https://codereview.chromium.org/998663002/diff/60001/extensions/common/file_util.cc#newcode57 extensions/common/file_util.cc:57: void FlushFilesInDir(const base::FilePath& path, bool one_file_only) { On 2015/03/13 ...
5 years, 9 months ago (2015-03-16 17:49:13 UTC) #16
Thiemo Nagel
https://codereview.chromium.org/998663002/diff/80001/extensions/common/file_util.cc File extensions/common/file_util.cc (right): https://codereview.chromium.org/998663002/diff/80001/extensions/common/file_util.cc#newcode79 extensions/common/file_util.cc:79: for(base::FilePath current = temp_traversal.Next(); !current.empty(); Nit: could you please ...
5 years, 9 months ago (2015-03-16 17:57:31 UTC) #17
xc
https://codereview.chromium.org/998663002/diff/80001/extensions/common/file_util.cc File extensions/common/file_util.cc (right): https://codereview.chromium.org/998663002/diff/80001/extensions/common/file_util.cc#newcode79 extensions/common/file_util.cc:79: for(base::FilePath current = temp_traversal.Next(); !current.empty(); On 2015/03/16 17:57:31, Thiemo ...
5 years, 9 months ago (2015-03-16 18:40:44 UTC) #18
asargent_no_longer_on_chrome
Having the experiment is great - I think it would also be good to have ...
5 years, 9 months ago (2015-03-16 19:31:02 UTC) #19
xc
Added a histogram to measure exactly the file installation time here. https://codereview.chromium.org/998663002/diff/80001/extensions/common/file_util.cc File extensions/common/file_util.cc (right): ...
5 years, 9 months ago (2015-03-16 21:34:53 UTC) #20
xc
Add histograms owner for owner's review.
5 years, 9 months ago (2015-03-16 21:42:45 UTC) #22
Mark P
https://codereview.chromium.org/998663002/diff/120001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/998663002/diff/120001/tools/metrics/histograms/histograms.xml#newcode8879 tools/metrics/histograms/histograms.xml:8879: + location to the final installation directory. please explicitly ...
5 years, 9 months ago (2015-03-16 21:59:42 UTC) #23
Thiemo Nagel
It might make sense to measure the download and unpack times, too, so that the ...
5 years, 9 months ago (2015-03-17 17:12:26 UTC) #24
xc
On 2015/03/17 17:12:26, Thiemo Nagel wrote: > It might make sense to measure the download ...
5 years, 9 months ago (2015-03-17 18:35:50 UTC) #25
xc
https://codereview.chromium.org/998663002/diff/120001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/998663002/diff/120001/tools/metrics/histograms/histograms.xml#newcode8879 tools/metrics/histograms/histograms.xml:8879: + location to the final installation directory. On 2015/03/16 ...
5 years, 9 months ago (2015-03-17 18:36:27 UTC) #26
asargent_no_longer_on_chrome
lgtm
5 years, 9 months ago (2015-03-17 20:36:20 UTC) #27
Thiemo Nagel
https://codereview.chromium.org/998663002/diff/140001/chrome/browser/extensions/webstore_installer.cc File chrome/browser/extensions/webstore_installer.cc (right): https://codereview.chromium.org/998663002/diff/140001/chrome/browser/extensions/webstore_installer.cc#newcode703 chrome/browser/extensions/webstore_installer.cc:703: I'd suggest to add "if (download.GetEndTime() >= download.GetStartTime())" since ...
5 years, 9 months ago (2015-03-17 20:41:55 UTC) #28
asargent_no_longer_on_chrome
https://codereview.chromium.org/998663002/diff/140001/chrome/browser/extensions/webstore_installer.cc File chrome/browser/extensions/webstore_installer.cc (right): https://codereview.chromium.org/998663002/diff/140001/chrome/browser/extensions/webstore_installer.cc#newcode703 chrome/browser/extensions/webstore_installer.cc:703: On 2015/03/17 20:41:55, Thiemo Nagel wrote: > I'd suggest ...
5 years, 9 months ago (2015-03-17 20:46:49 UTC) #29
xc
https://codereview.chromium.org/998663002/diff/140001/chrome/browser/extensions/webstore_installer.cc File chrome/browser/extensions/webstore_installer.cc (right): https://codereview.chromium.org/998663002/diff/140001/chrome/browser/extensions/webstore_installer.cc#newcode703 chrome/browser/extensions/webstore_installer.cc:703: On 2015/03/17 20:41:55, Thiemo Nagel wrote: > I'd suggest ...
5 years, 9 months ago (2015-03-17 21:05:19 UTC) #30
Mark P
histograms.xml lgtm
5 years, 9 months ago (2015-03-17 21:08:43 UTC) #31
Thiemo Nagel
Thank you for your patience, Xiaohui! https://codereview.chromium.org/998663002/diff/160001/chrome/browser/extensions/webstore_installer.cc File chrome/browser/extensions/webstore_installer.cc (right): https://codereview.chromium.org/998663002/diff/160001/chrome/browser/extensions/webstore_installer.cc#newcode705 chrome/browser/extensions/webstore_installer.cc:705: if (download.GetEndTime() > ...
5 years, 9 months ago (2015-03-17 21:09:37 UTC) #32
xc
https://codereview.chromium.org/998663002/diff/160001/chrome/browser/extensions/webstore_installer.cc File chrome/browser/extensions/webstore_installer.cc (right): https://codereview.chromium.org/998663002/diff/160001/chrome/browser/extensions/webstore_installer.cc#newcode705 chrome/browser/extensions/webstore_installer.cc:705: if (download.GetEndTime() > download.GetStartTime()) { On 2015/03/17 21:09:37, Thiemo ...
5 years, 9 months ago (2015-03-17 21:24:42 UTC) #33
xc
5 years, 9 months ago (2015-03-17 21:24:45 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/998663002/180001
5 years, 9 months ago (2015-03-17 21:58:36 UTC) #37
commit-bot: I haz the power
Committed patchset #10 (id:180001)
5 years, 9 months ago (2015-03-17 23:00:39 UTC) #38
commit-bot: I haz the power
5 years, 9 months ago (2015-03-17 23:01:21 UTC) #39
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/0b1f9920ac6dd13d4987e41b745bae8601324498
Cr-Commit-Position: refs/heads/master@{#321006}

Powered by Google App Engine
This is Rietveld 408576698