RESOLVED FIXED 103410
[Texmap] REGRESSION(r135620) QtTestBrowser crashes on Google-gravity. 
https://bugs.webkit.org/show_bug.cgi?id=103410
Summary [Texmap] REGRESSION(r135620) QtTestBrowser crashes on Google-gravity.
Zeno Albisser
Reported 2012-11-27 07:42:36 PST
To reproduce start "QtTestBrowser -graphicsbased" and visit the following page: http://mrdoob.com/projects/chromeexperiments/google-gravity/. The problem seems to be related to TextureMapper / GraphicsLayerTextureMapper creation. Either we end up having multiple root layers, or a root layer without a TextureMapper (possibly due to a race condition). I will investigate further and post my findings.
Attachments
Backtrace (6.89 KB, text/plain)
2012-11-27 08:28 PST, Zeno Albisser 		no flags
Details
Patch : GraphicsLayerTextureMapper::updateBackingStore() early returns until TextureMapper is set. (3.51 KB, patch)
2012-11-27 21:11 PST, Dongseong Hwang 		noam: review+
noam: commit-queue-
DetailsFormatted DiffDiff
Patch : TextureMapperLayerClientQt::setTextureMapper() must call flushCompositingStateForThisLayerOnly() of the root layer. (2.37 KB, patch)
2012-11-27 21:13 PST, Dongseong Hwang 		noam: review+
noam: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Simon Hausmann
Comment 1 2012-11-27 07:48:12 PST
Note to No'am and Huang: assert(textureMapper) in GraphicsLayerTextureMapper::updateBackingStore is what's failing after the change.
Zeno Albisser
Comment 2 2012-11-27 08:25:59 PST
minimal example to reproduce the problem: <html><body> <div style="-webkit-transform: translateZ(0px);">hello</div> </body></html>
Zeno Albisser
Comment 3 2012-11-27 08:28:07 PST
Created attachment 176272 [details] Backtrace
Dongseong Hwang
Comment 4 2012-11-27 18:00:53 PST
After I read code again, I understand GraphicsLayerTextureMapper::updateBackingStore has a problem. GraphicsLayer::FlushCompositingState can be called by RenderLayerBacking. It means this method can be called before creating TextureMapper. So TextureMapperLayer::flushCompositingState() checks and if textureMapper does not exist, do nothing. void TextureMapperLayer::flushCompositingState(GraphicsLayerTextureMapper* graphicsLayer, TextureMapper* textureMapper, int options) { if (!textureMapper) return; ... } But GraphicsLayerTextureMapper::updateBackingStore expects textureMapper always exists. Bug 103366 can fix this bug, but I think it is right that this bug fixes hitting assertion and then reviews Bug 103366. If we change code like following code, this bug can be fixed. In the detail, we must keep m_needsDisplay until GraphicsLayerTextureMapper::prepareBackingStore() is completed, so m_needsDisplay should be removed in GraphicsLayerTextureMapper::didFlushCompositingState(). I'm glad anyone to fix it. If you want, I can do it of course. @@ -404,8 +410,6 @@ void GraphicsLayerTextureMapper::didFlushCompositingState() { updateBackingStore(); m_changeMask = 0; - m_needsDisplay = false; - m_needsDisplayRect = IntRect(); } void GraphicsLayerTextureMapper::didFlushCompositingStateRecursive() @@ -430,6 +434,10 @@ void GraphicsLayerTextureMapper::updateBackingStore() void GraphicsLayerTextureMapper::prepareBackingStore() { + TextureMapper* textureMapper = m_layer->textureMapper(); + if (!textureMapper) + return; + if (!shouldHaveBackingStore()) { m_backingStore.clear(); return; @@ -441,9 +449,6 @@ void GraphicsLayerTextureMapper::prepareBackingStore() if (dirtyRect.isEmpty()) return; - TextureMapper* textureMapper = m_layer->textureMapper(); - ASSERT(textureMapper); - if (!m_backingStore) m_backingStore = TextureMapperTiledBackingStore::create();
Noam Rosenthal
Comment 5 2012-11-27 18:07:36 PST
> I'm glad anyone to fix it. If you want, I can do it of course. Yes, if you could fix this before posting more refactor patches it would be great :)
Dongseong Hwang
Comment 6 2012-11-27 20:25:52 PST
(In reply to comment #5) > > I'm glad anyone to fix it. If you want, I can do it of course. > Yes, if you could fix this before posting more refactor patches it would be great :) My explanation can not fix this bug yet. "QtTestBrowser -graphicsbased" can not draw minimal text in #c2. I'm also investigating why.
Dongseong Hwang
Comment 7 2012-11-27 21:11:41 PST
Created attachment 176391 [details] Patch : GraphicsLayerTextureMapper::updateBackingStore() early returns until TextureMapper is set.
Dongseong Hwang
Comment 8 2012-11-27 21:13:50 PST
Created attachment 176392 [details] Patch : TextureMapperLayerClientQt::setTextureMapper() must call flushCompositingStateForThisLayerOnly() of the root layer.
Noam Rosenthal
Comment 9 2012-11-27 22:38:47 PST
Comment on attachment 176391 [details] Patch : GraphicsLayerTextureMapper::updateBackingStore() early returns until TextureMapper is set. View in context: https://bugs.webkit.org/attachment.cgi?id=176391&action=review LGTM with changelog rewordings. > Source/WebCore/ChangeLog:8 > + GraphicsLayerTextureMapper::updateBackingStore() early returns until TextureMapper is set. early returns until -> should return early before > Source/WebCore/ChangeLog:12 > + TextureMapperLayer::flushCompositingState() checks and early returns if returns early > Source/WebCore/ChangeLog:13 > + TextureMapper does not exist. does not exist -> was not created > Source/WebCore/ChangeLog:17 > + However, GraphicsLayerTextureMapper::updateBackingStore() expects that TextureMapper > + always exists. This patch makes updateBackingStore() early return like > + TextureMapperLayer::flushCompositingState(). updateBackingStore should also return early when TextureMapper was not created.
Noam Rosenthal
Comment 10 2012-11-27 22:41:33 PST
Comment on attachment 176392 [details] Patch : TextureMapperLayerClientQt::setTextureMapper() must call flushCompositingStateForThisLayerOnly() of the root layer. View in context: https://bugs.webkit.org/attachment.cgi?id=176392&action=review LGTM with ChangeLog rewording. > Source/WebKit/qt/ChangeLog:16 > + Currently, PageClientQGraphicsWidget::setRootGraphicsLayer does not flush layer > + states of the root layer after setting TextureMapper while > + PageClientQWidget::setRootGraphicsLayer() flushes them of the root layer. If > + not flushing states, descendant layers can not reach the root layer. It means > + descendant layers can not reach the TextureMapper that the root layer has. > + This is already the behavior for PageClientQWidget, which should be the case for PageClientQGraphicsWidget as well.
Simon Hausmann
Comment 11 2012-11-27 23:46:01 PST
Thanks Huang and No'am! We're trying to cut a release of the Qt port and branch this weekend. I appreciate how quickly you responded. Thanks a lot! :) We can test the patches during the course of today also and land them with the suggested changes if they work, unless somebody is quicker :)
Zeno Albisser
Comment 12 2012-11-28 01:28:31 PST
LGTM :-) I tried both patches, and they seem to reliably resolve the issue. Thank you very much for looking into it so quickly!
Zeno Albisser
Comment 13 2012-11-28 01:46:55 PST
Comment on attachment 176391 [details] Patch : GraphicsLayerTextureMapper::updateBackingStore() early returns until TextureMapper is set. Committed r135986.
Zeno Albisser
Comment 14 2012-11-28 01:47:22 PST
Comment on attachment 176392 [details] Patch : TextureMapperLayerClientQt::setTextureMapper() must call flushCompositingStateForThisLayerOnly() of the root layer. Committed r135987.
Zeno Albisser
Comment 15 2012-11-28 01:47:57 PST
All reviewed patches have been landed. Closing bug.
Dongseong Hwang
Comment 16 2012-11-28 04:53:31 PST
Thanks for review and manual patch and good advice!!
Noam Rosenthal
Comment 17 2012-11-28 08:13:04 PST
> We're trying to cut a release of the Qt port and branch this weekend. I appreciate how quickly you responded. Thanks a lot! :) No problem Simon, let me know if anything in coordinated graphics is blocking your release and I'll respond as quickly as I can.
Note You need to log in before you can comment on or make changes to this bug.