sync integration tests rely on urls_with_favicons_ to avoid raciness of mac |
||||||
Issue descriptionAs observed in crbug.com/782315 , certain tests on Mac fail if urls_with_favicons_ is removed. This set is apparently a minor optimization to make tests faster, but it's fishy that tests rely on it, and specifically on Mac. The test is TwoClientBookmarksSyncTest.Sanity which has nothing to do with favicons and the failing expectation is: https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/bookmarks_helper.cc?l=92&rcl=da33c66d427ec0df4fc172162d9abb129bb75b64 From the original failing logs [1], which I could repro on trybot mac_chromium_10.10_macviews, it seems like someone other than BookmarkNodeFaviconChanged() quits the runloop, for example StatusChangeChecker::OnTimeout() (but this particular case doesn't seem to be the case because the test fails fast). [1] https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.12_Tests%2F7078%2F%2B%2Frecipes%2Fsteps%2Fsync_integration_tests_on_Mac-10.12%2F0%2Fstdout
,
Nov 15 2017
This could be a good starter bug for one of the Munich folks.
,
Nov 22 2017
I took one more look and start to understand what's going on. I'll take care of this bug.
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/07edac9ec1a40b737569afb0d4db3a2402c3f39b commit 07edac9ec1a40b737569afb0d4db3a2402c3f39b Author: Mikel Astiz <mastiz@chromium.org> Date: Tue Nov 28 13:00:47 2017 Migrate away from deprecated RunLoop API Instead, we keep around an instance of base::RunLoop and pass it explicitly for Quit(), which makes sure the code quits the intended loop and not a nested one. Bug: 783774 Change-Id: I3ea5092699bbae127f581e5c955cb27904d98f0a Reviewed-on: https://chromium-review.googlesource.com/784790 Reviewed-by: Pavel Yatsuk <pavely@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#519664} [modify] https://crrev.com/07edac9ec1a40b737569afb0d4db3a2402c3f39b/chrome/browser/sync/test/integration/bookmarks_helper.cc [modify] https://crrev.com/07edac9ec1a40b737569afb0d4db3a2402c3f39b/chrome/browser/sync/test/integration/passwords_helper.cc [modify] https://crrev.com/07edac9ec1a40b737569afb0d4db3a2402c3f39b/chrome/browser/sync/test/integration/status_change_checker.cc [modify] https://crrev.com/07edac9ec1a40b737569afb0d4db3a2402c3f39b/chrome/browser/sync/test/integration/status_change_checker.h
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0861e6f564663c53fa7f41c25563b424eef6f77 commit a0861e6f564663c53fa7f41c25563b424eef6f77 Author: Henrik Boström <hbos@chromium.org> Date: Tue Nov 28 14:41:26 2017 Revert "Migrate away from deprecated RunLoop API" This reverts commit 07edac9ec1a40b737569afb0d4db3a2402c3f39b. Reason for revert: Speculative revert for sync_integration_tests / TwoClientPasswordsSyncTest.SetPassphraseAndThenSetupSync failure on https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/47531 Original change's description: > Migrate away from deprecated RunLoop API > > Instead, we keep around an instance of base::RunLoop and pass it > explicitly for Quit(), which makes sure the code quits the intended loop > and not a nested one. > > Bug: 783774 > Change-Id: I3ea5092699bbae127f581e5c955cb27904d98f0a > Reviewed-on: https://chromium-review.googlesource.com/784790 > Reviewed-by: Pavel Yatsuk <pavely@chromium.org> > Commit-Queue: Mikel Astiz <mastiz@chromium.org> > Cr-Commit-Position: refs/heads/master@{#519664} TBR=pavely@chromium.org,mastiz@chromium.org Change-Id: I25ed5a61f5b9e13159af82c4ef6d6f652170d4a4 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 783774 Reviewed-on: https://chromium-review.googlesource.com/793630 Reviewed-by: Henrik Boström <hbos@chromium.org> Commit-Queue: Henrik Boström <hbos@chromium.org> Cr-Commit-Position: refs/heads/master@{#519680} [modify] https://crrev.com/a0861e6f564663c53fa7f41c25563b424eef6f77/chrome/browser/sync/test/integration/bookmarks_helper.cc [modify] https://crrev.com/a0861e6f564663c53fa7f41c25563b424eef6f77/chrome/browser/sync/test/integration/passwords_helper.cc [modify] https://crrev.com/a0861e6f564663c53fa7f41c25563b424eef6f77/chrome/browser/sync/test/integration/status_change_checker.cc [modify] https://crrev.com/a0861e6f564663c53fa7f41c25563b424eef6f77/chrome/browser/sync/test/integration/status_change_checker.h
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8d9405ee5f9be31c742c98a6f9e3518e44d741b commit d8d9405ee5f9be31c742c98a6f9e3518e44d741b Author: Mikel Astiz <mastiz@chromium.org> Date: Thu Nov 30 14:50:44 2017 Reland "Migrate away from deprecated RunLoop API" In this second attempt, we stick to QuitWhenIdle() for passwords, with no apparent legit reason. TODO added for further investigation. This is a reland of 07edac9ec1a40b737569afb0d4db3a2402c3f39b Original change's description: > Migrate away from deprecated RunLoop API > > Instead, we keep around an instance of base::RunLoop and pass it > explicitly for Quit(), which makes sure the code quits the intended loop > and not a nested one. > > Bug: 783774 > Change-Id: I3ea5092699bbae127f581e5c955cb27904d98f0a > Reviewed-on: https://chromium-review.googlesource.com/784790 > Reviewed-by: Pavel Yatsuk <pavely@chromium.org> > Commit-Queue: Mikel Astiz <mastiz@chromium.org> > Cr-Commit-Position: refs/heads/master@{#519664} TBR=pavely@chromium.org Bug: 783774 Change-Id: Ia93b738ad34e78289d938809f61e6c6d06c4bc80 Reviewed-on: https://chromium-review.googlesource.com/793910 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#520539} [modify] https://crrev.com/d8d9405ee5f9be31c742c98a6f9e3518e44d741b/chrome/browser/sync/test/integration/bookmarks_helper.cc [modify] https://crrev.com/d8d9405ee5f9be31c742c98a6f9e3518e44d741b/chrome/browser/sync/test/integration/passwords_helper.cc [modify] https://crrev.com/d8d9405ee5f9be31c742c98a6f9e3518e44d741b/chrome/browser/sync/test/integration/status_change_checker.cc [modify] https://crrev.com/d8d9405ee5f9be31c742c98a6f9e3518e44d741b/chrome/browser/sync/test/integration/status_change_checker.h
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b99fd086f6645833061e7c3fe1db3d96a254057 commit 7b99fd086f6645833061e7c3fe1db3d96a254057 Author: Mikel Astiz <mastiz@chromium.org> Date: Thu Nov 30 16:37:31 2017 Revert "Reland "Migrate away from deprecated RunLoop API"" This reverts commit d8d9405ee5f9be31c742c98a6f9e3518e44d741b. Reason for revert: <INSERT REASONING HERE> Original change's description: > Reland "Migrate away from deprecated RunLoop API" > > In this second attempt, we stick to QuitWhenIdle() for passwords, with > no apparent legit reason. TODO added for further investigation. > > This is a reland of 07edac9ec1a40b737569afb0d4db3a2402c3f39b > Original change's description: > > Migrate away from deprecated RunLoop API > > > > Instead, we keep around an instance of base::RunLoop and pass it > > explicitly for Quit(), which makes sure the code quits the intended loop > > and not a nested one. > > > > Bug: 783774 > > Change-Id: I3ea5092699bbae127f581e5c955cb27904d98f0a > > Reviewed-on: https://chromium-review.googlesource.com/784790 > > Reviewed-by: Pavel Yatsuk <pavely@chromium.org> > > Commit-Queue: Mikel Astiz <mastiz@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#519664} > > TBR=pavely@chromium.org > > Bug: 783774 > Change-Id: Ia93b738ad34e78289d938809f61e6c6d06c4bc80 > Reviewed-on: https://chromium-review.googlesource.com/793910 > Reviewed-by: Mikel Astiz <mastiz@chromium.org> > Commit-Queue: Mikel Astiz <mastiz@chromium.org> > Cr-Commit-Position: refs/heads/master@{#520539} TBR=pavely@chromium.org,mastiz@chromium.org Change-Id: I82972cfe1b3b7b33dd962a8fabb001bda9791c89 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 783774 Reviewed-on: https://chromium-review.googlesource.com/801111 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#520570} [modify] https://crrev.com/7b99fd086f6645833061e7c3fe1db3d96a254057/chrome/browser/sync/test/integration/bookmarks_helper.cc [modify] https://crrev.com/7b99fd086f6645833061e7c3fe1db3d96a254057/chrome/browser/sync/test/integration/passwords_helper.cc [modify] https://crrev.com/7b99fd086f6645833061e7c3fe1db3d96a254057/chrome/browser/sync/test/integration/status_change_checker.cc [modify] https://crrev.com/7b99fd086f6645833061e7c3fe1db3d96a254057/chrome/browser/sync/test/integration/status_change_checker.h
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c000b6b59005fb58ea664dee4d3ec132d07147bb commit c000b6b59005fb58ea664dee4d3ec132d07147bb Author: Avi Drissman <avi@chromium.org> Date: Thu Nov 30 16:47:37 2017 Revert "Reland "Migrate away from deprecated RunLoop API"" This reverts commit d8d9405ee5f9be31c742c98a6f9e3518e44d741b. Reason for revert: Builder: Mac10.9 Tests (dbg) https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29 started breaking in TwoClientPasswordsSyncTest.SetPassphraseAndThenSetupSync and this is the only sync change. Sheriff notes say that this was reverted the first time for this exact test breakage. Please DO NOT RELAND until you ensure the test won't fail. Original change's description: > Reland "Migrate away from deprecated RunLoop API" > > In this second attempt, we stick to QuitWhenIdle() for passwords, with > no apparent legit reason. TODO added for further investigation. > > This is a reland of 07edac9ec1a40b737569afb0d4db3a2402c3f39b > Original change's description: > > Migrate away from deprecated RunLoop API > > > > Instead, we keep around an instance of base::RunLoop and pass it > > explicitly for Quit(), which makes sure the code quits the intended loop > > and not a nested one. > > > > Bug: 783774 > > Change-Id: I3ea5092699bbae127f581e5c955cb27904d98f0a > > Reviewed-on: https://chromium-review.googlesource.com/784790 > > Reviewed-by: Pavel Yatsuk <pavely@chromium.org> > > Commit-Queue: Mikel Astiz <mastiz@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#519664} > > TBR=pavely@chromium.org > > Bug: 783774 > Change-Id: Ia93b738ad34e78289d938809f61e6c6d06c4bc80 > Reviewed-on: https://chromium-review.googlesource.com/793910 > Reviewed-by: Mikel Astiz <mastiz@chromium.org> > Commit-Queue: Mikel Astiz <mastiz@chromium.org> > Cr-Commit-Position: refs/heads/master@{#520539} TBR=pavely@chromium.org,mastiz@chromium.org Change-Id: I1f45e1795eaa8d7a8aa22a7af6d87b5cb173ff0a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 783774 Reviewed-on: https://chromium-review.googlesource.com/801170 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#520576}
,
Dec 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f2dd6b5d9db503563c2cd8a6e79b79b09075d87 commit 3f2dd6b5d9db503563c2cd8a6e79b79b09075d87 Author: Mikel Astiz <mastiz@chromium.org> Date: Wed Dec 06 10:14:21 2017 Reland "Migrate away from deprecated RunLoop API" This is a second reland attempt for 07edac9ec1a40b737569afb0d4db3a2402c3f39b after a fix for the underlying issue has landed: https://chromium-review.googlesource.com/c/chromium/src/+/807664 Original change's description: > Migrate away from deprecated RunLoop API > > Instead, we keep around an instance of base::RunLoop and pass it > explicitly for Quit(), which makes sure the code quits the intended loop > and not a nested one. > > Bug: 783774 > Change-Id: I3ea5092699bbae127f581e5c955cb27904d98f0a > Reviewed-on: https://chromium-review.googlesource.com/784790 > Reviewed-by: Pavel Yatsuk <pavely@chromium.org> > Commit-Queue: Mikel Astiz <mastiz@chromium.org> > Cr-Commit-Position: refs/heads/master@{#519664} TBR=pavely@chromium.org Bug: 783774 Change-Id: I615b4a92ff59a8f4cf871d63327bb8ba063d1cfc Reviewed-on: https://chromium-review.googlesource.com/803314 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#522055} [modify] https://crrev.com/3f2dd6b5d9db503563c2cd8a6e79b79b09075d87/chrome/browser/sync/test/integration/bookmarks_helper.cc [modify] https://crrev.com/3f2dd6b5d9db503563c2cd8a6e79b79b09075d87/chrome/browser/sync/test/integration/passwords_helper.cc [modify] https://crrev.com/3f2dd6b5d9db503563c2cd8a6e79b79b09075d87/chrome/browser/sync/test/integration/status_change_checker.cc [modify] https://crrev.com/3f2dd6b5d9db503563c2cd8a6e79b79b09075d87/chrome/browser/sync/test/integration/status_change_checker.h
,
Dec 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/692e82ab8be4141d326bcb599f59a4c73af8e6ab commit 692e82ab8be4141d326bcb599f59a4c73af8e6ab Author: Mikel Astiz <mastiz@chromium.org> Date: Wed Dec 06 15:18:45 2017 Avoid weird nested runloops while verifying model in sync tests The use of WaitForGetFavicon() is unnecessary and misleading because it introduces a nested runloop where many things can happen, including model changes that call BookmarksMatchVerifierChecker::IsExitConditionSatisfied() and hence make it reentrant. It is unnecessary because, starting with 55b3fe20e2e1c5586d54004447d2953fee767599, a loading favicon is guaranteed to ultimately notify a model change, which will trigger MultiClientStatusChangeChecker::OnStateChanged() and hence IsExitConditionSatisfied(). Bug: 783774 Change-Id: I02c7623e3bb173b25b9c715b7bdf9c646300e3ee Reviewed-on: https://chromium-review.googlesource.com/785091 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Pavel Yatsuk <pavely@chromium.org> Cr-Commit-Position: refs/heads/master@{#522089} [modify] https://crrev.com/692e82ab8be4141d326bcb599f59a4c73af8e6ab/chrome/browser/sync/test/integration/bookmarks_helper.cc [modify] https://crrev.com/692e82ab8be4141d326bcb599f59a4c73af8e6ab/components/bookmarks/browser/bookmark_model.h
,
Jan 17 2018
,
Jul 3
,
Oct 24
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by mastiz@chromium.org
, Nov 15 2017