Summary: | [chromium] LayerRenderChromium asserts about leaking textures. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrienne Walker <enne> | ||||||||
Component: | WebCore Misc. | Assignee: | James Robinson <jamesr> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | enne, jamesr, kbr, mihaip, nduca, vangelis, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Adrienne Walker
2011-09-15 14:16:17 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. One thing I forgot to do was 0-initialize m_contentsTextureMemoryUseBytes in LRC's ctor - wonder if that's it. (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. 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. Created attachment 108097 [details]
Patch
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(). 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. Created attachment 108404 [details]
Patch
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()? (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 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 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. (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 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 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 Committed r96141: <http://trac.webkit.org/changeset/96141> Reverted r96141 for reason: Breaks PrerenderBrowserTest.PrerenderHTML5Video in browser_tests Committed r96173: <http://trac.webkit.org/changeset/96173> Reverted because I broke the path where initial compositor context creation fails. Fix coming soon... Created attachment 108941 [details]
Patch
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 on attachment 108941 [details]
Patch
OK. r=me assuming you've tested the new patch against the previous failure mode.
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).
(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 on attachment 108941 [details] Patch Clearing flags on attachment: 108941 Committed r96186: <http://trac.webkit.org/changeset/96186> All reviewed patches have been landed. Closing bug. |