Bug 81841 – [chromium] Fix scrollbar layers holding onto invalid textures after lost context

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

[Adrienne Walker]

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

[Adrienne Walker]

Created attachment 133148 [details]
Patch

[Nat Duca]

W00t

[James Robinson]

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.

[Adrienne Walker]

(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.

[Adrienne Walker]

Created attachment 133181 [details]
Use setTextureManager

[James Robinson]

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.

[Nat Duca]

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.

[WebKit Review Bot]

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

[Nat Duca]

Whommmmp

[Dana Jansens]

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?

[WebKit Review Bot]

Comment on attachment 133181 [details]
Use setTextureManager

Clearing flags on attachment: 133181

Committed r111708: <http://trac.webkit.org/changeset/111708>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

(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.