[Regression] Creating the profile directory should happen before *any* other file I/O under Profile::GetPath() |
|||||||||||
Issue description
The current ProfileImpl does a likely already somewhat broken attempt at this [1].
But with the migration to TaskScheduler this was likely made much worse (we should fix upstream in M61 at least). Most profile I/O is independent from each other and therefore scheduled on independent sequences *except* for the creation of the profile directory.
Instead of making an attempt at creating the profile async I think we should just create the profile synchronously always, it's a lightweight I/O operation and a justified tradeoff IMO (avoiding the massive complexity to have this specific I/O operation be sequenced ahead of all others).
[1]
// Initiates creation of profile directory on |io_task_runner| and ensures that
// FILE thread is blocked until that operation finishes. If |create_readme| is
// true, the profile README will be created in the profile directory.
void CreateProfileDirectory(base::SequencedTaskRunner* io_task_runner,
const base::FilePath& path,
bool create_readme) {
base::WaitableEvent* done_creating =
new base::WaitableEvent(base::WaitableEvent::ResetPolicy::AUTOMATIC,
base::WaitableEvent::InitialState::NOT_SIGNALED);
io_task_runner->PostTask(
FROM_HERE, base::BindOnce(&CreateDirectoryAndSignal, path, done_creating,
create_readme));
// Block the FILE thread until directory is created on I/O task runner to make
// sure that we don't attempt any operation until that part completes.
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
base::BindOnce(&BlockFileThreadOnDirectoryCreate,
base::Owned(done_creating)));
}
,
Aug 14 2017
,
Aug 14 2017
Please add appropriate OSs.
,
Aug 14 2017
,
Aug 14 2017
[Bulk Edit] URGENT - PTAL. Your bug is labelled as M61 Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP. Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label. Thank you.
,
Aug 15 2017
It doesn't need to block M61 Stable release but it would make it more stable against a theoretical race introduced in its time. I'll try to get a mergeable fix landed ASAP.
,
Aug 15 2017
[Bulk Edit] URGENT - PTAL. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP. Thank you! Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label or move to M62.
,
Aug 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0b08bf2eaee22458d459ef824e644d4493b29c20 commit 0b08bf2eaee22458d459ef824e644d4493b29c20 Author: Gabriel Charette <gab@chromium.org> Date: Tue Aug 15 20:10:06 2017 Make profile directory creation always synchronous. This requires allowing IO on the main thread for two lightweight I/O operations (PathExists + CreateDirectory). The tradeoff is worth it as it ensures the directory is up before any of the asynchronous I/O from every service starts touching their portion of the profile directory. Sequencing all of these operations on the directory creation would be a herculian task and isn't worth it. R=rch@chromium.org NOPRESUBMIT=True (for intentional ScopedAllowIO usage) Bug: 755157 , 689520 Change-Id: I81498d20e91cae79ea3a1948b3d84d255adb6bd6 Reviewed-on: https://chromium-review.googlesource.com/613683 Reviewed-by: Anthony Vallee-Dubois <anthonyvd@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#494517} [modify] https://crrev.com/0b08bf2eaee22458d459ef824e644d4493b29c20/chrome/browser/profiles/profile_impl.cc [modify] https://crrev.com/0b08bf2eaee22458d459ef824e644d4493b29c20/chrome/browser/profiles/profile_manager_unittest.cc
,
Aug 16 2017
Hi, I would like to merge r494517 to M61. It fixes a theoretical race made worse in M61 with the TaskScheduler migration. There's no way to "verify" this but I think it can alleviate weird bugs down the road and feel M61 is safer with it than without.
,
Aug 16 2017
Thank you gab@. * Per comment #6 it doesn't need to block M61 release, can this wait until M62? * Is it M61 regression? * Is the change well baked in Canary, having enough automation tests coverage and safe to merge to M61? * Any other important details to justify the merge. Please note M61 is already in Beta, so merge bar is VERY high. Thank you.
,
Aug 16 2017
It's not a regression per se but it fixes a race (which can cause all kinds of headaches later) Every single browser test goes through this code so yes it's well tested. M61 is just better off with this IMO (I'd be more nervous shipping without the merge)
,
Aug 16 2017
Thank you gab@. Per comment #6 it doesn't need to block M61 release and this is not M61 regression. Per comment #9 & #11 it fixed a race and code is well tested. + abdulsyed@ to take his input on this merge.
,
Aug 16 2017
This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 17 2017
Thanks gab@ for the fix! Since this is a fix to a theoretical race condition, I'd like to confirm if this is overall very safe before approving merge. I see there is an open comment in https://chromium-review.googlesource.com/c/613683 regarding remote filesystems. Is that a concern? Can you please confirm if this fix is safe and will not introduce new regressions? If yes, then I'm fine taking this in M61.
,
Aug 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1abda036eaee58bcb8aee1c2704b93b45a773ea8 commit 1abda036eaee58bcb8aee1c2704b93b45a773ea8 Author: Gabriel Charette <gab@chromium.org> Date: Thu Aug 17 11:27:32 2017 Make ios browser state directory creation always synchronous. This is the ios mirror to https://chromium-review.googlesource.com/c/613683 NOPRESUBMIT=True (for intentional ScopedAllowIO usage) Bug: 755157 , 689520 Change-Id: I856a31ce1e2fc930587e04e5b32c49b19161e155 Reviewed-on: https://chromium-review.googlesource.com/618087 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#495138} [modify] https://crrev.com/1abda036eaee58bcb8aee1c2704b93b45a773ea8/ios/chrome/browser/browser_state/chrome_browser_state_impl.cc
,
Aug 18 2017
@#14: yes we seem it's safe (safer with than without). Will also merge matching change to iOS side of things (r495138) if that's okay.
,
Aug 21 2017
@gab -- Could you please provide the reproducible steps to verify this issue from TE end. Thanks.
,
Aug 21 2017
[Bulk Edit] URGENT - PTAL. M61 Stable promotion is coming soon and your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP. Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label or move to M62. Thank you! Note: We will only have 2 beta releases before Stable promotion. Plan is to cut M61 Stable RC on 08/31/17.
,
Aug 21 2017
@pnangunoori: there's nothing testable here, it just fixes a theoretical data race (which could have resulted in undefined behavior but we have no practical repro)
,
Aug 21 2017
Based on all the comments above, I am approving this for iOS as well.
,
Aug 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/328cfe6556457ddf08715764748e9606cee8b03c commit 328cfe6556457ddf08715764748e9606cee8b03c Author: Gabriel Charette <gab@chromium.org> Date: Mon Aug 21 18:59:56 2017 [Merge M61] Make profile directory creation always synchronous. [Merge M61] Make ios browser state directory creation always synchronous. This requires allowing IO on the main thread for two lightweight I/O operations (PathExists + CreateDirectory). The tradeoff is worth it as it ensures the directory is up before any of the asynchronous I/O from every service starts touching their portion of the profile directory. Sequencing all of these operations on the directory creation would be a herculian task and isn't worth it. NOPRESUBMIT=True (for intentional ScopedAllowIO usage) Bug: 755157 , 689520 Change-Id: I81498d20e91cae79ea3a1948b3d84d255adb6bd6 Reviewed-on: https://chromium-review.googlesource.com/613683 Reviewed-by: Anthony Vallee-Dubois <anthonyvd@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#494517} (cherry picked from commit 0b08bf2eaee22458d459ef824e644d4493b29c20) TBR=anthonyvd@chromium.org, rch@chromium.org, sdefresne@chromium.org (cherry picked from commit 1abda036eaee58bcb8aee1c2704b93b45a773ea8) Change-Id: I856a31ce1e2fc930587e04e5b32c49b19161e155 Reviewed-on: https://chromium-review.googlesource.com/618087 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#495138} Reviewed-on: https://chromium-review.googlesource.com/624455 Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#707} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/328cfe6556457ddf08715764748e9606cee8b03c/chrome/browser/profiles/profile_impl.cc [modify] https://crrev.com/328cfe6556457ddf08715764748e9606cee8b03c/chrome/browser/profiles/profile_manager_unittest.cc [modify] https://crrev.com/328cfe6556457ddf08715764748e9606cee8b03c/ios/chrome/browser/browser_state/chrome_browser_state_impl.cc
,
Aug 22 2017
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by manoranj...@chromium.org
, Aug 14 2017Components: Internals