Race in ~RegistryHashStoreContentsWin() |
||||||||||||
Issue descriptionsync_integration_tests failing on chromium.win/Win7 Tests (dbg)(1) Builders failed on: - Win7 Tests (dbg)(1): https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29 It looks like this is a problem of the bot itself.
,
May 29 2017
,
May 29 2017
,
May 29 2017
Speculatively reverting suspect CL here: https://chromium-review.googlesource.com/c/517487 Will reland if the revert does not work.
,
May 29 2017
The revert worked. Closing this. The bot is now failing for other reason, but the sync_integration_tests now pass.
,
May 29 2017
Unfortunately the revert did not work. After sync_integration_tests succeeded in run 60386 (after failing 10 times consecutively), they failed again in run 60387. See https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60386 https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60387 I am relanding the reverted CL here: https://chromium-review.googlesource.com/c/517492
,
May 30 2017
Still failing https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60402 SingleClientAppsSyncTest.InstallSomePlatformApps https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60401 TwoClientAppsSyncTest.InstallDifferentApps_E2ETest https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60400 SingleClientAppsSyncTest.StartWithSomeLegacyApps TwoClientExtensionsSyncTest.StartWithDifferentExtensions_E2ETest TwoClientExtensionsSyncTest.InstallDifferentExtensions_E2ETest TwoClientAppsSyncTest.InstallDifferentApps_E2ETest https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60399 SingleClientBookmarksSyncTest.InjectedBookmark TwoClientAppsSyncTest.UpdateAppLaunchOrdinal_E2ETest TwoClientUssSyncTest.Error SingleClientExtensionsSyncTest.StartWithNoExtensions TwoClientSearchEnginesSyncTest.DeleteSyncedDefault_E2ETest MigrationSingleClientTest.AllTypesAtOnce TwoClientUssSyncTest.Encryption TwoClientExtensionsSyncTest.InstallDifferentExtensions_E2ETest TwoClientBookmarksSyncTest.SingleClientEnabledEncryption TwoClientBookmarksSyncTest.MC_MergeSimpleBMHierarchyEqualSetsUnderBMBar TwoClientUssSyncTest.Sanity TwoClientAppsSyncTest.BookmarkApp TwoClientAutofillSyncTest.MaxLength TwoClientBookmarksSyncTest.SC_ReverseTheOrderOf10BMs SyncAuthTest.RetryOnMalformedToken SyncAuthTest.TokenExpiry TwoClientPasswordsSyncTest.Delete TwoClientBookmarksSyncTest.MC_MergeSimpleBMHierarchySubsetUnderBookmarkBar TwoClientBookmarksSyncTest.SC_SinkEmptyBMFold5LevelsDown MigrationSingleClientTest.PrefsOnlyTriggerNotification TwoClientDictionarySyncTest.AddDifferentToEach_E2ETest TwoClientSearchEnginesSyncTest.Delete_E2ETest SingleClientWalletSyncTest.EnabledByDefault SingleClientTypedUrlsSyncTest.Sanity TwoClientExtensionsSyncTest.StartWithSameExtensions_E2ETest SyncErrorTest.ErrorWhileSettingUp SingleClientPasswordManagerSettingMigratorServiceSyncTest.LocalOnOffSyncOffOn TwoClientExtensionsSyncTest.StartWithDifferentExtensions_E2ETest TwoClientExtensionSettingsAndAppSettingsSyncTest.ExtensionsStartWithDifferentSettings TwoClientSessionsSyncTest.BothClientsEnabledEncryption TwoClientSearchEnginesSyncTest.Add_E2ETest TwoClientExtensionsSyncTest.UpdateIncognitoEnableDisable_E2ETest TwoClientBookmarksSyncTest.SC_ReverseTheOrderOfTenBMFolders https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60394 TwoClientExtensionsSyncTest.StartWithDifferentExtensions_E2ETest TwoClientExtensionsSyncTest.InstallDifferentExtensions_E2ETest
,
May 30 2017
Looked at a few of the recent failures. All of them are failing at registry_hash_store_contents_win.cc CC-ing one of the OWNERS of it: gab@ Assigning to one of the OWNERS of the sync tests: pavely@
,
May 30 2017
Perhaps prefs+Mojo related? Looks like the RegistryHashStoreContentsWin ultimately owned by the service_manager::Service is deleted from a context that doesn't have access to the registry (5 is ERROR_ACCESS_DENIED). Calling Reset() from ~RegistryHashStoreContentsWin() is new though (r472565 to solve another important issue) so this may not be a recent prefs+Mojo commit but still something to figure out in the scope of that project IMO. @sammc: PTaL, thanks! [4212:4868:0529/181304.186:FATAL:registry_hash_store_contents_win.cc(108)] Check failed: result == ERROR_SUCCESS || result == ERROR_FILE_NOT_FOUND. 5 Backtrace: base::debug::StackTrace::StackTrace [0x06BADDC7+55] base::debug::StackTrace::StackTrace [0x06BADA61+17] logging::LogMessage::~LogMessage [0x06C01D2E+94] RegistryHashStoreContentsWin::Reset [0x012546B3+195] RegistryHashStoreContentsWin::~RegistryHashStoreContentsWin [0x01253D13+35]
,
May 30 2017
I did some digging on the Win7 Tests bot and I found that the failures started happening with https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60372 but retry mechanism hide the errors for all builds up to the first red one in https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/60374 (I checked the swarming job output and searched for FATAL:registry_hash_store_contents_win.cc) Because of that, I believe the error is https://chromium-review.googlesource.com/c/517604/ so I'm going to revert that one.
,
May 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c404ba2c32273e73801f36c7c8b49abe76de779e commit c404ba2c32273e73801f36c7c8b49abe76de779e Author: Henrik Kjellander <kjellander@chromium.org> Date: Tue May 30 08:10:06 2017 Revert "Enable PrefService by default." This reverts commit 33eaae62b5c0005b2959393bb473445a7d77bb4f. Reason for revert: Probable culprit for crbug.com/727282 (flakiness and failures in sync_integration_tests on Win7 Tests bot). Original change's description: > Enable PrefService by default. > > Bug: 654988 ,707602 > Change-Id: I4abf66b0c333d6ab27f1239b18a24576967a8835 > Reviewed-on: https://chromium-review.googlesource.com/517604 > Reviewed-by: Johan Tibell <tibell@chromium.org> > Commit-Queue: Sam McNally <sammc@chromium.org> > Cr-Commit-Position: refs/heads/master@{#475300} TBR=sammc@chromium.org,tibell@chromium.org No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 654988 ,707602, 727282 Change-Id: I593fe2b0ea00b379f394cbdd214cee6330d27297 Reviewed-on: https://chromium-review.googlesource.com/517494 Commit-Queue: Henrik Kjellander <kjellander@chromium.org> Reviewed-by: Henrik Kjellander <kjellander@chromium.org> Cr-Commit-Position: refs/heads/master@{#475459} [modify] https://crrev.com/c404ba2c32273e73801f36c7c8b49abe76de779e/chrome/common/chrome_features.cc
,
May 30 2017
The revert in #11 solved this. Several green builds at https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29 now.
,
May 31 2017
I think this is caused by multiple RegistryHashStoreContentsWin instances on different threads, pointing at the same key. One holds a handle to the key (or a subkey) while another tries to delete it. I suspect the copies made in PrefHashFilter::GetOnWriteSynchronousCallbacks() are responsible; further, they could be deleting registry keys in the middle of tests if they take long enough.
,
May 31 2017
Only of this classes should only be accessed on the main thread so racing isn't possible (unless you meant between parallel tests instead of between threads?)
,
May 31 2017
Theory: enabling PrefService highlighted an existing issue that was never hit before: Reset can be called from a worker thread (from one of the clones). Without PrefService, the non-clone RegistryHashStoreContentsWin's dtor is called on the main thread. With PrefService, it's called on an IO thread. It's likely that the issue didn't trigger when it was (Main + Worker) but does trigger when it's (IO + Worker).
,
May 31 2017
Interesting, the copies should sure use the same execution sequence as their parent (or be read-only -- I forget the exact use case)
,
May 31 2017
The copies are created here: https://cs.chromium.org/chromium/src/services/preferences/tracked/pref_hash_filter.cc?q=GetOnWriteSynchronousCallbacks&dr=CSs&l=352 The copy is passed to the callback, which owns them. The callback is bound and posted on a task here: https://cs.chromium.org/chromium/src/base/files/important_file_writer.cc?l=183 This was done to allow the registry write to properly mirror the file write when the browser is shutting down. The main thread terminates before prefs are written to disk by the ImportantFileWriter, so we needed some fragment of RegistryHashStoreContentsWin to survive during shutdown to write to the registry.
,
Jun 1 2017
Got it so ultimately what we want is to cleanup once (unracily) on shutdown in tests after all writes... Let me know if you need help hammering a solution for this.
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cd0c21dfb32b6235c5c4bed740dac090b4e25bae commit cd0c21dfb32b6235c5c4bed740dac090b4e25bae Author: Sam McNally <sammc@chromium.org> Date: Fri Jun 09 01:31:36 2017 Enable PrefService by default. This also changes a DCHECK in RegistryHashStoreContentsWin to be more permissive to workaround crbug.com/727282 , which is triggered more reliably with PrefService enabled. Bug: 654988 , 707602, 727282 Change-Id: I39e94dd6d2c7360d8763608a1ac9897f98141902 Reviewed-on: https://chromium-review.googlesource.com/527959 Reviewed-by: Johan Tibell <tibell@chromium.org> Commit-Queue: Sam McNally <sammc@chromium.org> Cr-Commit-Position: refs/heads/master@{#478157} [modify] https://crrev.com/cd0c21dfb32b6235c5c4bed740dac090b4e25bae/chrome/common/chrome_features.cc [modify] https://crrev.com/cd0c21dfb32b6235c5c4bed740dac090b4e25bae/services/preferences/tracked/registry_hash_store_contents_win.cc
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cdfa351523e32ad73579797f54483fd7c6edc3be commit cdfa351523e32ad73579797f54483fd7c6edc3be Author: Sam McNally <sammc@chromium.org> Date: Fri Jun 09 05:36:19 2017 Allow ERROR_KEY_DELETED in the DCHECK in RegistryHashStoreContentsWin::Reset(). Bug: 727282 Change-Id: I04e3367680ebe7de5c6ae291a009368e1832175f Reviewed-on: https://chromium-review.googlesource.com/527724 Reviewed-by: Johan Tibell <tibell@chromium.org> Commit-Queue: Sam McNally <sammc@chromium.org> Cr-Commit-Position: refs/heads/master@{#478208} [modify] https://crrev.com/cdfa351523e32ad73579797f54483fd7c6edc3be/services/preferences/tracked/registry_hash_store_contents_win.cc
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7c8c48226fd78ad01858284bd2ee91bed27b513c commit 7c8c48226fd78ad01858284bd2ee91bed27b513c Author: proberge <proberge@chromium.org> Date: Tue Jun 13 15:04:51 2017 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} [modify] https://crrev.com/7c8c48226fd78ad01858284bd2ee91bed27b513c/services/preferences/tracked/registry_hash_store_contents_win.cc [modify] https://crrev.com/7c8c48226fd78ad01858284bd2ee91bed27b513c/services/preferences/tracked/registry_hash_store_contents_win.h [modify] https://crrev.com/7c8c48226fd78ad01858284bd2ee91bed27b513c/services/preferences/tracked/registry_hash_store_contents_win_unittest.cc [add] https://crrev.com/7c8c48226fd78ad01858284bd2ee91bed27b513c/services/preferences/tracked/temp_scoped_dir_cleaner.h [modify] https://crrev.com/7c8c48226fd78ad01858284bd2ee91bed27b513c/services/preferences/tracked/tracked_persistent_pref_store_factory.cc
,
Jun 13 2017
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a3a1cec34a434ca7c0ef4ac4f52b9bd8540873af commit a3a1cec34a434ca7c0ef4ac4f52b9bd8540873af Author: dalecurtis <dalecurtis@chromium.org> Date: Tue Jun 13 16:19:07 2017 Revert of Fix a race condition in ~RegistryHashStoreContentsWin (patchset #9 id:180001 of https://codereview.chromium.org/2926453002/ ) Reason for revert: temped_scoped_dir_cleaner is failing, see issue http://crbug.com/732835 Original issue's description: > 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/+/7c8c48226fd78ad01858284bd2ee91bed27b513c TBR=gab@chromium.org,proberge@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 727282 Review-Url: https://codereview.chromium.org/2937633003 Cr-Commit-Position: refs/heads/master@{#479037} [modify] https://crrev.com/a3a1cec34a434ca7c0ef4ac4f52b9bd8540873af/services/preferences/tracked/registry_hash_store_contents_win.cc [modify] https://crrev.com/a3a1cec34a434ca7c0ef4ac4f52b9bd8540873af/services/preferences/tracked/registry_hash_store_contents_win.h [modify] https://crrev.com/a3a1cec34a434ca7c0ef4ac4f52b9bd8540873af/services/preferences/tracked/registry_hash_store_contents_win_unittest.cc [delete] https://crrev.com/0a02ecd5e26cd16d87a195f627885abddeeed89b/services/preferences/tracked/temp_scoped_dir_cleaner.h [modify] https://crrev.com/a3a1cec34a434ca7c0ef4ac4f52b9bd8540873af/services/preferences/tracked/tracked_persistent_pref_store_factory.cc
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0901195b98d8c46db5a0d164e27a896345dc943c commit 0901195b98d8c46db5a0d164e27a896345dc943c Author: proberge <proberge@chromium.org> Date: Tue Jun 13 20:18:54 2017 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/+/7c8c48226fd78ad01858284bd2ee91bed27b513c Review-Url: https://codereview.chromium.org/2926453002 Cr-Commit-Position: refs/heads/master@{#479123} [modify] https://crrev.com/0901195b98d8c46db5a0d164e27a896345dc943c/services/preferences/tracked/BUILD.gn [modify] https://crrev.com/0901195b98d8c46db5a0d164e27a896345dc943c/services/preferences/tracked/registry_hash_store_contents_win.cc [modify] https://crrev.com/0901195b98d8c46db5a0d164e27a896345dc943c/services/preferences/tracked/registry_hash_store_contents_win.h [modify] https://crrev.com/0901195b98d8c46db5a0d164e27a896345dc943c/services/preferences/tracked/registry_hash_store_contents_win_unittest.cc [add] https://crrev.com/0901195b98d8c46db5a0d164e27a896345dc943c/services/preferences/tracked/temp_scoped_dir_cleaner.h [modify] https://crrev.com/0901195b98d8c46db5a0d164e27a896345dc943c/services/preferences/tracked/tracked_persistent_pref_store_factory.cc
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5e829660f4e678f3fd1edc36b6a6785de4714bb4 commit 5e829660f4e678f3fd1edc36b6a6785de4714bb4 Author: Primiano Tucci <primiano@chromium.org> Date: Wed Jun 28 17:07:53 2017 Revert "Enable PrefService by default." This reverts commit cd0c21dfb32b6235c5c4bed740dac090b4e25bae. Reason for revert: The CL caused perf regressions ( crbug.com/732294 ). No immediate plan to address the problem showed up. Reverting in accordance with perf sheriffing rules in https://chromium.googlesource.com/chromium/src/+/master/docs/speed/perf_regression_sheriffing.md Original change's description: > Enable PrefService by default. > > This also changes a DCHECK in RegistryHashStoreContentsWin to be more > permissive to workaround crbug.com/727282 , which is triggered more > reliably with PrefService enabled. > > Bug: 654988 , 707602, 727282 > Change-Id: I39e94dd6d2c7360d8763608a1ac9897f98141902 > Reviewed-on: https://chromium-review.googlesource.com/527959 > Reviewed-by: Johan Tibell <tibell@chromium.org> > Commit-Queue: Sam McNally <sammc@chromium.org> > Cr-Commit-Position: refs/heads/master@{#478157} TBR=sammc@chromium.org,jam@chromium.org Bug: 732294 , 654988 , 707602, 727282 Change-Id: I2636bf309da1f32b76f5c6eff3e10145e769f1a2 Reviewed-on: https://chromium-review.googlesource.com/552142 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Cr-Commit-Position: refs/heads/master@{#483036} [modify] https://crrev.com/5e829660f4e678f3fd1edc36b6a6785de4714bb4/chrome/common/chrome_features.cc |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by hajimehoshi@chromium.org
, May 29 2017