New issue
Advanced search Search tips

Issue 910947 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 4
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug
Flaky-Test: SingleClientSessionsSyncTest.NavigateThenCloseTabThenOpenTab



Sign in to add a comment

SingleClientSessionsSyncTest.NavigateThenCloseTabThenOpenTab is flaky

Project Member Reported by Findit, Dec 2

Issue description


Flaky test: SingleClientSessionsSyncTest.NavigateThenCloseTabThenOpenTab
Sample failed build due to flakiness: https://ci.chromium.org/buildbot/chromium.mac/Mac10.11%20Tests/31315
Test output log: https://chromium-swarm.appspot.com/task?id=4188364101b6d610
Culprit (100.0% confidence): r612971
Analysis: https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVy4gELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCKrAWNocm9taXVtLm1hYy9NYWMxMC4xMSBUZXN0cy8zMTMxNS9zeW5jX2ludGVncmF0aW9uX3Rlc3RzIG9uIChub25lKSBHUFUgb24gTWFjIG9uIE1hYy0xMC4xMS9VMmx1WjJ4bFEyeHBaVzUwVTJWemMybHZibk5UZVc1alZHVnpkQzVPWVhacFoyRjBaVlJvWlc1RGJHOXpaVlJoWWxSb1pXNVBjR1Z1VkdGaQwLEhNNYXN0ZXJGbGFrZUFuYWx5c2lzGAEM

Please revert the culprit, or disable the test and find the appropriate owner.

If the culprit above is wrong, please file a bug using this link:
https://bugs.chromium.org/p/chromium/issues/entry?status=Unconfirmed&labels=Pri-1,Test-Findit-Wrong&components=Tools%3ETest%3EFindit%3EFlakiness&summary=%5BFindit%5D%20Flake%20Analyzer%20-%20Wrong%20result%20for%20SingleClientSessionsSyncTest.NavigateThenCloseTabThenOpenTab&comment=Link%20to%20Analysis%3A%20https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVy4gELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCKrAWNocm9taXVtLm1hYy9NYWMxMC4xMSBUZXN0cy8zMTMxNS9zeW5jX2ludGVncmF0aW9uX3Rlc3RzIG9uIChub25lKSBHUFUgb24gTWFjIG9uIE1hYy0xMC4xMS9VMmx1WjJ4bFEyeHBaVzUwVTJWemMybHZibk5UZVc1alZHVnpkQzVPWVhacFoyRjBaVlJvWlc1RGJHOXpaVlJoWWxSb1pXNVBjR1Z1VkdGaQwLEhNNYXN0ZXJGbGFrZUFuYWx5c2lzGAEM

Automatically posted by the findit-for-me app (https://goo.gl/Ot9f7N).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/949ec112f644666953284e5f205311d31e638062

commit 949ec112f644666953284e5f205311d31e638062
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Sun Dec 02 15:08:17 2018

Revert "Avoid recycling sync tabs if commit pending"

This reverts commit 62414cfcde8b704599847d6acc731d254479655f.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 612971 as the
culprit for flakes in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vNjI0MTRjZmNkZThiNzA0NTk5ODQ3ZDZhY2M3MzFkMjU0NDc5NjU1Zgw

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.mac/Mac10.11%20Tests/31315

Sample Failed Step: sync_integration_tests on (none) GPU on Mac on Mac-10.11

Sample Flaky Test: SingleClientSessionsSyncTest.NavigateThenCloseTabThenOpenTab

Original change's description:
> Avoid recycling sync tabs if commit pending
> 
> When a tab is closed, it's possible that the corresponding history
> hasn't been committed yet, and hence there is a risk that synced history
> is lost if the entity is recycled (for another tab that is opened).
> 
> In this patch, and behind a feature toggle, this issue is prevented by
> *not* freeing tab nodes while the sync entity is unsynced. Old tabs are
> excluded from this (to avoid problems with expired history) and a max
> cap is also introduced to the number of tabs in this state, in order to
> avoid memory regressions.
> 
> Bug: 882489
> Change-Id: I6dd796642f9553f2713a0814731897a4ffb13f0b
> Reviewed-on: https://chromium-review.googlesource.com/c/1356541
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612971}

Change-Id: I7735b817751abcbd18bded6a731115333ee50b26
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 882489,  910947 
Reviewed-on: https://chromium-review.googlesource.com/c/1357999
Cr-Commit-Position: refs/heads/master@{#612976}
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/local_session_event_handler_impl.cc
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/local_session_event_handler_impl.h
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/local_session_event_handler_impl_unittest.cc
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/session_sync_bridge.h
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/test_synced_window_delegates_getter.cc
[modify] https://crrev.com/949ec112f644666953284e5f205311d31e638062/components/sync_sessions/test_synced_window_delegates_getter.h

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f40e8ac5a8c078f2921430a6b77560529253470d

commit f40e8ac5a8c078f2921430a6b77560529253470d
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Sun Dec 02 21:01:37 2018

Revert "Reland "Avoid recycling sync tabs if commit pending""

This reverts commit a61d2bb4304df828da60af07f0b05f33e49fd18f.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 612980 as the
culprit for flakes in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vYTYxZDJiYjQzMDRkZjgyOGRhNjBhZjA3ZjBiMDVmMzNlNDlmZDE4Zgw

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/9304

Sample Failed Step: sync_integration_tests

Sample Flaky Test: SingleClientSessionsSyncTest.NavigateThenCloseTabThenOpenTab

Original change's description:
> Reland "Avoid recycling sync tabs if commit pending"
> 
> This is a reland of 62414cfcde8b704599847d6acc731d254479655f
> 
> Test improved to avoid flakes.
> 
> Original change's description:
> > Avoid recycling sync tabs if commit pending
> >
> > When a tab is closed, it's possible that the corresponding history
> > hasn't been committed yet, and hence there is a risk that synced history
> > is lost if the entity is recycled (for another tab that is opened).
> >
> > In this patch, and behind a feature toggle, this issue is prevented by
> > *not* freeing tab nodes while the sync entity is unsynced. Old tabs are
> > excluded from this (to avoid problems with expired history) and a max
> > cap is also introduced to the number of tabs in this state, in order to
> > avoid memory regressions.
> >
> > Bug: 882489
> > Change-Id: I6dd796642f9553f2713a0814731897a4ffb13f0b
> > Reviewed-on: https://chromium-review.googlesource.com/c/1356541
> > Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> > Reviewed-by: Marc Treib <treib@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#612971}
> 
> TBR=treib@chromium.org
> 
> Bug: 882489
> Change-Id: Ib3f3ff9e4d620512435d9b22ba7b2c338f92203f
> Reviewed-on: https://chromium-review.googlesource.com/c/1357086
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612980}

Change-Id: I69bf304dc196b29f2b6ccf5f15ff8dd77ca7d610
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 882489,  910947 
Reviewed-on: https://chromium-review.googlesource.com/c/1357826
Cr-Commit-Position: refs/heads/master@{#612982}
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/local_session_event_handler_impl.cc
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/local_session_event_handler_impl.h
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/local_session_event_handler_impl_unittest.cc
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/session_sync_bridge.h
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/synced_session_tracker.cc
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/synced_session_tracker.h
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/synced_session_tracker_unittest.cc
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/tab_node_pool.h
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/test_synced_window_delegates_getter.cc
[modify] https://crrev.com/f40e8ac5a8c078f2921430a6b77560529253470d/components/sync_sessions/test_synced_window_delegates_getter.h

Components: Tests>Flaky
Labels: -Pri-1 -Sheriff-Chromium Pri-2
Owner: mastiz@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed1c99d3f024717637faa7f13457109d45f5addf

commit ed1c99d3f024717637faa7f13457109d45f5addf
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Dec 03 17:04:09 2018

Add sync integration tests related to history sync

FakeServer/LoopbackServer is extended with logic that mimics what real
servers do, in order to verify what contributes to synced history.

This functionality is leveraged to add proper integration test coverage
for various relevant scenarios to history sync, starting with the
simplest, to more advanced including closed tabs.

This approach is expected to fix some flakes we've observed in recent
attempts to land tests, because it doesn't suffer from server-side
session entities potentially being overwritten by newly open tabs.

Bug: 882489, 910947 
Change-Id: I4a69fa080e3deacc8d33c64de17f964c34436ea3
Reviewed-on: https://chromium-review.googlesource.com/c/1356811
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613127}
[modify] https://crrev.com/ed1c99d3f024717637faa7f13457109d45f5addf/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
[modify] https://crrev.com/ed1c99d3f024717637faa7f13457109d45f5addf/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
[modify] https://crrev.com/ed1c99d3f024717637faa7f13457109d45f5addf/components/sync/engine_impl/loopback_server/loopback_server.cc
[modify] https://crrev.com/ed1c99d3f024717637faa7f13457109d45f5addf/components/sync/engine_impl/loopback_server/loopback_server.h
[modify] https://crrev.com/ed1c99d3f024717637faa7f13457109d45f5addf/components/sync/test/fake_server/fake_server.cc
[modify] https://crrev.com/ed1c99d3f024717637faa7f13457109d45f5addf/components/sync/test/fake_server/fake_server.h

Status: Started (was: Assigned)
Status: Fixed (was: Started)

Sign in to add a comment