New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 842325 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocking:
issue 760181



Sign in to add a comment

BrowserSideFlingBrowserTest Failing on Viz Mac

Project Member Reported by jonr...@chromium.org, May 11 2018

Issue description

OS: Mac
Test suite: viz_content_browsertests
Tests:
  BrowserSideFlingBrowserTest.TouchpadFling
  BrowserSideFlingBrowserTest.TouchscreenFling
  BrowserSideFlingBrowserTest.AutoscrollFling

Example failing build: https://ci.chromium.org/buildbot/chromium.fyi/Chromium%20Mac%2010.13/2797

The tests are all currently timing out while waiting for RenderFrameMetadata.

However each test has while loops waiting for metadata to arrive with an expected scroll offset. So it's likely that we are receiving data, but that its not at the desired scroll offset. Then eventually frames stop being submitted.

I'm assigning to ccameron@ whom has been working on Viz support for Mac.

I'm CCing sahel@ whom added the test and may be able to comment further about this.
 

Comment 1 by sahel@chromium.org, May 11 2018

I think this information might be useful:

The tests timeout since the expected scrollers don't scroll and that's because FlingController::ProgressFling never gets called.

FlingController::ProgressFling is responsible for generating GSU events and sending them to the renderer while a fling is active and FlingScheduler is responsible for making sure that FlingController::ProgressFling gets called on every begin frame.

FlingScheduler does the job in either of the two ways:
If ui::Compositor is available, it adds the FlingController to it as an animation observer, otherwise it Calls SetNeedsBeginFrame to schedule a beginFrame.

On mac the first option doesn't work (please check  crbug.com/833985 ) and the second option doesn't work when viz is enabled.
Cc: sadrul@chromium.org ccameron@chromium.org
Owner: sahel@chromium.org
Thanks for that info sahel@,

sadrul@ mentioned that you may have work inflight which could address the viz support?

Comment 3 by sahel@chromium.org, May 16 2018

https://chromium-review.googlesource.com/c/chromium/src/+/994181 is the cl that addresses viz support and it is already landed.

Please see the todo comment here:
https://chromium-review.googlesource.com/c/chromium/src/+/994181/22/content/browser/renderer_host/input/fling_scheduler_mac.mm

when FlingSchedulerMac::GetCompositor() returns null then fling progress happens by calling setNeedsBeginFrame which is not allowed with viz enabled.

On aura when FlingScheduler::GetCompositor() returns the proper compositor the tests pass with viz enabled.

Comment 4 by sahel@chromium.org, May 16 2018

I think the fix here is to make sure that FlingSchedulerMac::GetCompositor() works on mac. sadrul@ or ccameron@ please correct me if I am wrong.

Comment 5 by sahel@chromium.org, May 16 2018

https://docs.google.com/presentation/d/1miFAvKuz7tRT4IX_nCwZshFkUDX8bDYqfPlTOm2LS8U/edit?usp=sharing contains a class diagram for how fling controller and fling scheduler works.
Owner: ccameron@chromium.org
Status: Assigned (was: Available)
Also:
SitePerProcessBrowserTest.TouchpadGestureFlingStart
SitePerProcessBrowserTest.TouchscreenGestureFlingStart
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 13 2018

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

commit a83e4da7b488f41b30c5d08491cad4723bf14bb0
Author: jonross <jonross@chromium.org>
Date: Wed Jun 13 17:41:07 2018

Disable failing sling tests on Viz Mac

Two new fling tests were added to SitePerProcessBrowserTest, these do not
currently work on Mac with Viz.

This filters those tests out.

TBR=kylechar@chromium.org
TEST=SitePerProcessBrowserTest.TouchpadGestureFlingStart,
SitePerProcessBrowserTest.TouchscreenGestureFlingStart

Bug:  842325 
Change-Id: Ia45eaf8f5cfb2f8865a9f1d5e3aa4d677e37c1cd
Reviewed-on: https://chromium-review.googlesource.com/1099372
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566890}
[modify] https://crrev.com/a83e4da7b488f41b30c5d08491cad4723bf14bb0/testing/buildbot/filters/viz.content_browsertests.filter

Reproduced today ahead of triage
Labels: -Pri-3 Pri-1
Hey Chris,

Would you be able to take a look at this? We'd like to clear up the known Mac issues before finch trials for Viz
Sure, I'll try to get this soon.
Hey Chris, I was wondering if you'd had the chance to take a look at this? It's the main Mac-only failure in Viz tests at the moment.
I'm 110% on MacViews ATM, but I will find time eventually.
Fixing  issue 833985  (uncommenting the "can't use ui::Compositor yet" part) seems to fix this.
Owner: jonr...@chromium.org
Status: Started (was: Assigned)
Nice!!

Based on that I've put together a patch which enables it: https://chromium-review.googlesource.com/c/chromium/src/+/1150210
Owner: ccameron@chromium.org
Nevermind, ccameron@ is already working on landing a patch for this: https://chromium-review.googlesource.com/c/chromium/src/+/1149612
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 26

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

commit 4c440b7c6b5edb7c01d41d498bc00f686c2251fa
Author: Yoichi Osato <yoichio@chromium.org>
Date: Thu Jul 26 01:55:25 2018

Revert "Enable FlingSchedulerMac"

This reverts commit d6ec81545fea9d795fe5655d8453c343bc61420a.

Reason for revert: Findit identified the culprit r577957 with confidence 100.0% in the config "chromium.mac / Mac10.11 Tests"

Original change's description:
> Enable FlingSchedulerMac
> 
> Enable tests that were disabled because of this functionality missing.
> 
> Bug:  842325 
> Change-Id: I6f540460f4e5f81d7ff8dd36e4abf369f0bd4785
> Reviewed-on: https://chromium-review.googlesource.com/1149612
> Reviewed-by: Jonathan Ross <jonross@chromium.org>
> Commit-Queue: ccameron <ccameron@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577957}

TBR=ccameron@chromium.org,jonross@chromium.org

Change-Id: I4a38151ba5333ca99fb28c00f57f93f057f5fd4b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  842325 , 867668
Reviewed-on: https://chromium-review.googlesource.com/1150946
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578166}
[modify] https://crrev.com/4c440b7c6b5edb7c01d41d498bc00f686c2251fa/content/browser/renderer_host/browser_compositor_view_mac.h
[modify] https://crrev.com/4c440b7c6b5edb7c01d41d498bc00f686c2251fa/content/browser/renderer_host/browser_compositor_view_mac.mm
[modify] https://crrev.com/4c440b7c6b5edb7c01d41d498bc00f686c2251fa/content/browser/renderer_host/input/fling_scheduler_mac.mm
[modify] https://crrev.com/4c440b7c6b5edb7c01d41d498bc00f686c2251fa/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/4c440b7c6b5edb7c01d41d498bc00f686c2251fa/testing/buildbot/filters/viz.content_browsertests.filter

Project Member

Comment 19 by bugdroid1@chromium.org, Jul 26

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

commit 3fff2b9f19ac86aec16ed5c0179cfe3cfe9dabc7
Author: Jonathan Ross <jonross@chromium.org>
Date: Thu Jul 26 17:57:19 2018

Early Exit Failing Mac Fling Tests on Viz

There are two remaining Mac Fling tests which fail under Viz. Work is
ongoing to land a fix, but there was flakiness if the resolution.

This change updates the failing tests to exit early when it is Viz on
Mac. This will unblock removing our filter files.

TEST=BrowserSideFlingBrowserTest.TouchpadFling
BrowserSideFlingBrowserTest.TouchscreenFling

Bug:  842325 
Change-Id: I67e7c6165d821c9df4907ebf8803fe22fbdf1663
Reviewed-on: https://chromium-review.googlesource.com/1151392
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578362}
[modify] https://crrev.com/3fff2b9f19ac86aec16ed5c0179cfe3cfe9dabc7/content/browser/renderer_host/input/fling_browsertest.cc

Actually, the flake was some layout test ... see issue 867668. It starts passing 93% of the time on some ancient macOS version (grumble).
So the test in question is middleClickAutoscroll-click-hyperlink.html

Looking into it further, this test was first marked with an expectation of failure by sheriffs on June 8, way before your patch. See issue 851090

It was also later one tracked by  issue 867628  as well.

So it looks like its a flaky test in general, and your patch may have helped reveal it further.

Thoughts on relanding your patch, and coordinating with sahel@ about the flakiness of the test?
There are a few middleClickAutoscroll related tests that are flaky on Mac. I think it is ok to skip the test on Mac and land your patch. Me or 	sunyunjia@ will work on the test to make it less flaky and re-enable it.
Project Member

Comment 23 by bugdroid1@chromium.org, Aug 7

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

commit 5b164dcf2b045ee787e69a20322a4a2808fbe9c0
Author: jonross <jonross@chromium.org>
Date: Tue Aug 07 19:47:31 2018

Reland FlingSchedulerMac for Viz

This relands the following change:

Enable FlingSchedulerMac

Enable tests that were disabled because of this functionality missing.

Bug:  842325 
Change-Id: I6f540460f4e5f81d7ff8dd36e4abf369f0bd4785
Reviewed-on: https://chromium-review.googlesource.com/1149612
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577957}

This was identified as a cause of flakiness in Mac webkit_layout_tests. But that
test's flakiness also pre-dated this patch. The test in question has also been
marked to expect fails on Mac. The source of that flakiness will be investigated
separately.

Change-Id: I22364cb8e18507b528c6c0c32f452fcf208caf7e
Reviewed-on: https://chromium-review.googlesource.com/1162716
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581309}
[modify] https://crrev.com/5b164dcf2b045ee787e69a20322a4a2808fbe9c0/content/browser/renderer_host/browser_compositor_view_mac.h
[modify] https://crrev.com/5b164dcf2b045ee787e69a20322a4a2808fbe9c0/content/browser/renderer_host/browser_compositor_view_mac.mm
[modify] https://crrev.com/5b164dcf2b045ee787e69a20322a4a2808fbe9c0/content/browser/renderer_host/input/fling_browsertest.cc
[modify] https://crrev.com/5b164dcf2b045ee787e69a20322a4a2808fbe9c0/content/browser/renderer_host/input/fling_scheduler_mac.mm
[modify] https://crrev.com/5b164dcf2b045ee787e69a20322a4a2808fbe9c0/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/5b164dcf2b045ee787e69a20322a4a2808fbe9c0/third_party/WebKit/LayoutTests/TestExpectations

Status: Fixed (was: Started)
Fling is now re-enabled for Viz. With the flaky test expectations updated and issue 851090 tracking their re-work.

Sign in to add a comment