Bug 103410

Summary:

[Texmap] REGRESSION(r135620) QtTestBrowser crashes on Google-gravity.

Product:

WebKit

Reporter:

Zeno Albisser <zeno>

Component:

WebKit Qt

Assignee:

Dongseong Hwang <dongseong.hwang>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dongseong.hwang, hausmann, noam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Backtrace	none
Patch : GraphicsLayerTextureMapper::updateBackingStore() early returns until TextureMapper is set.	noam: review+, noam: commit-queue-
Patch : TextureMapperLayerClientQt::setTextureMapper() must call flushCompositingStateForThisLayerOnly() of the root layer.	noam: review+, noam: commit-queue-

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-	Details Formatted Diff Diff
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-	Details Formatted Diff Diff
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.