Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2926453002:
Fix a race condition in ~RegistryHashStoreContentsWin (Closed)
DescriptionFix a race condition in ~RegistryHashStoreContentsWin
Enabling mojo-ification of Tracked Preference code highlighted a race
condition in integration tests: two instances of
RegistryHashStoreContentsWin for the same profile can destructed at the
same time on different threads, causing one of the registry operations
initiated by the destructor to fail.
BUG=727282
Review-Url: https://codereview.chromium.org/2926453002
Cr-Original-Commit-Position: refs/heads/master@{#479020}
Committed: https://chromium.googlesource.com/chromium/src/+/7c8c48226fd78ad01858284bd2ee91bed27b513c
Review-Url: https://codereview.chromium.org/2926453002
Cr-Commit-Position: refs/heads/master@{#479123}
Committed: https://chromium.googlesource.com/chromium/src/+/0901195b98d8c46db5a0d164e27a896345dc943c
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review comments, ran format #
Total comments: 1
Patch Set 3 : Share RefCounted object between pref stores, add unit test. #Patch Set 4 : Add file I forgot to git add #
Total comments: 19
Patch Set 5 : Address review comments on #4 #Patch Set 6 : Use MakeCopy in the test #
Total comments: 3
Patch Set 7 : Address review comments on #6 #Patch Set 8 : Rebase, which also reverts a change from https://chromium-review.googlesource.com/527724 #Patch Set 9 : Add explicit out of line destructor to fix clang warning #Patch Set 10 : Add temp_scoped_dir_cleaner to a build file #
Messages
Total messages: 44 (19 generated)
proberge@chromium.org changed reviewers: + gab@chromium.org
Nothing prevents multiple copies and nothing forces that a copy by made. Hence cleaning up only in the copy is surprising to me. How about having a RefCountedThreadSafe internal object which does the cleanup when it is deleted. An instance is only created in the main instance if StartsWith(...) and then a ref is handed to the copies. The last RegistryHashStoreContentsWin() will trigger its thread-safe deletion and the cleanup. https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... File services/preferences/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... services/preferences/tracked/registry_hash_store_contents_win.cc:91: // orignal and clone are destructed (and try to delete the registry key) at s/destructed/destroyed/ https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... services/preferences/tracked/registry_hash_store_contents_win.cc:99: DCHECK_EQ(other.reset_on_delete_, false); DCHECK(!other.reset_on_delete_);
On 2017/06/05 18:04:22, gab wrote: > Nothing prevents multiple copies and nothing forces that a copy by made. Hence > cleaning up only in the copy is surprising to me. > > How about having a RefCountedThreadSafe internal object which does the cleanup > when it is deleted. An instance is only created in the main instance if > StartsWith(...) and then a ref is handed to the copies. The last > RegistryHashStoreContentsWin() will trigger its thread-safe deletion and the > cleanup. > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > File services/preferences/tracked/registry_hash_store_contents_win.cc (right): > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > services/preferences/tracked/registry_hash_store_contents_win.cc:91: // orignal > and clone are destructed (and try to delete the registry key) at > s/destructed/destroyed/ > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > services/preferences/tracked/registry_hash_store_contents_win.cc:99: > DCHECK_EQ(other.reset_on_delete_, false); > DCHECK(!other.reset_on_delete_); Tried adding a RefCountedThreadSafe internal object as you suggested. However, I ran into another issue: For each profile, two copies of the RegistryHashStoreContentsWin are created: one for unprotected_hash_filter and one for protected_hash_filter: https://cs.chromium.org/chromium/src/services/preferences/tracked/tracked_per... If each RegistryHashStoreContentsWin has its own scoped_refptr, the registry deletion logic can be still racy if Chrome shuts down while only one of the (protected, unprotected) pref types had a scheduled write. (If neither have to write, then both destructors are hit on the main thread. If both have to write, then both destructors are hit on the Mojo IO thread). It sounds like making the RefCountedThreadSafe internal object also be BrowserThread::DeleteOnIOThread would prevent the race. However, I don't think including "content/public/browser/browser_thread.h" from services is acceptable. WDYT?
https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... File services/preferences/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... services/preferences/tracked/registry_hash_store_contents_win.cc:91: // orignal and clone are destructed (and try to delete the registry key) at On 2017/06/05 18:04:21, gab wrote: > s/destructed/destroyed/ Done. https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... services/preferences/tracked/registry_hash_store_contents_win.cc:99: DCHECK_EQ(other.reset_on_delete_, false); On 2017/06/05 18:04:21, gab wrote: > DCHECK(!other.reset_on_delete_); Done.
On 2017/06/06 21:29:42, proberge wrote: > On 2017/06/05 18:04:22, gab wrote: > > Nothing prevents multiple copies and nothing forces that a copy by made. Hence > > cleaning up only in the copy is surprising to me. > > > > How about having a RefCountedThreadSafe internal object which does the cleanup > > when it is deleted. An instance is only created in the main instance if > > StartsWith(...) and then a ref is handed to the copies. The last > > RegistryHashStoreContentsWin() will trigger its thread-safe deletion and the > > cleanup. > > > > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > > File services/preferences/tracked/registry_hash_store_contents_win.cc (right): > > > > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > > services/preferences/tracked/registry_hash_store_contents_win.cc:91: // > orignal > > and clone are destructed (and try to delete the registry key) at > > s/destructed/destroyed/ > > > > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > > services/preferences/tracked/registry_hash_store_contents_win.cc:99: > > DCHECK_EQ(other.reset_on_delete_, false); > > DCHECK(!other.reset_on_delete_); > > Tried adding a RefCountedThreadSafe internal object as you suggested. However, > I ran into another issue: For each profile, two copies of the > RegistryHashStoreContentsWin are created: one for unprotected_hash_filter and > one for protected_hash_filter: > https://cs.chromium.org/chromium/src/services/preferences/tracked/tracked_per... > > If each RegistryHashStoreContentsWin has its own scoped_refptr, the registry > deletion logic can be still racy if Chrome shuts down while only one of the > (protected, unprotected) pref types had a scheduled write. > (If neither have to write, then both destructors are hit on the main thread. > If both have to write, then both destructors are hit on the Mojo IO thread). > > It sounds like making the RefCountedThreadSafe internal object also be > BrowserThread::DeleteOnIOThread would prevent the race. However, I don't think > including "content/public/browser/browser_thread.h" from services is acceptable. > WDYT? Right we can't make that include. There is base::DeleteOnTaskRunner but it's not currently compatible with RefCountedThreadSafe sadly (though we could make it work through hoops, i.e. have the RefCountedThreadSafe object hold a unique_ptr which is itself using DeleteOnTaskRunner as a deleter...). But that's also not guaranteed to run right (as the main thread won't be executing tasks on shutdown)? The best way would be to have all stores (protected/unprotected) using the same key share the same RefCounted cleaner, would it be too much of a mess to pipe it from ProfilePrefStoreManager or something..?!
Patchset #3 (id:40001) has been deleted
On 2017/06/08 13:07:48, gab wrote: > On 2017/06/06 21:29:42, proberge wrote: > > On 2017/06/05 18:04:22, gab wrote: > > > Nothing prevents multiple copies and nothing forces that a copy by made. > Hence > > > cleaning up only in the copy is surprising to me. > > > > > > How about having a RefCountedThreadSafe internal object which does the > cleanup > > > when it is deleted. An instance is only created in the main instance if > > > StartsWith(...) and then a ref is handed to the copies. The last > > > RegistryHashStoreContentsWin() will trigger its thread-safe deletion and the > > > cleanup. > > > > > > > > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > > > File services/preferences/tracked/registry_hash_store_contents_win.cc > (right): > > > > > > > > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > > > services/preferences/tracked/registry_hash_store_contents_win.cc:91: // > > orignal > > > and clone are destructed (and try to delete the registry key) at > > > s/destructed/destroyed/ > > > > > > > > > https://codereview.chromium.org/2926453002/diff/1/services/preferences/tracke... > > > services/preferences/tracked/registry_hash_store_contents_win.cc:99: > > > DCHECK_EQ(other.reset_on_delete_, false); > > > DCHECK(!other.reset_on_delete_); > > > > Tried adding a RefCountedThreadSafe internal object as you suggested. However, > > > I ran into another issue: For each profile, two copies of the > > RegistryHashStoreContentsWin are created: one for unprotected_hash_filter and > > one for protected_hash_filter: > > > https://cs.chromium.org/chromium/src/services/preferences/tracked/tracked_per... > > > > If each RegistryHashStoreContentsWin has its own scoped_refptr, the registry > > deletion logic can be still racy if Chrome shuts down while only one of the > > (protected, unprotected) pref types had a scheduled write. > > (If neither have to write, then both destructors are hit on the main thread. > > If both have to write, then both destructors are hit on the Mojo IO thread). > > > > It sounds like making the RefCountedThreadSafe internal object also be > > BrowserThread::DeleteOnIOThread would prevent the race. However, I don't think > > including "content/public/browser/browser_thread.h" from services is > acceptable. > > WDYT? > > Right we can't make that include. There is base::DeleteOnTaskRunner but it's not > currently compatible with RefCountedThreadSafe sadly (though we could make it > work through hoops, i.e. have the RefCountedThreadSafe object hold a unique_ptr > which is itself using DeleteOnTaskRunner as a deleter...). But that's also not > guaranteed to run right (as the main thread won't be executing tasks on > shutdown)? > > The best way would be to have all stores (protected/unprotected) using the same > key share the same RefCounted cleaner, would it be too much of a mess to pipe it > from ProfilePrefStoreManager or something..?! Good idea. It's a bit of a mess, but otherwise works. LMK what you think.
Nice, I like that best, few nits but lvg https://codereview.chromium.org/2926453002/diff/20001/services/preferences/tr... File services/preferences/tracked/registry_hash_store_contents_win.h (right): https://codereview.chromium.org/2926453002/diff/20001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win.h:23: ~RegistryHashStoreContentsWin() override; Need to keep out of line override per using non-POD members https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... File services/preferences/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win.cc:84: registry_path_ = registry_path; if (resitry_path_.empty()) registry_path_ = registry_path; else DCHECK_EQ(registry_path_, registry_path); (avoids string copy in = case) https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win.cc:88: DCHECK(registry_path_ != L""); DCHECK(!registry_path_.empty()); https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win.cc:97: temp_dir_cleaner_(temp_dir_cleaner) { std::move https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... File services/preferences/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:135: new TempScopedDirRegistryCleaner(); MakeRefCounted<TempScopedDirRegistryCleaner>() https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:136: RegistryHashStoreContentsWin* contentsA = new RegistryHashStoreContentsWin( Use unique_ptr, MakeUnique, and unique_ptr::reset() below https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:154: } Also make a test with a Copy() that runs on another physical base::Thread (to ensure TSAN doesn't complain about any racy bits) https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... File services/preferences/tracked/tracked_persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/tracked_persistent_pref_store_factory.cc:58: temp_dir_cleaner)); std::move https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/tracked_persistent_pref_store_factory.cc:101: temp_scoped_dir_cleaner = new TempScopedDirRegistryCleaner(); MakeRefCounted
https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... File services/preferences/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win.cc:84: registry_path_ = registry_path; On 2017/06/12 15:52:16, gab wrote: > if (resitry_path_.empty()) > registry_path_ = registry_path; > else > DCHECK_EQ(registry_path_, registry_path); > > (avoids string copy in = case) Done. https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win.cc:88: DCHECK(registry_path_ != L""); On 2017/06/12 15:52:16, gab wrote: > DCHECK(!registry_path_.empty()); Done. https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win.cc:97: temp_dir_cleaner_(temp_dir_cleaner) { On 2017/06/12 15:52:16, gab wrote: > std::move Done. https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... File services/preferences/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:135: new TempScopedDirRegistryCleaner(); On 2017/06/12 15:52:16, gab wrote: > MakeRefCounted<TempScopedDirRegistryCleaner>() Done. Just out of curiosity, is this by convention or for performance? https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:136: RegistryHashStoreContentsWin* contentsA = new RegistryHashStoreContentsWin( On 2017/06/12 15:52:16, gab wrote: > Use unique_ptr, MakeUnique, and unique_ptr::reset() below Done. https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:154: } On 2017/06/12 15:52:16, gab wrote: > Also make a test with a Copy() that runs on another physical base::Thread (to > ensure TSAN doesn't complain about any racy bits) Done (not sure what you mean by "with a Copy()", however). https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... File services/preferences/tracked/tracked_persistent_pref_store_factory.cc (right): https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/tracked_persistent_pref_store_factory.cc:58: temp_dir_cleaner)); On 2017/06/12 15:52:17, gab wrote: > std::move Done. https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/tracked_persistent_pref_store_factory.cc:101: temp_scoped_dir_cleaner = new TempScopedDirRegistryCleaner(); On 2017/06/12 15:52:16, gab wrote: > MakeRefCounted Done.
https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... File services/preferences/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:135: new TempScopedDirRegistryCleaner(); On 2017/06/12 18:46:02, proberge wrote: > On 2017/06/12 15:52:16, gab wrote: > > MakeRefCounted<TempScopedDirRegistryCleaner>() > > Done. Just out of curiosity, is this by convention or for performance? We try to avoid "new" as much as possible these days, using smart types + moving them around allows to ensure we don't lose a pointer (and moving them around ensures we don't add counting overhead). https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:154: } On 2017/06/12 18:46:02, proberge wrote: > On 2017/06/12 15:52:16, gab wrote: > > Also make a test with a Copy() that runs on another physical base::Thread (to > > ensure TSAN doesn't complain about any racy bits) > > Done (not sure what you mean by "with a Copy()", however). I meant using HashStoreContents::MakeCopy() just like the initial bug.
https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... File services/preferences/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2926453002/diff/80001/services/preferences/tr... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:154: } On 2017/06/12 18:52:24, gab wrote: > On 2017/06/12 18:46:02, proberge wrote: > > On 2017/06/12 15:52:16, gab wrote: > > > Also make a test with a Copy() that runs on another physical base::Thread > (to > > > ensure TSAN doesn't complain about any racy bits) > > > > Done (not sure what you mean by "with a Copy()", however). > > I meant using HashStoreContents::MakeCopy() just like the initial bug. Ah, gotcha. Done.
lgtm w/ nit https://codereview.chromium.org/2926453002/diff/120001/services/preferences/t... File services/preferences/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2926453002/diff/120001/services/preferences/t... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:199: std::move(contents->MakeCopy())); base::Passed(&contents->MakeCopy())
https://codereview.chromium.org/2926453002/diff/120001/services/preferences/t... File services/preferences/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2926453002/diff/120001/services/preferences/t... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:199: std::move(contents->MakeCopy())); On 2017/06/12 19:35:37, gab wrote: > base::Passed(&contents->MakeCopy()) base::Passed(&contents->MakeCopy()) does not work (class rvalue used as lvalue). MakeCopy returns a std::unique_ptr<HashStoreContents>. Changed to base::Passed(contents->MakeCopy()) instead.
lgtm++ https://codereview.chromium.org/2926453002/diff/120001/services/preferences/t... File services/preferences/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2926453002/diff/120001/services/preferences/t... services/preferences/tracked/registry_hash_store_contents_win_unittest.cc:199: std::move(contents->MakeCopy())); On 2017/06/12 20:04:06, proberge wrote: > On 2017/06/12 19:35:37, gab wrote: > > base::Passed(&contents->MakeCopy()) > > base::Passed(&contents->MakeCopy()) does not work (class rvalue used as lvalue). > MakeCopy returns a std::unique_ptr<HashStoreContents>. > > Changed to base::Passed(contents->MakeCopy()) instead. Ah right, you can either have a T&& or a T* but you can't grab T* of an rvalue ;) : https://cs.chromium.org/chromium/src/base/bind_helpers.h?rcl=41d32a3c1bd0153a...
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2926453002/#ps160001 (title: "Rebase, which also reverts a change from https://chromium-review.googlesource.com/527724")
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2926453002/#ps180001 (title: "Add explicit out of line destructor to fix clang warning")
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": 180001, "attempt_start_ts": 1497362548065780,
"parent_rev": "9c87bf21f52885c923c067ca043c97864ce70614", "commit_rev":
"7c8c48226fd78ad01858284bd2ee91bed27b513c"}
Message was sent while issue was closed.
Description was changed from ========== Fix a race condition in ~RegistryHashStoreContentsWin Enabling mojo-ification of Tracked Preference code highlighted a race condition in integration tests: two instances of RegistryHashStoreContentsWin for the same profile can destructed at the same time on different threads, causing one of the registry operations initiated by the destructor to fail. BUG=727282 ========== to ========== Fix a race condition in ~RegistryHashStoreContentsWin Enabling mojo-ification of Tracked Preference code highlighted a race condition in integration tests: two instances of RegistryHashStoreContentsWin for the same profile can destructed at the same time on different threads, causing one of the registry operations initiated by the destructor to fail. BUG=727282 Review-Url: https://codereview.chromium.org/2926453002 Cr-Commit-Position: refs/heads/master@{#479020} Committed: https://chromium.googlesource.com/chromium/src/+/7c8c48226fd78ad01858284bd2ee... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/7c8c48226fd78ad01858284bd2ee...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:180001) has been created in https://codereview.chromium.org/2937633003/ by dalecurtis@chromium.org. The reason for reverting is: temped_scoped_dir_cleaner is failing, see issue http://crbug.com/732835.
Message was sent while issue was closed.
Description was changed from ========== Fix a race condition in ~RegistryHashStoreContentsWin Enabling mojo-ification of Tracked Preference code highlighted a race condition in integration tests: two instances of RegistryHashStoreContentsWin for the same profile can destructed at the same time on different threads, causing one of the registry operations initiated by the destructor to fail. BUG=727282 Review-Url: https://codereview.chromium.org/2926453002 Cr-Commit-Position: refs/heads/master@{#479020} Committed: https://chromium.googlesource.com/chromium/src/+/7c8c48226fd78ad01858284bd2ee... ========== to ========== Fix a race condition in ~RegistryHashStoreContentsWin Enabling mojo-ification of Tracked Preference code highlighted a race condition in integration tests: two instances of RegistryHashStoreContentsWin for the same profile can destructed at the same time on different threads, causing one of the registry operations initiated by the destructor to fail. BUG=727282 Review-Url: https://codereview.chromium.org/2926453002 Cr-Commit-Position: refs/heads/master@{#479020} Committed: https://chromium.googlesource.com/chromium/src/+/7c8c48226fd78ad01858284bd2ee... ==========
On 2017/06/13 16:18:28, DaleCurtis wrote: > A revert of this CL (patchset #9 id:180001) has been created in > https://codereview.chromium.org/2937633003/ by mailto:dalecurtis@chromium.org. > > The reason for reverting is: temped_scoped_dir_cleaner is failing, see issue > http://crbug.com/732835. Re-opening. Patch set 10 has the BUILD change needed.
On 2017/06/13 16:18:28, DaleCurtis wrote: > A revert of this CL (patchset #9 id:180001) has been created in > https://codereview.chromium.org/2937633003/ by mailto:dalecurtis@chromium.org. > > The reason for reverting is: temped_scoped_dir_cleaner is failing, see issue > http://crbug.com/732835. Re-opening. Patch set 10 has the BUILD change needed.
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2926453002/#ps200001 (title: "Add temp_scoped_dir_cleaner to a build file")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_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 proberge@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
wychen@chromium.org changed reviewers: + wychen@chromium.org
PS9 passed CQ because step "analysis" wrongly skipped target "all".
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497378546761800,
"parent_rev": "72acf2fdedd9a9e895c058f1d19d8e09567fef65", "commit_rev":
"fa05314e25b0cd65e7bee998eeeaed8e2a805366"}
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497378546761800,
"parent_rev": "7e325eee4ea667f7ac01ba53cbd3140eba24245b", "commit_rev":
"42e9e4884b386d2e45bb610bfc91934932255074"}
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497378546761800,
"parent_rev": "ab3a2e8b39dfbf6d9e76b413f995c119da103bb4", "commit_rev":
"0901195b98d8c46db5a0d164e27a896345dc943c"}
Message was sent while issue was closed.
Description was changed from ========== Fix a race condition in ~RegistryHashStoreContentsWin Enabling mojo-ification of Tracked Preference code highlighted a race condition in integration tests: two instances of RegistryHashStoreContentsWin for the same profile can destructed at the same time on different threads, causing one of the registry operations initiated by the destructor to fail. BUG=727282 Review-Url: https://codereview.chromium.org/2926453002 Cr-Commit-Position: refs/heads/master@{#479020} Committed: https://chromium.googlesource.com/chromium/src/+/7c8c48226fd78ad01858284bd2ee... ========== to ========== Fix a race condition in ~RegistryHashStoreContentsWin Enabling mojo-ification of Tracked Preference code highlighted a race condition in integration tests: two instances of RegistryHashStoreContentsWin for the same profile can destructed at the same time on different threads, causing one of the registry operations initiated by the destructor to fail. BUG=727282 Review-Url: https://codereview.chromium.org/2926453002 Cr-Original-Commit-Position: refs/heads/master@{#479020} Committed: https://chromium.googlesource.com/chromium/src/+/7c8c48226fd78ad01858284bd2ee... Review-Url: https://codereview.chromium.org/2926453002 Cr-Commit-Position: refs/heads/master@{#479123} Committed: https://chromium.googlesource.com/chromium/src/+/0901195b98d8c46db5a0d164e27a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/chromium/src/+/0901195b98d8c46db5a0d164e27a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
