Bug 68190 - [chromium] LayerRenderChromium asserts about leaking textures.
Summary: [chromium] LayerRenderChromium asserts about leaking textures.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-15 14:16 PDT by Adrienne Walker
Modified: 2011-09-27 21:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.69 KB, patch)
2011-09-20 18:49 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (60.38 KB, patch)
2011-09-22 14:53 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (60.49 KB, patch)
2011-09-27 17:35 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-09-15 14:16:17 PDT
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).
Comment 1 Adrienne Walker 2011-09-15 14:17:27 PDT
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.
Comment 2 James Robinson 2011-09-15 14:18:17 PDT
One thing I forgot to do was 0-initialize m_contentsTextureMemoryUseBytes in LRC's ctor - wonder if that's it.
Comment 3 James Robinson 2011-09-15 14:25:53 PDT
(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.
Comment 4 James Robinson 2011-09-20 18:45:27 PDT
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.
Comment 5 James Robinson 2011-09-20 18:49:34 PDT
Created attachment 108097 [details]
Patch
Comment 6 Vangelis Kokkevis 2011-09-21 00:32:37 PDT
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().
Comment 7 James Robinson 2011-09-21 00:51:23 PDT
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.
Comment 8 James Robinson 2011-09-22 14:53:33 PDT
Created attachment 108404 [details]
Patch
Comment 9 James Robinson 2011-09-22 14:55:16 PDT
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()?
Comment 10 James Robinson 2011-09-22 14:58:53 PDT
(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.
Comment 11 Adrienne Walker 2011-09-22 17:43:46 PDT
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?
Comment 12 James Robinson 2011-09-22 17:51:45 PDT
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.
Comment 13 Adrienne Walker 2011-09-22 18:00:56 PDT
(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.  :)
Comment 14 Kenneth Russell 2011-09-26 19:05:36 PDT
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.
Comment 15 WebKit Review Bot 2011-09-27 10:47:56 PDT
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
Comment 16 James Robinson 2011-09-27 12:40:32 PDT
Committed r96141: <http://trac.webkit.org/changeset/96141>
Comment 17 Mihai Parparita 2011-09-27 17:24:04 PDT
Reverted r96141 for reason:

Breaks PrerenderBrowserTest.PrerenderHTML5Video in browser_tests

Committed r96173: <http://trac.webkit.org/changeset/96173>
Comment 18 James Robinson 2011-09-27 17:31:54 PDT
Reverted because I broke the path where initial compositor context creation fails.  Fix coming soon...
Comment 19 James Robinson 2011-09-27 17:35:03 PDT
Created attachment 108941 [details]
Patch
Comment 20 James Robinson 2011-09-27 17:39:50 PDT
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.
Comment 21 Kenneth Russell 2011-09-27 20:48:22 PDT
Comment on attachment 108941 [details]
Patch

OK. r=me assuming you've tested the new patch against the previous failure mode.
Comment 22 James Robinson 2011-09-27 20:50:48 PDT
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).
Comment 23 James Robinson 2011-09-27 20:51:11 PDT
(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
Comment 24 WebKit Review Bot 2011-09-27 21:55:37 PDT
Comment on attachment 108941 [details]
Patch

Clearing flags on attachment: 108941

Committed r96186: <http://trac.webkit.org/changeset/96186>
Comment 25 WebKit Review Bot 2011-09-27 21:55:43 PDT
All reviewed patches have been landed.  Closing bug.