Bug 103077 – REGRESSION(r135486): ASSERT(isMainThread()) hit in LayerTreeRenderer::setActive(bool)

• Component: New Bugs
• Status: RESOLVED FIXED

[Andras Becsi]

The assert has been added in http://trac.webkit.org/changeset/135486 and is hit when starting MiniBrowser:

#0  0x00007ffff7042d9d in WebKit::LayerTreeRenderer::setActive (this=0x7042b0, active=true) at /home/abecsi/devel/git/webkit/Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:662
#1  0x00007ffff7261708 in WebKit::ContentsSGNode::ContentsSGNode (this=0x7fff740b4f30, renderer=...) at /home/abecsi/devel/git/webkit/Source/WebKit2/UIProcess/qt/QtWebPageSGNode.cpp:38
#2  0x00007ffff7261f91 in WebKit::QtWebPageSGNode::setRenderer (this=0x7fff740b4cf0, renderer=...) at /home/abecsi/devel/git/webkit/Source/WebKit2/UIProcess/qt/QtWebPageSGNode.cpp:137
#3  0x00007ffff7241b70 in QQuickWebPage::updatePaintNode (this=0x6dff30, oldNode=0x0) at /home/abecsi/devel/git/webkit/Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:89
#4  0x00007fffeac68576 in QQuickWindowPrivate::updateDirtyNode(QQuickItem*) () from /home/abecsi/devel/qt5/build/qtbase/lib/libQt5Quick.so.5
#5  0x00007fffeac69474 in QQuickWindowPrivate::updateDirtyNodes() () from /home/abecsi/devel/qt5/build/qtbase/lib/libQt5Quick.so.5
#6  0x00007fffeac694d2 in QQuickWindowPrivate::syncSceneGraph() () from /home/abecsi/devel/qt5/build/qtbase/lib/libQt5Quick.so.5
#7  0x00007fffead3fbe9 in QQuickRenderThreadSingleContextWindowManager::run() () from /home/abecsi/devel/qt5/build/qtbase/lib/libQt5Quick.so.5
#8  0x00007fffe86d1c44 in QThreadPrivate::start(void*) () from /home/abecsi/devel/qt5/build/qtbase/lib/libQt5Core.so.5
#9  0x00007fffe810ce0f in start_thread () from /usr/lib/libpthread.so.0
#10 0x00007fffe741f31d in clone () from /usr/lib/libc.so.6

[Dongseong Hwang]

I'm sorry for this crash and thanks for reporting.

I think setActive() was designed that only the main thread calls, because m_active is used in the main thread without locking.
In this time, we can make LayerTreeRenderer more thread-safe.

[Dongseong Hwang]

Created attachment 175714 [details]
Patch

[Dongseong Hwang]

(In reply to comment #2)
> Created an attachment (id=175714) [details]
> Patch

I removed misunderstanding assertions.

setActive() is called from both main thread and painting thread.
It is thread-safe because QQuickWebPage::updatePaintNode locks the main thread before QQuickWebPage::updatePaintNode indirectly calls setActive().

[Dongseong Hwang]

Comment on attachment 175714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175714&action=review

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:551
>  void LayerTreeRenderer::renderNextFrame()

callOnMainThread and LayerTreeRenderer::setActive call this method

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:593
>  void LayerTreeRenderer::purgeGLResources()

QQuickWebPage::updatePaintNode indirectly calls this method.

purgeGLResources() accesses main thread's data structures and painting thread's data structures.
However, it is thread-safe because QQuickWebPage::updatePaintNode locks already.

In my opinion, LayerTreeRenderer hugely depends on QQuickWebPage's locking. If other ports want to use LayerTreeRenderer for threaded compositing, it is a little bit confused. I think we need to refactor later.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:658
>  void LayerTreeRenderer::setActive(bool active)

LayerTreeRenderer::LayerTreeRenderer and QQuickWebPage::updatePaintNode call this method.

[Dongseong Hwang]

Comment on attachment 175714 [details]
Patch

I found some problems yet.

[Dongseong Hwang]

Created attachment 175729 [details]
Patch

[WebKit Review Bot]

Comment on attachment 175729 [details]
Patch

Clearing flags on attachment: 175729

Committed r135616: <http://trac.webkit.org/changeset/135616>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.