Bug 68190 – [chromium] LayerRenderChromium asserts about leaking textures.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adrienne Walker]

Open chromium.  Open http://www.webkit.org/blog-files/3d-transforms/poster-circle.html.  Duplicate that tab.  Close either tab.

Assert in ~LayerRendererChromium() on ASSERT(!m_contentsTextureMemoryUseBytes).

[Adrienne Walker]

This was with ToT WebKit (r95222).  I'm not sure if it's really leaking textures or if there's some misreporting in the final commit flow.

[James Robinson]

One thing I forgot to do was 0-initialize m_contentsTextureMemoryUseBytes in LRC's ctor - wonder if that's it.

[James Robinson]

(In reply to comment #2)
> One thing I forgot to do was 0-initialize m_contentsTextureMemoryUseBytes in LRC's ctor - wonder if that's it.

Nope, that's not it.  Something is genuinely funky with shutdown - I'll take a look.

[James Robinson]

Well I kind of suck.  The textures are getting deleted, but I'm not informing the LayerRendererChromium.  I'm really not sure why the layout tests don't catch this.

[James Robinson]

Created attachment 108097 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 108097 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108097&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:100
> +    hostImpl->layerRenderer()->setContentsTextureMemoryUseBytes(0);

This seems somewhat fragile. Could the layerRenderer keep a pointer to the contentsTextureManager so that it can query the memory use directly?  Otherwise, any operation on the textures will need to be followed by a call to setContentsTextureMemoryUseBytes().

[James Robinson]

Issue there is that the contents texture manager and the LayerRendererChromium live on different threads.  It's only safe for one to access the other during synchronization points - so during the commit phase, or during the synchronized shutdown routine.  I really want to avoid providing pointers to objects that can only be touched at very specific times whenever possible - it's very easy to introduce a bad use of a pointer if it's just sitting there.

It definitely is a little yucky to reach through the CCLayerTreeHostImpl to get at the LRC now that you point it out, however - it'd be better if that was interface on CCLayerTreeHostImpl.

[James Robinson]

Created attachment 108404 [details]
Patch

[James Robinson]

Well this patch got a little bigger but I think this way is much cleaner.  It should be possible to write direct unit tests for the TextureManager without having to mock out all of GraphicsContext3D, and this avoids poking through to the LayerRendererChromium from CCLayerTreeHost which is also nice.

I'm tracking the texture creation/deletion calls in TrackingTextureAllocator in terms of bytes, but upon reflection maybe it makes more sense to just track the number of calls made and make sure that we issue a delete() for every create()?

[James Robinson]

(In reply to comment #9)
> I'm tracking the texture creation/deletion calls in TrackingTextureAllocator in terms of bytes, but upon reflection maybe it makes more sense to just track the number of calls made and make sure that we issue a delete() for every create()?

Disregard this sentence - I need to track the estimate memory use so that we know how much memory to give the RenderSurface manager.

[Adrienne Walker]

Comment on attachment 108404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108404&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-435
> -// FIXME: This method should eventually be replaced by a proper texture manager.

Hooray for removing this code.  Is this just unused at this point?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-158
>  void CCLayerTreeHost::didRecreateGraphicsContext(bool success)
>  {
> -    m_contentsTextureManager->evictAndDeleteAllTextures(0);

I can understand that the delete step isn't needed here, but don't you still need an evictAllTextures() step? Otherwise you have a bunch of ManagedTexture objects with texture ids that aren't valid and a contents texture manager that will gladly say that it still has them (allocated by the wrong allocator).  Or, am I missing something?

[James Robinson]

Comment on attachment 108404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108404&action=review

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-435
>> -// FIXME: This method should eventually be replaced by a proper texture manager.
> 
> Hooray for removing this code.  Is this just unused at this point?

Yes, these two functions were uncalled before I made any other changes.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-158
>> -    m_contentsTextureManager->evictAndDeleteAllTextures(0);
> 
> I can understand that the delete step isn't needed here, but don't you still need an evictAllTextures() step? Otherwise you have a bunch of ManagedTexture objects with texture ids that aren't valid and a contents texture manager that will gladly say that it still has them (allocated by the wrong allocator).  Or, am I missing something?

That still happens, just in a different place.  See CCSingleThreadProxy::recreateContextIfNeeded():

221        m_layerTreeHost->deleteContentsTexturesOnCCThread(m_layerTreeHostImpl->contentsTextureAllocator());

The reason for moving it is in that function we're synchronized across threads and can access the real contents texture allocator, so we can go ahead and issue a bunch of delete calls.  Even though they won't do anything on the actual GraphicsContext3D, since it's a lost context, we can still track the delete calls and make sure we haven't lost any textures along the way.

[Adrienne Walker]

(In reply to comment #12)
> (From update of attachment 108404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108404&action=review

> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-158
> >> -    m_contentsTextureManager->evictAndDeleteAllTextures(0);
> > 
> > I can understand that the delete step isn't needed here, but don't you still need an evictAllTextures() step? Otherwise you have a bunch of ManagedTexture objects with texture ids that aren't valid and a contents texture manager that will gladly say that it still has them (allocated by the wrong allocator).  Or, am I missing something?
> 
> That still happens, just in a different place.  See CCSingleThreadProxy::recreateContextIfNeeded():
> 
> 221        m_layerTreeHost->deleteContentsTexturesOnCCThread(m_layerTreeHostImpl->contentsTextureAllocator());
> 
> The reason for moving it is in that function we're synchronized across threads and can access the real contents texture allocator, so we can go ahead and issue a bunch of delete calls.  Even though they won't do anything on the actual GraphicsContext3D, since it's a lost context, we can still track the delete calls and make sure we haven't lost any textures along the way.

Ah, I looked and must have missed that.  Being able to track potential texture loss even during a lost context seems like an excellent sanity check.


This patch unofficially looks good to me, then.  :)

[Kenneth Russell]

Comment on attachment 108404 [details]
Patch

This looks good to me overall but I only gave it a cursory look. r+ based mainly on enne's review. One thought is that the presence of both TextureManager and TextureAllocator classes is a little confusing.

[WebKit Review Bot]

Comment on attachment 108404 [details]
Patch

Rejecting attachment 108404 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 FAILED -- saving rejects to file Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp.rej
patching file Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp
Hunk #1 FAILED at 216.
Hunk #2 succeeded at 430 (offset 150 lines).
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/9884228

[James Robinson]

Committed r96141: <http://trac.webkit.org/changeset/96141>

[Mihai Parparita]

Reverted r96141 for reason:

Breaks PrerenderBrowserTest.PrerenderHTML5Video in browser_tests

Committed r96173: <http://trac.webkit.org/changeset/96173>

[James Robinson]

Reverted because I broke the path where initial compositor context creation fails.  Fix coming soon...

[James Robinson]

Created attachment 108941 [details]
Patch

[James Robinson]

The change in this patch from the one that was landed and reverted is changing CCLayerTreeHostImpl::contentsTextureAllocator() from:

TextureAllocator* CCLayerTreeHostImpl::contentsTextureAllocator() const
{
    return m_layerRenderer->contentsTextureAllocator();
}

to:

TextureAllocator* CCLayerTreeHostImpl::contentsTextureAllocator() const
{
    return m_layerRenderer ? m_layerRenderer->contentsTextureAllocator() : 0;
}

The reason is that if the initial compositor context initialization fails, CCLayerTreeHostImpl::initializeLayerRenderer() will return with a null m_layerRenderer.  This failure propagates back up through CCLayerTreeHost::initialize() to WebViewImpl, which then tears down the CCLayerTreeHost. ~CCLayerTreeHost() calls m_proxy->stop(), which calls m_layerTreeHost->deleteContentsTexturesOnCCThread(m_layerTreeHostImpl->contentsTextureAllocator());.  deleteContentsTexture() can handle a NULL TextureAllocator* just fine, but we were crashing before getting there.

This shows up on the browser_tests (and nowhere else) because this codepath is only hit when compositor initialization fails, and the GPU stack always fails to initialize on the VMs these tests run on.  I've added this failure case to the testing doc so we can get some better coverage.

[Kenneth Russell]

Comment on attachment 108941 [details]
Patch

OK. r=me assuming you've tested the new patch against the previous failure mode.

[James Robinson]

Comment on attachment 108941 [details]
Patch

Yeah - running the browser_tests binary with --use-gl=egl on my linux box goes down the same GL initialization path that bots see, so I could repro the failure (once I knew to check for that).

[James Robinson]

(In reply to comment #22)
> (From update of attachment 108941 [details])
> Yeah - running the browser_tests binary with --use-gl=egl on my linux box goes down the same GL initialization path that bots see

"the same path" in that it's a GL path that fails to initialize

[WebKit Review Bot]

Comment on attachment 108941 [details]
Patch

Clearing flags on attachment: 108941

Committed r96186: <http://trac.webkit.org/changeset/96186>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.