Bug 67955 – [skia] fast/canvas/setWidthResetAfterForcedRender is flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[skia] fast/canvas/setWidthResetAfterForcedRender is flaky

[James Robinson]

Created attachment 107088 [details]
Patch

[James Robinson]

See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcanvas%2FsetWidthResetAfterForcedRender.html&showExpectations=true&group=%40ToT%20GPU%20Mesa%20-%20chromium.org to see how often this test fails.

When running locally, I also saw: [24615:24615:3633584029826:FATAL:gles2_cmd_decoder.cc(4143)] glBindTexture: id not generated by glGenTextures on the console, which lead to this bugfix.  The texture in question is allocated by skia and deleted by:

	gpu::gles2::GLES2Implementation::DeleteTextures() [0x99a171]
	GLES2DeleteTextures [0x1ea521b]
	GrGLTexID::~GrGLTexID() [0x1051d9c]
	SkRefCnt::unref() [0x48d5be]
	SkSafeUnref<>() [0x1050a0c]
	GrGLRenderTarget::onRelease() [0x1050933]
	GrResource::release() [0x1003b88]
	GrGLRenderTarget::~GrGLRenderTarget() [0x1050b6a]
	SkRefCnt::unref() [0x48d5be]
	SkSafeUnref<>() [0x101fbe6]
	SkGrRenderTargetPixelRef::~SkGrRenderTargetPixelRef() [0x101f99f]
	SkRefCnt::unref() [0x48d5be]
	SkBitmap::freePixels() [0x4894b9]
	SkBitmap::~SkBitmap() [0x4888ba]
	SkDevice::~SkDevice() [0x4982ac]
	SkGpuDevice::~SkGpuDevice() [0x1017e86]
	SkRefCnt::unref() [0x48d5be]
	DeviceCM::~DeviceCM() [0x494b39]
	SkCanvas::internalRestore() [0x490051]
	SkCanvas::~SkCanvas() [0x48f0e7]
	skia::PlatformCanvas::~PlatformCanvas() [0x4f2c86]
	WTF::deleteOwnedPtr<>() [0x46ff1a]
	WTF::OwnPtr<>::~OwnPtr() [0x46e8e5]
	WebCore::ImageBufferData::~ImageBufferData() [0xf6073a]
	WebCore::ImageBuffer::~ImageBuffer() [0xf5f6fc]
	WTF::deleteOwnedPtr<>() [0xaaf70b]
	WTF::OwnPtr<>::clear() [0xca2862]
	WebCore::HTMLCanvasElement::setSurfaceSize() [0xca019f]
	WebCore::HTMLCanvasElement::reset() [0xc9fc5b]
	WebCore::HTMLCanvasElement::parseMappedAttribute() [0xc9f312]
	WebCore::StyledElement::attributeChanged() [0x1861e4e]
	WebCore::Element::setAttribute() [0x17f5e51]
	WebCore::Element::setAttribute() [0x17f3f38]
	WebCore::HTMLCanvasElement::setWidth() [0xc9f513]
	WebCore::HTMLCanvasElementInternal::widthAttrSetter() [0x1b91f72]


This might be causing all sorts of other issues in the wild - after landing I propose merging this to m15, just in case.

[Brian Salomon]

Great find. (unofficially) LGTM.

[Stephen White]

Comment on attachment 107088 [details]
Patch

Good catch!

BTW, twiz landed a one-liner just below this which might conflict.  I'll let you decide whether to update it yourself or try cq+.

[James Robinson]

Created attachment 107188 [details]
Patch

[James Robinson]

Patch slightly updated - the previous version was actually trying to draw from texture id 0 (erk).

[James Robinson]

*** Bug 67144 has been marked as a duplicate of this bug. ***

[Stephen White]

Comment on attachment 107188 [details]
Patch

After some discussion offline, we're a little concerned that this might be papering over a deeper problem (outstanding question is, why is the compositor still rendering with this layer after the ImageBuffer has unref'ed it).

However, this fix looks safe regardless, so I'll let James decide whether to land this or hold off until after deeper investigation.  r=me

[Stephen White]

This test seems to have become unflaky.  The last failure on the Mesa bots was against WebKit r94962, and it has been green since.

I suspect that perhaps twiz's change in r94982 fixed it, although that is quite a few revs from the last failure.  James, it is possible that your change in r94964 fixed it?

At any rate, I'm going to mark it as passing in test_expectations.

[James Robinson]

I've noticed that as well, although I'm still getting stderr output locally.  Marking the test as passing seems like a fine thing to do, I'll pull up to ToT and see if I am still getting stderr output or not.

[James Robinson]

It looks like the problem is still happening, although it's not causing the test to fail.  For example on this run:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/13256/steps/webkit_gpu_tests/logs/stdio

grep for "output stderr lines" and you see:


2011-09-14 11:40:19,310 31916 single_test_runner.py:221 DEBUG worker/1 fast/canvas/setWidthResetAfterForcedRender.html output stderr lines:
[31927:31927:4826333073742:ERROR:gles2_cmd_decoder.cc(4136)] glBindTexture: id not generated by glGenTextures


this means we're still binding a texture that has already been deleted because HTMLCanvasElement::setSurfaceSize() is deleting the ImageBuffer (which owns the backing texture for the canvas) but nothing is triggering a compositing tree rebuild.  I think I have a better fix in mind, however.  Will update soon.

[James Robinson]

I've changed my mind, I think the original patch is still valid.  When a canvas is reset we destroy the ImageBuffer (which in ganesh owns the texture) but we aren't logically changing the compositing requirements of the page, so it makes sense to leave the layer in place.  Most of the time we'll recreate the ImageBuffer very soon anyway.

[James Robinson]

Committed r95114: <http://trac.webkit.org/changeset/95114>