Bug 67955 - [skia] fast/canvas/setWidthResetAfterForcedRender is flaky
Summary: [skia] fast/canvas/setWidthResetAfterForcedRender is flaky
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
: 67144 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-09-12 15:00 PDT by James Robinson
Modified: 2011-09-14 13:11 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.88 KB, patch)
2011-09-12 15:02 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (3.37 KB, patch)
2011-09-13 10:02 PDT, James Robinson
senorblanco: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-09-12 15:00:49 PDT
[skia] fast/canvas/setWidthResetAfterForcedRender is flaky
Comment 1 James Robinson 2011-09-12 15:02:05 PDT
Created attachment 107088 [details]
Patch
Comment 2 James Robinson 2011-09-12 15:04:18 PDT
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.
Comment 3 Brian Salomon 2011-09-13 05:54:01 PDT
Great find. (unofficially) LGTM.
Comment 4 Stephen White 2011-09-13 08:49:10 PDT
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+.
Comment 5 James Robinson 2011-09-13 10:02:15 PDT
Created attachment 107188 [details]
Patch
Comment 6 James Robinson 2011-09-13 10:04:15 PDT
Patch slightly updated - the previous version was actually trying to draw from texture id 0 (erk).
Comment 7 James Robinson 2011-09-13 10:22:21 PDT
*** Bug 67144 has been marked as a duplicate of this bug. ***
Comment 8 Stephen White 2011-09-13 12:05:48 PDT
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
Comment 9 Stephen White 2011-09-14 10:46:51 PDT
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.
Comment 10 James Robinson 2011-09-14 10:55:51 PDT
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.
Comment 11 James Robinson 2011-09-14 11:48:06 PDT
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.
Comment 12 James Robinson 2011-09-14 12:09:49 PDT
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.
Comment 13 James Robinson 2011-09-14 13:11:29 PDT
Committed r95114: <http://trac.webkit.org/changeset/95114>