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

Issue 900948 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression

Blocked on:
issue 915915



Sign in to add a comment

Crash inside cc::LayerTreeHost::SetViewportSizeAndScale making context_lost_tests flaky on mac

Project Member Reported by riajiang@chromium.org, Nov 1

Issue description

* gpu_tests.context_lost_integration_test.ContextLostIntegrationTest.ContextLost_WebGLUnblockedAfterUserInitiatedReload

becomes flaky on Mac Retina Release (AMD) after https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/43907

(list https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29?limit=1000)


  Traceback (most recent call last):
    _RunGpuTest at content/test/gpu/gpu_tests/gpu_integration_test.py:138
      self.RunActualGpuTest(url, *args)
    RunActualGpuTest at content/test/gpu/gpu_tests/context_lost_integration_test.py:106
      getattr(self, test_name)(test_path)
    _ContextLost_WebGLUnblockedAfterUserInitiatedReload at content/test/gpu/gpu_tests/context_lost_integration_test.py:326
      'window.domAutomationController._succeeded'):
    traced_function at third_party/catapult/common/py_trace_event/py_trace_event/trace_event_impl/decorators.py:52
      return func(*args, **kwargs)
    EvaluateJavaScript at third_party/catapult/telemetry/telemetry/internal/browser/web_contents.py:221
      return self._inspector_backend.EvaluateJavaScript(*args, **kwargs)
    traced_function at third_party/catapult/common/py_trace_event/py_trace_event/trace_event_impl/decorators.py:52
      return func(*args, **kwargs)
    EvaluateJavaScript at third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:263
      user_gesture=user_gesture)
    traced_function at third_party/catapult/common/py_trace_event/py_trace_event/trace_event_impl/decorators.py:52
      return func(*args, **kwargs)
    Inner at third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:38
      return func(inspector_backend, *args, **kwargs)
    _EvaluateJavaScript at third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:547
      user_gesture)
    Evaluate at third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_runtime.py:45
      res = self._inspector_websocket.SyncRequest(request, timeout)
    SyncRequest at third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_websocket.py:132
      res = self._Receive(timeout)
    _Receive at third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_websocket.py:190
      self._HandleNotification(result)
    _HandleNotification at third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_websocket.py:203
      self._domain_handlers[domain_name](result)
    traced_function at third_party/catapult/common/py_trace_event/py_trace_event/trace_event_impl/decorators.py:52
      return func(*args, **kwargs)
    _HandleInspectorDomainNotification at third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:484
      raise exception
  DevtoolsTargetCrashException: Devtools target crashed
  ********************************************************************************
  (/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:539 _AddDebuggingInformation) Received a socket error in the browser connection and the tab no longer exists. The tab probably crashed.


* gpu_tests.context_lost_integration_test.ContextLostIntegrationTest.GpuCrash_GPUProcessCrashesExactlyOncePerVisitToAboutGpuCrash

failed once on Mac Debug (Intel) https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Debug%20%28Intel%29/87181

(list https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Debug%20%28Intel%29?limit=1000)



* gpu_tests.context_lost_integration_test.ContextLostIntegrationTest.ContextLost_WebGLBlockedAfterJSNavigation

failed on Mac Release (Intel) https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Release%20%28Intel%29/101992

(list https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Release%20%28Intel%29?limit=1000)



suspecting https://chromium-review.googlesource.com/c/chromium/src/+/1298298, ericorth@, could you please take a look to see if they are related? thanks!
 
Owner: riajiang@chromium.org
Took a brief look at the log output.  Didn't see anything obviously related (didn't see any interaction with network sockets at the crash, especially not multicast sockets).
Cc: danakj@chromium.org fsam...@chromium.org riajiang@chromium.org enne@chromium.org
Components: Internals>Services>Viz Internals>Compositing
Labels: -Type-Bug -Pri-3 Pri-2 Type-Bug-Regression
Owner: ----
Status: Available (was: Untriaged)
riajiang@ please dig a little further into the logs. A little further down there is this clear crash inside cc::LayerTreeHost::SetViewportSizeAndScale which is the reason the test harness reported that the DevTools target crashed.

Looking at the bot's recent logs, this isn't extremely flaky - ~4 flakes in 200 builds here:

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29?limit=200

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/44416
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/44404
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/44361
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/44342

They're all the same. riajiang@ this means that the regression range could be possibly huge (especially since the one you found is even before these) so the first failing build is definitely not the one that introduced the problem.

enne@ danakj@ fsamuel@ I'm not sure which component this belongs in (compositing and/or viz) but who would be an appropriate owner for tracking down the possible root cause? Thanks in advance for your help.

Upgrading to P2 - I don't think the flakiness level warrants P1 but this is important to track down and fix. If it won't be fixed unless it's P1 then I'll gladly upgrade it.


	Operating system: Mac OS X
	                  10.13.6 17G65
	CPU: amd64
	     family 6 model 70 stepping 1
	     8 CPUs
	
	GPU: UNKNOWN
	
	Crash reason:  EXC_BREAKPOINT / EXC_I386_BPT
	Crash address: 0x1213419c1
	Process uptime: 4 seconds
	
	Thread 0 (crashed)
	 0  Chromium Framework!__ZN4base5debug13BreakDebuggerEv + 0x11
	    rax = 0x00007fd32b85183d   rdx = 0x00007fd32b85183d
	    rcx = 0x0000000000000087   rbx = 0x0000000000000087
	    rsi = 0x0000000000000489   rdi = 0x0000000129454814
	    rbp = 0x00007ffee269ef00   rsp = 0x00007ffee269ef00
	     r8 = 0x00007fd32b8518c4    r9 = 0x00000000000015de
	    r10 = 0x00007fd32b8518c4   r11 = 0x0000000121388bd0
	    r12 = 0x00007fd329a231a0   r13 = 0x00007fd32b8518c4
	    r14 = 0x00007ffee269f820   r15 = 0x00007ffee269f818
	    rip = 0x00000001213419c1
	    Found by: given as instruction pointer in context
	 1  Chromium Framework!__ZN7logging10LogMessageD2Ev + 0x900
	    rbp = 0x00007ffee269f800   rsp = 0x00007ffee269ef10
	    rip = 0x0000000121244e90
	    Found by: previous frame's frame pointer
	 2  Chromium Framework!__ZN2cc13LayerTreeHost23SetViewportSizeAndScaleERKN3gfx4SizeEfRKN3viz14LocalSurfaceIdEN4base9TimeTicksE + 0x21e
	    rbp = 0x00007ffee269f960   rsp = 0x00007ffee269f810
	    rip = 0x0000000122d919be
	    Found by: previous frame's frame pointer
	 3  Chromium Framework!__ZN7content12RenderWidget26UpdateSurfaceAndScreenInfoERKN3viz14LocalSurfaceIdEN4base9TimeTicksERKN3gfx4SizeERKNS_10ScreenInfoE + 0x168
	    rbp = 0x00007ffee269fbc0   rsp = 0x00007ffee269f970
	    rip = 0x00000001275ee978
	    Found by: previous frame's frame pointer
	 4  Chromium Framework!__ZN7content12RenderWidget27SynchronizeVisualPropertiesERKNS_16VisualPropertiesE + 0x109
	    rbp = 0x00007ffee269fc70   rsp = 0x00007ffee269fbd0
	    rip = 0x00000001275f7ab9
	    Found by: previous frame's frame pointer
	 5  Chromium Framework!__ZN7content12RenderWidget29OnSynchronizeVisualPropertiesERKNS_16VisualPropertiesE + 0x206
	    rbp = 0x00007ffee269ff40   rsp = 0x00007ffee269fc80
	    rip = 0x00000001275f43a6
	    Found by: previous frame's frame pointer
	 6  Chromium Framework!__ZN3IPC8MessageTI42WidgetMsg_SynchronizeVisualProperties_MetaNSt3__15tupleIJN7content16VisualPropertiesEEEEvE8DispatchINS4_12RenderWidgetES9_vMS9_FvRKS5_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ + 0x7d
	    rbp = 0x00007ffee26a00a0   rsp = 0x00007ffee269ff50
	    rip = 0x00000001275ef6cd
	    Found by: previous frame's frame pointer
	 7  Chromium Framework!__ZN7content12RenderWidget17OnMessageReceivedERKN3IPC7MessageE + 0x1ca
	    rbp = 0x00007ffee26a0200   rsp = 0x00007ffee26a00b0
	    rip = 0x00000001275eef3a
	    Found by: previous frame's frame pointer
	 8  Chromium Framework!__ZN7content14RenderViewImpl17OnMessageReceivedERKN3IPC7MessageE + 0x5b5
	    rbp = 0x00007ffee26a0490   rsp = 0x00007ffee26a0210
	    rip = 0x00000001275e6185
	    Found by: previous frame's frame pointer



Summary: Crash inside cc::LayerTreeHost::SetViewportSizeAndScale making context_lost_tests flaky on mac (was: context_lost_tests are flaky on mac)
Cc: ccameron@chromium.org
Would be helpful to know what DCHECK or CHECK is failing. There is a mac-specific CHECK inside that method about parent surface ids.#if defined(OS_MACOSX)
    // TODO(ccameron): This check is not valid on Aura or Mus yet, but should
    // be.
    CHECK(!has_pushed_local_surface_id_from_parent_ ||
          new_local_surface_id_request_ ||
          !local_surface_id_from_parent_.is_valid());
#endif

Agreed it would be helpful to find the actual assertion failure. I tried to find it in the logs but didn't explicitly do searches which could have turned it up. Would hope that the pixel wrangler could do that analysis.

Since there's only that one CHECK in the file, it definitely looks as though that's the one that is hit. I think fsamuel tried to remote the Mac-only guard, but it was getting hit in the wild.
Cc: odejesush@chromium.org weiliangc@chromium.org
 Issue 911283  has been merged into this issue.
Cc: -fsam...@chromium.org sadrul@chromium.org samans@chromium.org
Labels: -Pri-2 M-73 Pri-1
Owner: sadrul@chromium.org
Status: Assigned (was: Available)
Per  Issue 911283 , this crash is causing significant flakiness on this key test bot:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29?limit=200

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47206
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47201
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47184
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47175
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47161
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47150
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47148
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47147
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47146
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47145

We have to get these tests back to a reliable state. While this is a corner case that probably only happens during context loss, it has to fail gracefully.

sadrul@ could I please ask you to take this and either reassign or drive this to a fix? Thank you.

Owner: jonr...@chromium.org
--> jonross@ who recently worked on LSI allocations. Mind looking into this?


Logs in #8 confirm that the suspected CHECK in #4 is the cause.

However I cannot reproduce this locally. So I have a patch up to add extra debug info to this CHECK. I'd like to know why it's failing. Then I can put together an appropriate graceful exit.

https://chromium-review.googlesource.com/c/chromium/src/+/1363952
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 5

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

commit 3f967741d22a6d280288db2cfbfd3c3d16d77feb
Author: Jonathan Ross <jonross@chromium.org>
Date: Wed Dec 05 22:52:02 2018

Add extra debug info to LayerTreeHost::SetViewportSizeAndScale CHECK

LayerTreeHost::SetViewportSizeAndScale is hitting a CHECK on Mac during
context_lost_tests.

However it is quite flaky and I cannot reproduce locally.

I'm adding debug output to the CHECK statement so I can find out what
exactly is the error. After which we can determine an appropriate fix
or graceful shutdown.

TEST=gpu_tests.context_lost_integration_test.ContextLostIntegrationTest.ContextLost_WebGLUnblockedAfterUserInitiatedReload

Bug: 900948
Change-Id: I492992e94790e1dc9d358a69aae164f62c676f55
Reviewed-on: https://chromium-review.googlesource.com/c/1363952
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614148}
[modify] https://crrev.com/3f967741d22a6d280288db2cfbfd3c3d16d77feb/cc/trees/layer_tree_host.cc

We have a failure after the updated logging was added:

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20Retina%20Release%20%28AMD%29/47405

The result:
Invalid Surface Id State: !has_pushed 0 new_id_request 0 !valid_parent_id 0

Local runs which aren't failing are showing: 
                          !has_pushed 1 new_id_request 0 !valid_parent_id 0

So it appears that we still have the flag set for a pushed id.

Looking at LayerTreeHost::SetLocalSurfaceIdAllocationFromParent there's an early out if the LocalSurfaceIdd being set matches the cached one.
So we are being notified to Sync Visual Properties, a property has changed, but the LocalSurfaceId hasn't. Which is definitely a violation.

RenderWidgetHostImpl::GetVisualProperties, which preps the update to send to RenderWidget has a note:
    // TODO(ccameron): GetLocalSurfaceId is not synchronized with the device
    // scale factor of the surface. Fix this.

Which I suspect could be the case. I'll put together a patch to verify this within the test. And will look into the browser-side handling of device scale factor to see about potential fixes.
Proposed next follow up debug messaging:  https://chromium-review.googlesource.com/c/chromium/src/+/1366296/

A local run shows that the test does in fact trigger a device_scale_factor change. However it's arriving with a device_viewport_size change. Which brings a new LocalSurfaceId.

I'd like to confirm that the test has just the scale factor change arriving alone. Which would confirm the suspicions in #12
Keep in mind that context loss typically causes a whole bunch of invariants to be violated. It would be fine if we could reliably detect that if this invariant violation occurs, context loss has also happened - which may itself be a little problematic since context loss deliveries aren't guaranteed to be prompt.

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 10

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

commit 6f3bfd0ab9e5e120d5c3e79c8946d54893a5c674
Author: Jonathan Ross <jonross@chromium.org>
Date: Mon Dec 10 15:27:07 2018

Even more debug info to LayerTreeHost::SetViewportSizeAndScale CHECK

This is a follow up to: https://chromium-review.googlesource.com/c/chromium/src/+/1363952

We narrowed down the failure. However we need to confirm a second aspect
of the crash. This adds a bit more debug info to the CHECK statement.

Bug: 900948
Change-Id: I4752e5adb7cebbb0f0e0d9d7f3e9b4638cd45f81
Reviewed-on: https://chromium-review.googlesource.com/c/1366296
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615124}
[modify] https://crrev.com/6f3bfd0ab9e5e120d5c3e79c8946d54893a5c674/cc/trees/layer_tree_host.cc

This is causing a significant amount of flakiness on the commit queue. Just a few recent examples:

https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng/205536
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng/205456
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng/205449
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng/205445

See more generally:
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng?limit=200

I'm going to mark these tests flaky but resolving the root cause here is urgent - it's affecting other Chromium developers' productivity. Please prioritize fixing this.

Suppressing ContextLost_WebGLBlockedAfterJSNavigation and ContextLost_WebGLUnblockedAfterUserInitiatedReload on macOS in https://chromium-review.googlesource.com/1372350 .

Project Member

Comment 19 by bugdroid1@chromium.org, Dec 11

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

commit a1484b14c06d83361c9c209b616ce583e4e7a01a
Author: Kenneth Russell <kbr@chromium.org>
Date: Tue Dec 11 23:46:04 2018

Suppress two WebGL-related context lost tests on macOS.

  ContextLost_WebGLBlockedAfterJSNavigation
  ContextLost_WebGLUnblockedAfterUserInitiatedReload

Both are flaking due to the same root cause.

Tbr: jonross@chromium.org
Bug: 900948
No-Try: True
Change-Id: Ic527ce1914bacd539222c7a8e8c9265be404aff2
Reviewed-on: https://chromium-review.googlesource.com/c/1372350
Commit-Queue: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615727}
[modify] https://crrev.com/a1484b14c06d83361c9c209b616ce583e4e7a01a/content/test/gpu/gpu_tests/context_lost_expectations.py

jonross@: please remove the suppressions added above once the underlying bug is fixed. Thanks!

So the updated debug info invalidated my original suppositions:

Check failed: !has_pushed_local_surface_id_from_parent_ || new_local_surface_id_request_ || !local_surface_id_allocation_from_parent_.IsValid(). Invalid Surface Id State: !has_pushed 0 new_id_request 0 !valid_parent_id 0. Changed state: device_viewport_size 1 painted_device_scale_factor 0 device_scale_factor 0

This means that we are in a state of having pushed a parent allocated it to the tree_impl. While then receiving a viewport size change.

Looking at LayerTreeHost::SetViewportSizeAndScale, the call to SetLocalSurfaceIdAllocationFromParent will clear has_pushed_local_surface_id_from_parent_ unless the parent sequence numbers are the same.

Two new possible sources, which I've yet to be able to confirm (as debugging logs make the error go away):

1)  LayerTreeHostImpl is updating the child sequence number as a result of the LayerTreeFrameSink loss, and it is arriving with a new size, but no change in parent sequence.

2)  BrowserCompositorViewMac is somehow producing a size update, but not advancing the parent sequence number.


LayerTreeHost::SetLocalSurfaceIdAllocationFromParent used to always clear the the pushed state. However this was stopped as the subsequent work triggered duplicate commits. Leading to two pass of surface sync instead of one: https://chromium-review.googlesource.com/c/chromium/src/+/1126325/

It may be valid to just always clear the pushed state for all LocalSurfaceID changes, but to only trigger commits for parent sequence changes.
Blockedon: 915915
Project Member

Comment 23 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 9746a213f859bf3ce39f73c20b57e3bd24f8c130
Author: jonross <jonross@chromium.org>
Date: Fri Jan 18 19:08:41 2019

Enforce Surface Invariants in RenderWidgetHostImpl

Currently, when the expectations around Surface Synchronization
are broken, we do not find out until after Compositor Frames are
submitted. With the Viz Service verifying, and then rejecting the
submission.

With one additional check in cc:LayerTreeHost::SetViewportSizeAndScale,
which was only running on Mac. Which occurs in the Renderer process.

This makes violations tough to debug. Especially when the Browser
process is often allocating the viz::LocalSurfaceIds incorrectly.

The Mac check has been failing on webkit_layout_tests, but is flaky.
The check is also noted as to not be valid on Aura or Mus due to other
bugs.

This change updated the cc::LayerTreeHost::SetViewportSizeAndScale check
to be on all platforms. While updating Aura and Mus to fix violations.

This change also adds a Browser side check in RenderWidgetHostImpl::SynchronizeVisualProperties
to ensure that if sizes are changing, that viz::LocalSurfaceIds have
advanced. This check will let us catch the cause of errors, such as
the flaking Mac webkit_layout_tests, so that we can fix them.

This new check however caused lots of tests to fail. This is because
we were incorrectly handling ChildLocalSurfaceId updates. We were
previously doing:
     - RenderWidgetHostImpl::DidUpdateVisualProperties
         - updated child sequence number and new size
     - RenderWidgstHostImpl::SyncrhonizeVisualProperties
         - messages Renderer with new size, but old LocalSurfaceIds
     - viz::ScopedSurfaceIdAllocator callbacks run
         - most of which call ParentLocalSurfaceId::UpdateFromChild
     - RenderWidgstHostImpl::SyncrhonizeVisualProperties
         - called with newly merged Id
         - if there has been no ack or changes, then no message sent to Renderer

This leads to the Browser and Renderer having different viz::LocalSurfaceIds.
And with the Renderer re-using the same id for different sizes, which is
a violation.

This change updates the logic to not send messages until the ids have merged:
    - RenderWidgetHostImpl::DidUpdateVisualProperties
         - updated child sequence number and new size
     - RenderWidgstHostImpl::SyncrhonizeVisualProperties
         - exits early, not messaging Renderer
     - viz::ScopedSurfaceIdAllocator callbacks run
         - most of which call ParentLocalSurfaceId::UpdateFromChild
     - RenderWidgstHostImpl::SyncrhonizeVisualProperties
         - message Renderer with new size, and new merged LocalSurfaceId.

TBR=sadrul@chromium.org

Bug:  915915 , 900948
Change-Id: I72e2b60c8e1adbdcfd276e2b18c2d83427475fcc
Reviewed-on: https://chromium-review.googlesource.com/c/1379972
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624227}
[modify] https://crrev.com/9746a213f859bf3ce39f73c20b57e3bd24f8c130/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/9746a213f859bf3ce39f73c20b57e3bd24f8c130/components/viz/common/surfaces/parent_local_surface_id_allocator.cc
[modify] https://crrev.com/9746a213f859bf3ce39f73c20b57e3bd24f8c130/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/9746a213f859bf3ce39f73c20b57e3bd24f8c130/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/9746a213f859bf3ce39f73c20b57e3bd24f8c130/ui/compositor/layer_unittest.cc

Comment 24 by jonr...@chromium.org, Jan 18 (4 days ago)

Status: Started (was: Assigned)
I believe that this change should fix the cause of this issue. However if not the new CHECK in RenderWidgetHostImpl should allow us to narrow down on the root cause.

I plan to separately land a re-enable of the failing Mac tests
Project Member

Comment 25 by bugdroid1@chromium.org, Jan 19 (4 days ago)

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

commit 6d93258fa5f0ff66e72342ca6876c4ee12c36dc7
Author: jonross <jonross@chromium.org>
Date: Sat Jan 19 02:39:33 2019

Stop suppressing two WebGL-related context loss tests on macOS.

  ContextLost_WebGLBlockedAfterJSNavigation
  ContextLost_WebGLUnblockedAfterUserInitiatedReload

Were both flaking due to the same root cause. We believe that has now been fixed.

TBR=kbr@chromium.org

Bug: 900948
Change-Id: I50598872ba8965a047badb69862defebb49fdaee
Reviewed-on: https://chromium-review.googlesource.com/c/1423114
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624427}
[modify] https://crrev.com/6d93258fa5f0ff66e72342ca6876c4ee12c36dc7/content/test/gpu/gpu_tests/context_lost_expectations.py

Project Member

Comment 26 by bugdroid1@chromium.org, Yesterday (40 hours ago)

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

commit 8e6ef49f74adb41d02018b3ecdb22df860d7a81b
Author: Jonathan Ross <jonross@chromium.org>
Date: Mon Jan 21 15:11:52 2019

Revert "Stop suppressing two WebGL-related context loss tests on macOS."

This reverts commit 6d93258fa5f0ff66e72342ca6876c4ee12c36dc7.

Reason for revert: The expected fix did not work. They still flake.

Original change's description:
> Stop suppressing two WebGL-related context loss tests on macOS.
> 
>   ContextLost_WebGLBlockedAfterJSNavigation
>   ContextLost_WebGLUnblockedAfterUserInitiatedReload
> 
> Were both flaking due to the same root cause. We believe that has now been fixed.
> 
> TBR=kbr@chromium.org
> 
> Bug: 900948
> Change-Id: I50598872ba8965a047badb69862defebb49fdaee
> Reviewed-on: https://chromium-review.googlesource.com/c/1423114
> Reviewed-by: Jonathan Ross <jonross@chromium.org>
> Commit-Queue: Jonathan Ross <jonross@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#624427}

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

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

Bug: 900948
Change-Id: I455358e186e2c881c808b0879189105eba684d66
Reviewed-on: https://chromium-review.googlesource.com/c/1425377
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624583}
[modify] https://crrev.com/8e6ef49f74adb41d02018b3ecdb22df860d7a81b/content/test/gpu/gpu_tests/context_lost_expectations.py

Project Member

Comment 27 by bugdroid1@chromium.org, Yesterday (39 hours ago)

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

commit d558a85c3cbe6950706121009c19559315ce1ebb
Author: jonross <jonross@chromium.org>
Date: Mon Jan 21 16:32:44 2019

Restrict LayerTreeHost::SetViewportSizeAndScale CHECK to Mac

We expanded a check in LayerTreeHost::SetViewportSizeAndScale from being Mac
only to all platforms. This was done in https://chromium-review.googlesource.com/c/chromium/src/+/1379972

We thought that the changes to RenderWidgetHostImpl would ensure that this was
always valid. We were wrong.

This change returns the check to Mac only

TBR: samans@chromium.org
Bug: 900948
Change-Id: Iccba0545ad1ff32d3df2d4f9ab7c118f1987b72d
Reviewed-on: https://chromium-review.googlesource.com/c/1425418
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Auto-Submit: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624602}
[modify] https://crrev.com/d558a85c3cbe6950706121009c19559315ce1ebb/cc/trees/layer_tree_host.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Today (13 hours ago)

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

commit a527b4b81bea9531f730e1cca083c359fd3d8b31
Author: jonross <jonross@chromium.org>
Date: Tue Jan 22 18:53:34 2019

Update LayerTreeHost Caching of LocalSurfaceIds

LayerTreeHost caches the viz::LocalSurfaceId provided by the parent.
When a tree activates this id is passed down to the LayerTreeHostImpl,
which is responsible for allocating the child portion of that id.

When a new id is sent to the LayerTreeHost it will not cause a commit
if the parent sequence number has not changed. This saves on spurious
commits for information the LayerTreeHostImpl already has.

However LayerTreeHost::SetViewportSizeAndScale checks that when visual
properties, such as size, change that the cached LocalSurfaceId has
also changed.

Currently LayerTreeHost::SetLocalSurfaceIdAllocationFromParent only
updates the cached state if it is causing a commit. This leads to the
cache drifting from the actual state known in the parent and child.
This can lead to the expectations in LayerTreeHost::SetViewportSizeAndScale
to fail.

This change updates the cache on all pushes from the parent. But still
restricts the tree commit to only when the parent sequence as changed.

      LayerTreeHostTestLocalSurfaceIdSkipChildNum.RunMultiThread_DelegatingRenderer

Test: LayerTreeHostTestLocalSurfaceIdSkipChildNum.RunSingleThread_DelegatingRenderer
Bug: 900948
Change-Id: I85be1ffdabe04d0c833042efe25bd8252764c63e
Reviewed-on: https://chromium-review.googlesource.com/c/1425964
Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624849}
[modify] https://crrev.com/a527b4b81bea9531f730e1cca083c359fd3d8b31/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/a527b4b81bea9531f730e1cca083c359fd3d8b31/cc/trees/layer_tree_host_unittest.cc

Sign in to add a comment