Commit addebc68 authored by Fady Samuel's avatar Fady Samuel Committed by Commit Bot

Surface synchronization: Ensure monotonically increasing IDs at activation

The crash log in 826186 suggests that the previous surface was already
destroyed by the time the next activates. It's unclear how we get into this
state.

One possible theory is surfaces are activating out of order:

1. CompositorFrame with LocalSurfaceID 2 activates.
2. CompositorFrame with LocalSurfaceID 3 activates.
3. LocalSurfaceID 2 is marked for destruction.
4. CompositorFrame with LocalSurfaceID 2 activates again.
5. Garbage collection happens, and LocalSurfaceID 2's surface is destroyed.
6. CompositorFrame with LocalSurfaceID 4 activates, but the previous surface,
   LocalSurfaceId 2 has been destroyed so SetPreviousFrameSurface crashes trying
   to access 2.

The only way to get into this state if is if there are two pending CompositorFrames
for a FrameSink at once. It's unclear how this can happen because cc should not
be issuing a new CompositorFrame until the previous CompositorFrame has activated.

Perhaps there's a cc bug? This CL adds a diagnostic check to verify that
LocalSurfaceIds activate in a monotonically increasing fashion.

Bug: 826186, 672962
Change-Id: I2c3826c63d2538fa9562be683d864a11de904dd0
Reviewed-on: https://chromium-review.googlesource.com/990200
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: default avatarenne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547544}
parent f35f5ac0
...@@ -103,6 +103,18 @@ void CompositorFrameSinkSupport::OnSurfaceActivated(Surface* surface) { ...@@ -103,6 +103,18 @@ void CompositorFrameSinkSupport::OnSurfaceActivated(Surface* surface) {
DCHECK(surface->HasActiveFrame()); DCHECK(surface->HasActiveFrame());
if (last_activated_surface_id_ != surface->surface_id()) { if (last_activated_surface_id_ != surface->surface_id()) {
if (last_activated_surface_id_.is_valid()) { if (last_activated_surface_id_.is_valid()) {
const LocalSurfaceId& local_surface_id =
surface->surface_id().local_surface_id();
const LocalSurfaceId& last_activated_local_surface_id =
last_activated_surface_id_.local_surface_id();
CHECK_GE(local_surface_id.parent_sequence_number(),
last_activated_local_surface_id.parent_sequence_number());
CHECK_GE(local_surface_id.child_sequence_number(),
last_activated_local_surface_id.child_sequence_number());
CHECK(local_surface_id.parent_sequence_number() >
last_activated_local_surface_id.parent_sequence_number() ||
local_surface_id.child_sequence_number() >
last_activated_local_surface_id.child_sequence_number());
Surface* prev_surface = Surface* prev_surface =
surface_manager_->GetSurfaceForId(last_activated_surface_id_); surface_manager_->GetSurfaceForId(last_activated_surface_id_);
DCHECK(prev_surface); DCHECK(prev_surface);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment