Issue 642836 link

Starred by 1 user

Issue metadata

Status:	Fixed
Owner:	ericrk@chromium.org
Closed:	Sep 2016
Cc:	dsinclair@chromium.org
Components:	Internals>GPU>Internals
EstimatedDays:	----
NextAction:	----
OS:	All
Pri:	1
Type:	Bug-Regression
M-54 Hotlist-Merge-Approved merge-merged-2840

Context Visibility Tracking Does not Handle Destruction

Project Member Reported by ericrk@chromium.org, Aug 31 2016

Issue description

Currently, when a LayerTreeHostImpl or GLRenderer is destroyed, we do not transition visibility and do not correctly notify ContextSupport that a ContextClient has become invisible. This can lead to the visibility refcoutn on ContextSupport being unbalanced, meaning we won't free background resources correctly.

Comment 1 by ericrk@chromium.org, Aug 31 2016

This is fixed with crrev.com/2278283003

Comment 2 by ericrk@chromium.org, Aug 31 2016

Labels: Merge-Request-54
Status: Fixed (was: Started)

Comment 3 by ericrk@chromium.org, Aug 31 2016

Status: Started (was: Fixed)

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 31 2016

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

commit 1e16f5f83b5a634369a7e9d82fddfe2e1ec10a9d
Author: ericrk <ericrk@chromium.org>
Date: Wed Aug 31 22:56:09 2016

Fix LayerTreeHostCacheBehaviorOnOutputSurfaceRecreated test

LayerTreeHostCacheBehaviorOnOutputSurfaceRecreated test was incorrectly
relying on WillBeginImplFrameOnThread only being called once after the
OutputSurface was recreated. In reality, this function could be called
multiple times.

This patch causes us to handle cleanup in DidInitializeOutputSurface,
which will only be called once after recreating an OutputSurface.

BUG= 642881 , 642836 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2297413002
Cr-Commit-Position: refs/heads/master@{#415791}

[modify] https://crrev.com/1e16f5f83b5a634369a7e9d82fddfe2e1ec10a9d/cc/trees/layer_tree_host_unittest.cc

Comment 5 by dimu@chromium.org, Sep 1 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved

Your change meets the bar and is auto-approved for M54 (branch: 2840)

Comment 6 by ligim...@chromium.org, Sep 2 2016

Please merge your change to M54 (branch: 2840) before 5:00 PM PST Monday [09/05] if you would like to make it to M54 Beta promotion on Thursday [09/08].

Project Member

Comment 7 by sheriffbot@chromium.org, Sep 5 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 8 by ligim...@chromium.org, Sep 7 2016

Please merge your change to M54 (branch: 2840) before 3:00 PM PST today if you would like to make it to M54 Beta promotion on Thursday [09/08].

Project Member

Comment 9 by sheriffbot@chromium.org, Sep 8 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 10 by ligim...@chromium.org, Sep 8 2016

Could you please confirm whether this change is baked/verified in Canary and safe to merge?If yes, merge your change to M54 (branch: 2840) so that we could take this for next Beta Release.

Comment 11 by ligim...@chromium.org, Sep 16 2016

Gentle ping.

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 20 2016

Labels: -merge-approved-54 merge-merged-2840

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

commit e9cdadf4dbe2a508a156a97befd8a017d1595f18
Author: Eric Karl <ericrk@chromium.org>
Date: Tue Sep 20 15:53:53 2016

Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2278283003
Cr-Commit-Position: refs/heads/master@{#415491}
(cherry picked from commit f6c4fb6b0b77f35f094cb73c49b0bfe22d3f4461)

BUG= 642836 

Review URL: https://codereview.chromium.org/2357673002 .

Cr-Commit-Position: refs/branch-heads/2840@{#438}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/android_webview/browser/aw_render_thread_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/android_webview/browser/aw_render_thread_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/blimp/client/feature/compositor/blimp_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/blimp/client/feature/compositor/blimp_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/BUILD.gn
[add] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/context_cache_controller.cc
[add] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/context_cache_controller.h
[add] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/context_cache_controller_unittest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/gl_renderer.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/gl_renderer.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/gl_renderer_unittest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/raster/raster_buffer_provider_perftest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_context_support.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_context_support.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_in_process_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_in_process_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/content/common/gpu/client/context_provider_command_buffer.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/content/common/gpu/client/context_provider_command_buffer.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/gpu/command_buffer/client/context_support.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/gpu/command_buffer/client/gles2_implementation.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/gpu/command_buffer/client/gles2_implementation.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/gpu/command_buffer/client/gles2_implementation_unittest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/services/ui/public/cpp/context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/services/ui/public/cpp/context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/services/ui/surfaces/surfaces_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/services/ui/surfaces/surfaces_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/ui/compositor/test/in_process_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/ui/compositor/test/in_process_context_provider.h

Project Member

Comment 13 by bugdroid1@chromium.org, Sep 20 2016

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

commit 2a2b4b5a2d2a62d8f1c8fc6d86b53e561aaecd25
Author: Eric Karl <ericrk@chromium.org>
Date: Tue Sep 20 16:02:46 2016

Fix LayerTreeHostCacheBehaviorOnOutputSurfaceRecreated test

LayerTreeHostCacheBehaviorOnOutputSurfaceRecreated test was incorrectly
relying on WillBeginImplFrameOnThread only being called once after the
OutputSurface was recreated. In reality, this function could be called
multiple times.

This patch causes us to handle cleanup in DidInitializeOutputSurface,
which will only be called once after recreating an OutputSurface.

BUG= 642881 , 642836 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2297413002
Cr-Commit-Position: refs/heads/master@{#415791}
(cherry picked from commit 1e16f5f83b5a634369a7e9d82fddfe2e1ec10a9d)

Review URL: https://codereview.chromium.org/2354983002 .

Cr-Commit-Position: refs/branch-heads/2840@{#440}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/2a2b4b5a2d2a62d8f1c8fc6d86b53e561aaecd25/cc/trees/layer_tree_host_unittest.cc

Comment 14 by ericrk@chromium.org, Sep 20 2016

Status: Fixed (was: Started)

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 27 2016

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

commit e9cdadf4dbe2a508a156a97befd8a017d1595f18
Author: Eric Karl <ericrk@chromium.org>
Date: Tue Sep 20 15:53:53 2016

Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2278283003
Cr-Commit-Position: refs/heads/master@{#415491}
(cherry picked from commit f6c4fb6b0b77f35f094cb73c49b0bfe22d3f4461)

BUG= 642836 

Review URL: https://codereview.chromium.org/2357673002 .

Cr-Commit-Position: refs/branch-heads/2840@{#438}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/android_webview/browser/aw_render_thread_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/android_webview/browser/aw_render_thread_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/blimp/client/feature/compositor/blimp_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/blimp/client/feature/compositor/blimp_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/BUILD.gn
[add] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/context_cache_controller.cc
[add] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/context_cache_controller.h
[add] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/context_cache_controller_unittest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/gl_renderer.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/gl_renderer.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/output/gl_renderer_unittest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/raster/raster_buffer_provider_perftest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_context_support.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_context_support.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_in_process_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/test/test_in_process_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/content/common/gpu/client/context_provider_command_buffer.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/content/common/gpu/client/context_provider_command_buffer.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/gpu/command_buffer/client/context_support.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/gpu/command_buffer/client/gles2_implementation.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/gpu/command_buffer/client/gles2_implementation.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/gpu/command_buffer/client/gles2_implementation_unittest.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/services/ui/public/cpp/context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/services/ui/public/cpp/context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/services/ui/surfaces/surfaces_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/services/ui/surfaces/surfaces_context_provider.h
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/ui/compositor/test/in_process_context_provider.cc
[modify] https://crrev.com/e9cdadf4dbe2a508a156a97befd8a017d1595f18/ui/compositor/test/in_process_context_provider.h

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 27 2016

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

commit 2a2b4b5a2d2a62d8f1c8fc6d86b53e561aaecd25
Author: Eric Karl <ericrk@chromium.org>
Date: Tue Sep 20 16:02:46 2016

Fix LayerTreeHostCacheBehaviorOnOutputSurfaceRecreated test

LayerTreeHostCacheBehaviorOnOutputSurfaceRecreated test was incorrectly
relying on WillBeginImplFrameOnThread only being called once after the
OutputSurface was recreated. In reality, this function could be called
multiple times.

This patch causes us to handle cleanup in DidInitializeOutputSurface,
which will only be called once after recreating an OutputSurface.

BUG= 642881 , 642836 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2297413002
Cr-Commit-Position: refs/heads/master@{#415791}
(cherry picked from commit 1e16f5f83b5a634369a7e9d82fddfe2e1ec10a9d)

Review URL: https://codereview.chromium.org/2354983002 .

Cr-Commit-Position: refs/branch-heads/2840@{#440}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/2a2b4b5a2d2a62d8f1c8fc6d86b53e561aaecd25/cc/trees/layer_tree_host_unittest.cc

►

Issue 642836 link

Issue metadata

Context Visibility Tracking Does not Handle Destruction

Issue description

Comment 1 by ericrk@chromium.org, Aug 31 2016

Comment 1 Deleted

Comment 2 by ericrk@chromium.org, Aug 31 2016

Comment 2 Deleted

Comment 3 by ericrk@chromium.org, Aug 31 2016

Comment 3 Deleted

Comment 4 by bugdroid1@chromium.org, Aug 31 2016

Comment 4 Deleted

Comment 5 by dimu@chromium.org, Sep 1 2016

Comment 5 Deleted

Comment 6 by ligim...@chromium.org, Sep 2 2016

Comment 6 Deleted

Comment 7 by sheriffbot@chromium.org, Sep 5 2016

Comment 7 Deleted

Comment 8 by ligim...@chromium.org, Sep 7 2016

Comment 8 Deleted

Comment 9 by sheriffbot@chromium.org, Sep 8 2016

Comment 9 Deleted

Comment 10 by ligim...@chromium.org, Sep 8 2016

Comment 10 Deleted

Comment 11 by ligim...@chromium.org, Sep 16 2016

Comment 11 Deleted

Comment 12 by bugdroid1@chromium.org, Sep 20 2016

Comment 12 Deleted

Comment 13 by bugdroid1@chromium.org, Sep 20 2016

Comment 13 Deleted

Comment 14 by ericrk@chromium.org, Sep 20 2016

Comment 14 Deleted

Comment 15 by bugdroid1@chromium.org, Oct 27 2016

Comment 15 Deleted

Comment 16 by bugdroid1@chromium.org, Oct 27 2016

Comment 16 Deleted

Sign in to add a comment