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

Issue 768049 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Canvas is leaking textures on shutdown

Project Member Reported by danakj@chromium.org, Sep 22 2017

Issue description

Canvas uses a WeakPtr in its release callback, so if it destroys itself while the display compositor is still holding the texture resource, by the time it is returned, nothing calls DeleteTexture() on it.

Pepper has the same problem but we think it would destroy the context on shutdown (unlike canvas which uses the long-lived main thread shared context), which would free the resources.

Assigned to me, but feel free to take it.

2 possible ways to fix canvas:

1. Keep the canvas structures alive with refcount from the release callback, so they can delete the texture always.

2. Insert a free-methods ReleaseCallback in before it gets to the canvas code, that checks the weakptr, and has a ref on the context, and deletes the texture if the weakptr is null so it can't pass it on to the canvas code.
 

Comment 1 by danakj@chromium.org, Sep 25 2017

Cc: kenrb@chromium.org
List of clients to look at:

Canvas: Canvas2DLayerBridge::PrepareTextureMailbox -
 https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp?rcl=e7bf32bcbe0a431a9c3d566190f5b4643a73b1e8&l=954

- Uses a weakptr, will leak.

WebGL: DrawingBuffer::PrepareTextureMailbox - https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp?rcl=e7bf32bcbe0a431a9c3d566190f5b4643a73b1e8&l=263

- Uses a RefPtr, but early outs if destruction_in_progress_, without deleting the texture. However DrawingBuffer also owns the context provider, so it should clean up the textures when it is destroyed.

Canvas Something?: ImageLayerBridge::PrepareTextureMailbox -
 https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/gpu/ImageLayerBridge.cpp?rcl=e7bf32bcbe0a431a9c3d566190f5b4643a73b1e8&l=68

- Uses a weakptr in the callback. Can leak. Uses a context embedded in a StaticBitmapImage, which comes from v8? Probably the main thread shared context which is not destroyed ever.

Pepper: PepperPluginInstanceImpl::PrepareTextureMailbox - https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_plugin_instance_impl.cc?rcl=e7bf32bcbe0a431a9c3d566190f5b4643a73b1e8&l=2209

- Uses a weakptr in the callback. Can leak if the context isn't destroyed.

Comment 2 by danakj@chromium.org, Sep 25 2017

Also noticed https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp?rcl=e4ff3e734c7a074cac36ba5dd7582d5c5852b70f&l=1041

    if (info && !lost_resource) {
      if (context_or_layer_bridge_lost) {
        DeleteCHROMIUMImage(context_provider_wrapper,
                            std::move(info->gpu_memory_buffer_),
                            info->image_id_, info->texture_id_);
      } else {
        layer_bridge->image_info_cache_.push_back(info);
      }
    }

If the resource is reported lost, canvas does not delete it, which will leak it as well.

Comment 3 by junov@chromium.org, Sep 25 2017

Owner: junov@chromium.org
Thanks for investigating this.

By coincidence, I already have a WIP CL that fixes this. 
Basically I am refactoring Canvas2DLayerBridge to use an RAII design for resource management.  The release callback created by PreparTextureMailbox will hold a RefPtr<CanvasResource> that knows how to destroy itself without any dependency on Canvas2DLayerBridge.

Comment 4 by danakj@chromium.org, Sep 25 2017

Oh great! Do you want to also do the same for ImageLayerBridge?

I can send you a CL for the lost_resources thing in #2

Comment 5 by junov@chromium.org, Sep 25 2017

ImageLayerBridge -> In a follow-up CL

I did not realize that lost resources still needed to be explicitly deleted. The code you mention in #2 is going away in my refactor, so don't bother fixing it.  In the new code, I will make sure to delete lost resources.

Comment 6 by danakj@chromium.org, Sep 25 2017

Cc: -kenrb@chromium.org

Comment 7 by danakj@chromium.org, Sep 25 2017

Cool, thanks :)
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 25 2017

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

commit 183dc06b8570c6aee5f527fb9cf48591398b0b03
Author: danakj <danakj@chromium.org>
Date: Mon Sep 25 18:16:49 2017

Delete resources reported as lost when returned to Canvas2DLayerBridge.

Lost resources are still valid textures, but they can not be re-used
by the client. The client is still responsible for freeing them.

R=junov@chromium.org

Bug:  768049 
Change-Id: Icbd14789f27a6873648b2ffd3a07099664a02698
Reviewed-on: https://chromium-review.googlesource.com/682101
Reviewed-by: Justin Novosad <junov@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504105}
[modify] https://crrev.com/183dc06b8570c6aee5f527fb9cf48591398b0b03/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp

Components: Blink>Canvas
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 8 2017

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

commit 5b07996e178c5f4e153e771d48ab839867fa7440
Author: Justin Novosad <junov@chromium.org>
Date: Wed Nov 08 23:26:00 2017

Reland: Refactor of Canvas2DLayerBridge resource model

Transition to polymorphic RAII model. The purpose of this change
is to improve the maintainability and extendability of the code.
The GpuMemoryBuffer code is also getting a drive-by optimization
that will eliminate unnecessary gpu commandbuffer flushing and
verified sync tokens, which result in synchronous IPCs.

Original code review:
https://chromium-review.googlesource.com/c/chromium/src/+/728340

Fixed bad include of CanvasResourceProvider.cpp instead of .h

BUG= 773466 , 768049 ,735630
TBR=fserb@chromium.org,xlai@chromium.org,mostynb@vewd.com,sky@chromium.org

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I143b7dc78d50ded8f1541868edecda97946e6292
Reviewed-on: https://chromium-review.googlesource.com/758562
Commit-Queue: Justin Novosad <junov@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515001}
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/LayoutTests/fast/canvas/canvas-incremental-repaint.html
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/LayoutTests/fast/canvas/canvas-toDataURL-jpeg-maximum-quality.html
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/LayoutTests/fast/canvas/canvas-toDataURL-webp.html
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/LayoutTests/fast/canvas/script-tests/canvas-lost-gpu-context.js
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/LayoutTests/fast/canvas/script-tests/canvas-toBlob-toDataURL-race.js
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/LayoutTests/virtual/display_list_2d_canvas/fast/canvas/canvas-lost-gpu-context-expected.txt
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/LayoutTests/virtual/gpu/fast/canvas/canvas-lost-gpu-context-expected.txt
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/core/paint/HTMLCanvasPainterTest.cpp
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/modules/canvas/canvas2d/CanvasRenderingContext2DTest.cpp
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridgeTest.cpp
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/CanvasColorParams.cpp
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/CanvasColorParams.h
[add] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/CanvasResource.cpp
[add] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/CanvasResource.h
[add] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/CanvasResourceProvider.cpp
[add] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/CanvasResourceProvider.h
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/DEPS
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/GraphicsTypes.h
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/ImageBuffer.h
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/ImageBufferSurface.cpp
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/ImageBufferSurface.h
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.h
[modify] https://crrev.com/5b07996e178c5f4e153e771d48ab839867fa7440/tools/metrics/histograms/enums.xml

Status: Fixed (was: Assigned)

Sign in to add a comment