Issue 912621  has been merged into this issue.

 Issue 912565  has been merged into this issue.

 Issue 912538  has been merged into this issue.

 Issue 912473  has been merged into this issue.

 Issue 912458  has been merged into this issue.

 Issue 912454  has been merged into this issue.

Mustash sheriff here.

Confirmed reverting r614199 fixes this. Can we revert it?

jyasskin@ is reverting. Do you repro locally or through a bot, just so I have repro steps for fixing a reland.

Reverted in https://chromium-review.googlesource.com/c/chromium/src/+/1366359.

I reproduced by building browser_tests with the following GN args (on linux):

use_goma = true
dcheck_always_on = true
is_asan = true
is_component_build = false
is_debug = false
is_lsan = true
strip_absolute_paths_from_debug_symbols = true
symbol_level = 1
target_os = "chromeos"


Then run browser_tests binary with --enable-features=SingleProcessMash flag.

So, changing TabIcon::ShowingLoadingAnimation to return false, e.g. not play animations, makes this test flaky. The above revert makes it return false earlier.

I don't know how this test implicitly ends up depending on the tab icon animation. horo@ can you help me take a look here? Can we wait for the favicon as well before starting the test or something like that? Could test changes in r608189 be related to the favicon.ico entry showing up here? UI updates should probably be orthogonal to this test in general.

The following patch makes the test fail, flakily:

diff --git a/chrome/browser/ui/views/tabs/tab_icon.cc b/chrome/browser/ui/views/tabs/tab_icon.cc
index d82e8a332ee7..58f24657be5a 100644
--- a/chrome/browser/ui/views/tabs/tab_icon.cc
+++ b/chrome/browser/ui/views/tabs/tab_icon.cc
@@ -145,6 +145,7 @@ void TabIcon::SetAttention(AttentionType type, bool enabled) {
 }
 
 bool TabIcon::ShowingLoadingAnimation() const {
+  return false;
   if (inhibit_loading_animation_)
     return false;


Failing output is:

../../chrome/browser/chrome_service_worker_browsertest.cc:491: Failure
Expected equality of these values:
  RequestString(url, "cors", "include")
    Which is: "url:http://www.example.com:40079/manifest.json, mode:cors, credentials:include\n"
  ExecuteManifestFetchTest(url, "use-credentials")
    Which is: "url:http://127.0.0.1:40079/favicon.ico, mode:no-cors, credentials:include\nurl:http://www.example.com:40079/manifest.json, mode:cors, credentials:include\n"
With diff:
@@ +1,2 @@
+url:http://127.0.0.1:40079/favicon.ico, mode:no-cors, credentials:include
 url:http://www.example.com:40079/manifest.json, mode:cors, credentials:include\n

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

commit 49dd35bd0b884b3bdfd0f86867bb4a6114f77af3
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Tue Dec 11 09:30:06 2018

Ignores the default favicon request in ChromeServiceWorkerFetchTest

Currently if the default favicon request is caught by the testing service worker
the test fails.
This is causing the test flakiness.
To fix this flakiness, this CL change the fetch event handler of the service
worker to ignore the default favicon request.

Bug:  912543 
Change-Id: Icdddd902a4547361320f43db5b9090270ba74509
Reviewed-on: https://chromium-review.googlesource.com/c/1370160
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615470}
[modify] https://crrev.com/49dd35bd0b884b3bdfd0f86867bb4a6114f77af3/chrome/browser/chrome_service_worker_browsertest.cc

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

commit ad12d44124812f22484eb4d123e4a3b4aaac93b9
Author: Peter Boström <pbos@chromium.org>
Date: Tue Dec 11 18:02:22 2018

Reland "Disable the new tab-loading animation"

This reverts commit 742b8857c4dd0ab4550f7b2c1f200ce5be406b9c.

Reason for revert: Flaky tests should be fixed in r615470.

Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 

Original change's description:
> Revert "Disable the new tab-loading animation"
> 
> This reverts commit 355b8185716292a8f145c77168ee89148ecabf31.
> 
> Reason for revert: Made several tests flaky:  https://crbug.com/912543 
> 
> Original change's description:
> > Disable the new tab-loading animation
> > 
> > Makes sure that a lot of animation-related code is bypassed when the
> > new-tab-animation flag is off. This should hopefully fix a couple of
> > performance regressions that have not yet been root caused so that they
> > don't go out with M72.
> > 
> > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > Change-Id: Id3f131db427eb3ee1618d6c9683fd5e47dc134e8
> > Reviewed-on: https://chromium-review.googlesource.com/c/1364212
> > Reviewed-by: Sidney San Martín <sdy@chromium.org>
> > Commit-Queue: Peter Boström <pbos@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#614199}
> 
> TBR=pbos@chromium.org,sdy@chromium.org
> 
> Change-Id: Ib4c022a255ad085c1716d3559a7f84dcb61c2785
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> Reviewed-on: https://chromium-review.googlesource.com/c/1366359
> Reviewed-by: Jeffrey Yasskin <jyasskin@chromium.org>
> Commit-Queue: Jeffrey Yasskin <jyasskin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#614440}

TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
Change-Id: I3981455a00f743fae535de4626179dcceab9b2c4
Reviewed-on: https://chromium-review.googlesource.com/c/1372236
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615581}
[modify] https://crrev.com/ad12d44124812f22484eb4d123e4a3b4aaac93b9/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/ad12d44124812f22484eb4d123e4a3b4aaac93b9/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/ad12d44124812f22484eb4d123e4a3b4aaac93b9/chrome/common/chrome_features.cc

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

commit 6921bd825e416c7b412b454f60df8b183e906943
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Wed Dec 12 14:10:33 2018

Revert "Reland "Disable the new tab-loading animation""

This reverts commit ad12d44124812f22484eb4d123e4a3b4aaac93b9.

Reason for revert:
Findit found this to be the most likely culprit for the single_process_mash_browser_tests failures on various Linux bots:
https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/30518


Original change's description:
> Reland "Disable the new tab-loading animation"
>
> This reverts commit 742b8857c4dd0ab4550f7b2c1f200ce5be406b9c.
>
> Reason for revert: Flaky tests should be fixed in r615470.
>
> Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 
>
> Original change's description:
> > Revert "Disable the new tab-loading animation"
> >
> > This reverts commit 355b8185716292a8f145c77168ee89148ecabf31.
> >
> > Reason for revert: Made several tests flaky:  https://crbug.com/912543 
> >
> > Original change's description:
> > > Disable the new tab-loading animation
> > >
> > > Makes sure that a lot of animation-related code is bypassed when the
> > > new-tab-animation flag is off. This should hopefully fix a couple of
> > > performance regressions that have not yet been root caused so that they
> > > don't go out with M72.
> > >
> > > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > > Change-Id: Id3f131db427eb3ee1618d6c9683fd5e47dc134e8
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1364212
> > > Reviewed-by: Sidney San Martín <sdy@chromium.org>
> > > Commit-Queue: Peter Boström <pbos@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#614199}
> >
> > TBR=pbos@chromium.org,sdy@chromium.org
> >
> > Change-Id: Ib4c022a255ad085c1716d3559a7f84dcb61c2785
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > Reviewed-on: https://chromium-review.googlesource.com/c/1366359
> > Reviewed-by: Jeffrey Yasskin <jyasskin@chromium.org>
> > Commit-Queue: Jeffrey Yasskin <jyasskin@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#614440}
>
> TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> Change-Id: I3981455a00f743fae535de4626179dcceab9b2c4
> Reviewed-on: https://chromium-review.googlesource.com/c/1372236
> Reviewed-by: Peter Boström <pbos@chromium.org>
> Commit-Queue: Peter Boström <pbos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#615581}

TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org

Change-Id: Ib9599bb8fd44327ae756d3c9523049367409612c
Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 ,  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
Reviewed-on: https://chromium-review.googlesource.com/c/1373832
Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615878}
[modify] https://crrev.com/6921bd825e416c7b412b454f60df8b183e906943/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/6921bd825e416c7b412b454f60df8b183e906943/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/6921bd825e416c7b412b454f60df8b183e906943/chrome/common/chrome_features.cc

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

commit 831dad6e5036b31b683587078db3df8e32ce4f92
Author: Peter Boström <pbos@chromium.org>
Date: Thu Dec 13 16:50:04 2018

Reland "Reland "Disable the new tab-loading animation""

This reverts commit 6921bd825e416c7b412b454f60df8b183e906943.

Reason for revert: NOTE TO SHERIFFS: Both the revert and reland of this
CL is the culprit for FindIt flakes. Context:

This CL changes the duration of the tab-icon animation ("spinner"). This
likely has some effects on test loops that RunUntilIdle or similar, so a
pending request might either have happened or not before that loop gets
idle.

For instance:  crbug.com/912543  had to be changed to ignore a favicon.ico
response. This flakiness could be reproduced by just changing
TabIcon::ShowingLoadingAnimation to "return false;" even though the test
does not have anything to do with Chrome's UI directly.

This reland will likely trigger crbug.com/914232 again, but the revert
triggered  crbug.com/914644 . It's not unlikely that they are both flaky
as they implicitly rely on tab-icon load time.

Bug: chromium:914232,  chromium:914644 

Original change's description:
> Revert "Reland "Disable the new tab-loading animation""
> 
> This reverts commit ad12d44124812f22484eb4d123e4a3b4aaac93b9.
> 
> Reason for revert:
> Findit found this to be the most likely culprit for the single_process_mash_browser_tests failures on various Linux bots:
> https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/30518
> 
> 
> Original change's description:
> > Reland "Disable the new tab-loading animation"
> >
> > This reverts commit 742b8857c4dd0ab4550f7b2c1f200ce5be406b9c.
> >
> > Reason for revert: Flaky tests should be fixed in r615470.
> >
> > Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 
> >
> > Original change's description:
> > > Revert "Disable the new tab-loading animation"
> > >
> > > This reverts commit 355b8185716292a8f145c77168ee89148ecabf31.
> > >
> > > Reason for revert: Made several tests flaky:  https://crbug.com/912543 
> > >
> > > Original change's description:
> > > > Disable the new tab-loading animation
> > > >
> > > > Makes sure that a lot of animation-related code is bypassed when the
> > > > new-tab-animation flag is off. This should hopefully fix a couple of
> > > > performance regressions that have not yet been root caused so that they
> > > > don't go out with M72.
> > > >
> > > > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > > > Change-Id: Id3f131db427eb3ee1618d6c9683fd5e47dc134e8
> > > > Reviewed-on: https://chromium-review.googlesource.com/c/1364212
> > > > Reviewed-by: Sidney San Martín <sdy@chromium.org>
> > > > Commit-Queue: Peter Boström <pbos@chromium.org>
> > > > Cr-Commit-Position: refs/heads/master@{#614199}
> > >
> > > TBR=pbos@chromium.org,sdy@chromium.org
> > >
> > > Change-Id: Ib4c022a255ad085c1716d3559a7f84dcb61c2785
> > > No-Presubmit: true
> > > No-Tree-Checks: true
> > > No-Try: true
> > > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1366359
> > > Reviewed-by: Jeffrey Yasskin <jyasskin@chromium.org>
> > > Commit-Queue: Jeffrey Yasskin <jyasskin@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#614440}
> >
> > TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org
> >
> > # Not skipping CQ checks because original CL landed > 1 day ago.
> >
> > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > Change-Id: I3981455a00f743fae535de4626179dcceab9b2c4
> > Reviewed-on: https://chromium-review.googlesource.com/c/1372236
> > Reviewed-by: Peter Boström <pbos@chromium.org>
> > Commit-Queue: Peter Boström <pbos@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#615581}
> 
> TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org
> 
> Change-Id: Ib9599bb8fd44327ae756d3c9523049367409612c
> Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 ,  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> Reviewed-on: https://chromium-review.googlesource.com/c/1373832
> Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
> Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#615878}

TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org,fhorschig@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 ,  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
Change-Id: Ib8083b1320f3e1cd5c878bd55d2a007af7bf719a
Reviewed-on: https://chromium-review.googlesource.com/c/1376030
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616334}
[modify] https://crrev.com/831dad6e5036b31b683587078db3df8e32ce4f92/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/831dad6e5036b31b683587078db3df8e32ce4f92/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/831dad6e5036b31b683587078db3df8e32ce4f92/chrome/common/chrome_features.cc