Bug 81841 - [chromium] Fix scrollbar layers holding onto invalid textures after lost context
Summary: [chromium] Fix scrollbar layers holding onto invalid textures after lost context
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: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-21 15:52 PDT by Adrienne Walker
Modified: 2012-03-22 09:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.92 KB, patch)
2012-03-21 17:16 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Use setTextureManager (4.38 KB, patch)
2012-03-21 21:10 PDT, Adrienne Walker
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 2012-03-21 15:52:50 PDT
LayerRendererChromium owns the render surface texture manager.  CCScrollbarLayerImpl requests textures out of this texture manager.  The texture manager pointer is stored in the managed texture on the layer.  When the context is lost, the LRC gets recreated with a new RS texture manager.  The CCScrollbarLayerImpl's managed texture is pointing at some bogus texture manager and can't reserve textures from it.  Oops.

See: http://code.google.com/p/chromium/issues/detail?id=119354
Comment 1 Adrienne Walker 2012-03-21 17:16:39 PDT
Created attachment 133148 [details]
Patch
Comment 2 Nat Duca 2012-03-21 18:02:14 PDT
W00t
Comment 3 James Robinson 2012-03-21 20:53:06 PDT
Comment on attachment 133148 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:98
> +    m_texture.clear();

This texture's managed by the LRC::m_renderSurfaceTextureManager, and we rebuild the LRC after a lost context so this texture should go bogus. Do we just need a setTextureManager() call before the reserve() in willDraw(), mayhaps?

didLostContext should only be for non-managed resources.
Comment 4 Adrienne Walker 2012-03-21 20:57:43 PDT
(In reply to comment #3)
> (From update of attachment 133148 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133148&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:98
> > +    m_texture.clear();
> 
> This texture's managed by the LRC::m_renderSurfaceTextureManager, and we rebuild the LRC after a lost context so this texture should go bogus. Do we just need a setTextureManager() call before the reserve() in willDraw(), mayhaps?
> 
> didLostContext should only be for non-managed resources.

Ok, sure.  It's the same difference from the scrollbar layer's perspective.  Clearing the texture seemed slightly cleaner since there won't ever be an invalid texture manager, but I guess CCVideoLayerImpl does what you say above already.  I can do that.
Comment 5 Adrienne Walker 2012-03-21 21:10:27 PDT
Created attachment 133181 [details]
Use setTextureManager
Comment 6 James Robinson 2012-03-21 21:21:24 PDT
Comment on attachment 133181 [details]
Use setTextureManager

Thanks. This isn't really much different, but I have wild dreams sometimes of making this a bit more automagic so you don't have to explicitly do this hookup before using the texture - maybe by having the reserve call be on some object that can take care of these details.
Comment 7 Nat Duca 2012-03-22 00:48:38 PDT
Comment on attachment 133181 [details]
Use setTextureManager

Gonna put this on cq so folks evaluating impl thread tomorrow can get this in their morning builds.
Comment 8 WebKit Review Bot 2012-03-22 00:50:37 PDT
Comment on attachment 133181 [details]
Use setTextureManager

Rejecting attachment 133181 [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:
patching file Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp
patching file Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp
Hunk #1 FAILED at 25.
Hunk #2 succeeded at 981 (offset -103 lines).
1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp.rej

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

Full output: http://queues.webkit.org/results/12070863
Comment 9 Nat Duca 2012-03-22 02:43:33 PDT
Whommmmp
Comment 10 Dana Jansens 2012-03-22 08:22:35 PDT
Comment on attachment 133181 [details]
Use setTextureManager

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

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:28
> +#include "cc/CCQuadCuller.h"

was this on purpose?
Comment 11 WebKit Review Bot 2012-03-22 08:37:51 PDT
Comment on attachment 133181 [details]
Use setTextureManager

Clearing flags on attachment: 133181

Committed r111708: <http://trac.webkit.org/changeset/111708>
Comment 12 WebKit Review Bot 2012-03-22 08:37:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Adrienne Walker 2012-03-22 09:09:53 PDT
(In reply to comment #10)
> (From update of attachment 133181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133181&action=review
> 
> > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:28
> > +#include "cc/CCQuadCuller.h"
> 
> was this on purpose?

Oops, I had originally done this test without a CCLTHI, so that's just vestigial.

Thanks for landing this.