Summary: | [skia] fast/canvas/setWidthResetAfterForcedRender is flaky | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||
Component: | New Bugs | Assignee: | James Robinson <jamesr> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bsalomon, enne, kbr, nduca, reed, senorblanco | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
James Robinson
2011-09-12 15:00:49 PDT
Created attachment 107088 [details]
Patch
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. Great find. (unofficially) LGTM. 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+.
Created attachment 107188 [details]
Patch
Patch slightly updated - the previous version was actually trying to draw from texture id 0 (erk). *** Bug 67144 has been marked as a duplicate of this bug. *** 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
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. 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. 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. 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. Committed r95114: <http://trac.webkit.org/changeset/95114> |