Bug 92753 – [Qt] Snowshoe desktop crashes when opening a new tab

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

- Run Snowshoe desktop (https://github.com/snowshoe/snowshoe to get the code)
- Open a new tab (either ctrl+t or double clicking on the tab bar).
- The UIProcess crash.

Backtrace :

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff3df2e58 in WebKit::WebLayerTreeRenderer::getBackingStore (this=0x3085a10, id=176) at /home/darktears/dev/troll/webkit/Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:335
335         ASSERT(layer);
(gdb) bt
#0  0x00007ffff3df2e58 in WebKit::WebLayerTreeRenderer::getBackingStore (this=0x3085a10, id=176) at /home/darktears/dev/troll/webkit/Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:335
#1  0x00007ffff3df2ff6 in WebKit::WebLayerTreeRenderer::createTile (this=0x3085a10, layerID=176, tileID=91, scale=1) at /home/darktears/dev/troll/webkit/Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:347
#2  0x00007ffff3d98d93 in WTF::FunctionWrapper<void (WebKit::WebLayerTreeRenderer::*)(unsigned int, int, float)>::operator() (this=0x31b6950, c=0x3085a10, p1=176, p2=91, p3=1) at /home/darktears/dev/troll/webkit/Source/WTF/wtf/Functional.h:233
#3  0x00007ffff3d985e0 in WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::WebLayerTreeRenderer::*)(unsigned int, int, float)>, void (WebKit::WebLayerTreeRenderer*, int, int, float)>::operator()() (this=0x31b6940)
    at /home/darktears/dev/troll/webkit/Source/WTF/wtf/Functional.h:489
#4  0x00007ffff3df424c in WTF::Function<void ()>::operator()() const (this=0x31e9220) at /home/darktears/dev/troll/webkit/Source/WTF/wtf/Functional.h:613
#5  0x00007ffff3df3ac1 in WebKit::WebLayerTreeRenderer::syncRemoteContent (this=0x3085a10) at /home/darktears/dev/troll/webkit/Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:438
#6  0x00007ffff3d6b824 in QQuickWebPage::updatePaintNode (this=0x1f79c10, oldNode=0x31dfba0) at /home/darktears/dev/troll/webkit/Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:91
#7  0x00007ffff3322d0d in QQuickWindowPrivate::updateDirtyNode (this=0x85a0d0, item=0x1f79c10) at items/qquickwindow.cpp:2234
#8  0x00007ffff3321ce3 in QQuickWindowPrivate::updateDirtyNodes (this=0x85a0d0) at items/qquickwindow.cpp:2055
#9  0x00007ffff331a44f in QQuickWindowPrivate::syncSceneGraph (this=0x85a0d0) at items/qquickwindow.cpp:295
#10 0x00007ffff3434472 in QQuickTrivialWindowManager::renderWindow (this=0x841360, window=0x852440) at items/qquickwindowmanager.cpp:1222
#11 0x00007ffff3434888 in QQuickTrivialWindowManager::event (this=0x841360, e=0x30aa270) at items/qquickwindowmanager.cpp:1306
#12 0x00007ffff187cde4 in QApplicationPrivate::notify_helper (this=0x7fb200, receiver=0x841360, e=0x30aa270) at kernel/qapplication.cpp:3594
#13 0x00007ffff187a520 in QApplication::notify (this=0x7fffffff5a30, receiver=0x841360, e=0x30aa270) at kernel/qapplication.cpp:3051
#14 0x00007ffff04f4d56 in QCoreApplication::notifyInternal (this=0x7fffffff5a30, receiver=0x841360, event=0x30aa270) at kernel/qcoreapplication.cpp:725
#15 0x00007ffff04f8b05 in QCoreApplication::sendEvent (receiver=0x841360, event=0x30aa270) at kernel/qcoreapplication.h:211
#16 0x00007ffff04f606c in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x7fb380) at kernel/qcoreapplication.cpp:1325
#17 0x00007ffff04f5a1d in QCoreApplication::sendPostedEvents (receiver=0x0, event_type=0) at kernel/qcoreapplication.cpp:1185
#18 0x00007ffff0566b72 in postEventSourceDispatch (s=0x809240) at kernel/qeventdispatcher_glib.cpp:278
#19 0x00007fffe9929475 in g_main_context_dispatch () from /lib/libglib-2.0.so.0
#20 0x00007fffe99297a8 in ?? () from /lib/libglib-2.0.so.0
#21 0x00007fffe9929864 in g_main_context_iteration () from /lib/libglib-2.0.so.0
#22 0x00007ffff05673d9 in QEventDispatcherGlib::processEvents (this=0x7ff730, flags=...) at kernel/qeventdispatcher_glib.cpp:423
#23 0x00007ffff04f51e3 in QCoreApplication::processEvents (flags=...) at kernel/qcoreapplication.cpp:896
#24 0x00007ffff332507e in QQuickWindowIncubationController::event (this=0x896830, e=0x31c7310) at items/qquickwindow.cpp:100
#25 0x00007ffff187cde4 in QApplicationPrivate::notify_helper (this=0x7fb200, receiver=0x896830, e=0x31c7310) at kernel/qapplication.cpp:3594
#26 0x00007ffff187a520 in QApplication::notify (this=0x7fffffff5a30, receiver=0x896830, e=0x31c7310) at kernel/qapplication.cpp:3051
#27 0x00007ffff04f4d56 in QCoreApplication::notifyInternal (this=0x7fffffff5a30, receiver=0x896830, event=0x31c7310) at kernel/qcoreapplication.cpp:725
#28 0x00007ffff04f8b05 in QCoreApplication::sendEvent (receiver=0x896830, event=0x31c7310) at kernel/qcoreapplication.h:211
#29 0x00007ffff04f606c in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x7fb380) at kernel/qcoreapplication.cpp:1325
#30 0x00007ffff04f5a1d in QCoreApplication::sendPostedEvents (receiver=0x0, event_type=0) at kernel/qcoreapplication.cpp:1185
#31 0x00007ffff0566b72 in postEventSourceDispatch (s=0x809240) at kernel/qeventdispatcher_glib.cpp:278
#32 0x00007fffe9929475 in g_main_context_dispatch () from /lib/libglib-2.0.so.0
#33 0x00007fffe99297a8 in ?? () from /lib/libglib-2.0.so.0
#34 0x00007fffe9929864 in g_main_context_iteration () from /lib/libglib-2.0.so.0
#35 0x00007ffff05673d9 in QEventDispatcherGlib::processEvents (this=0x7ff730, flags=...) at kernel/qeventdispatcher_glib.cpp:423
#36 0x00007ffff04f1d3d in QEventLoop::processEvents (this=0x7fffffff5970, flags=...) at kernel/qeventloop.cpp:135
#37 0x00007ffff04f201f in QEventLoop::exec (this=0x7fffffff5970, flags=...) at kernel/qeventloop.cpp:211
#38 0x00007ffff04f5409 in QCoreApplication::exec () at kernel/qcoreapplication.cpp:977
#39 0x00007ffff0aef9bb in QGuiApplication::exec () at kernel/qguiapplication.cpp:1031
#40 0x00007ffff187a152 in QApplication::exec () at kernel/qapplication.cpp:

[Alexis Menard (darktears)]

Snowshoe opens a blank page with no url set on it. Notice that If I open about:blank instead (or any URL) the crash is not happening.

[Alexis Menard (darktears)]

(In reply to comment #1)
> Snowshoe opens a blank page with no url set on it. Notice that If I open about:blank instead (or any URL) the crash is not happening.

You can modify PageWidget.qml of snowshoe to force opening another URL.

[Jesus Sanchez-Palencia]

(In reply to comment #0)
> - Run Snowshoe desktop (https://github.com/snowshoe/snowshoe to get the code)
> - Open a new tab (either ctrl+t or double clicking on the tab bar).
> - The UIProcess crash.

Some additional info: 

- with Qt Release it only crashes with WebKit Release.
- with Qt Debug it crashes with both WebKit Release and Debug.

[Jesus Sanchez-Palencia]

This issue persists even after the roll out of https://bugs.webkit.org/show_bug.cgi?id=93077 , so I'll brief you guys a bit after I had a closer look into this. I will be over-explicative just because I'm not familiar with this code path and I will this as an exercise. :)

As Alexis pointed out, the problem is the ASSERT(layer) we are reaching at Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:335 . This is caused because when LayerTreeCoordinatorProxy::setCompositingLayerChildren is called for the new tab, the WebLayerTreeRenderer is not active yet. Therefore, the WebLayerTreeRenderer::appendUpdate calls don't do anything and setLayerChildren calls aren't queued to be dispatched by the next WebLayerTreeRenderer::syncRemoteContent.

This is all happening on the UIProcess side and the reason why we don't see this crash when we first start snowshoe is because when the first tab is created the following call stack happens before any LayerTreeCoordinatorProxy or WebLayerTreeRenderer calls:

QQuickWebPage::updatePaintNode
WebKit::QtWebPageSGNode::setRenderer

setRenderer creates a new ContentsSGNode that makes the renderer active.

When investigating why the WebProcess is sending messages to the UIProcess, triggering LayerTreeCoordinatorProxy::setCompositingLayerChildren even when the UIProcess is clearly not ready yet (no QtWebPageSGNode::setRenderer calls), I realized that the following call flow was happening on the WebProcess side:

WebKit::DrawingAreaImpl::sendDidUpdateBackingStoreState() 
WebKit::LayerTreeCoordinator::forceRepaint() 
WebKit::LayerTreeCoordinator::flushPendingLayerChanges()

This last function, flushPendingLayerChanges, is the one sending messages from the WebProcess to the UIProcess.

Reading LayerTreeCoordinator made me realize that perhaps we are missing a check to m_waitingForUIProcess somewhere, like the one in LayerTreeCoordinator::performScheduledLayerFlush. In fact, adding an early return to LayerTreeCoordinator::forceRepaint() fixes the crash. I haven't uploaded a patch for this because it feels wrong to early return like that in forceRepaint since the comments on this function say we use it exactly when "we are not waiting for a UIProcess to reply nicely."...

What do you guys think?

[Jocelyn Turcotte]

The inconsistent state causing a missing layer might be caused by "if (!m_isActive) return;" at the beginning of WebLayerTreeRenderer::appendUpdate that will drop part of the updates but leave the web process thinking that this part of the graphics layer tree is in sync already.

I'm starting to think that forceRepaint should just not highjack m_waitingForUIProcess. We should probably fix tst_qrawwebview.cpp to make sure that the web process is in the right state before calling it, and that forceRepaintAsync is implemented in DrawingAreaImpl for our purpose.

I thought about it and all other solutions I had felt hackish and don't make sense given that forceRepaint isn't only called by tests.

[Balazs Kelemen]

Created attachment 156946 [details]
proposed fix

Pixel tests works fine with this (after applying from bug 90394), I hope it also fix the bug.

[Jesus Sanchez-Palencia]

Comment on attachment 156946 [details]
proposed fix

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

Cool! LGTM.

> Source/WebKit2/ChangeLog:14
> +        could be implemented for the non conposited path in DrawingAreaImpl

I guess you meant "composited" here.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:181
>      scheduleLayerFlush();

Why do we still need to scheduleLayerFlush()?

[Balazs Kelemen]

(In reply to comment #7)
> (From update of attachment 156946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156946&action=review
> 
> Cool! LGTM.
> 
> > Source/WebKit2/ChangeLog:14
> > +        could be implemented for the non conposited path in DrawingAreaImpl
> 
> I guess you meant "composited" here.
> 

Yep.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:181
> >      scheduleLayerFlush();
> 
> Why do we still need to scheduleLayerFlush()?

Read the comment above :)

[Jocelyn Turcotte]

Comment on attachment 156946 [details]
proposed fix

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:-180
> -    // This is necessary for running layout tests. Since in this case we are not waiting for a UIProcess to reply nicely.
> -    // Instead we are just triggering forceRepaint. But we still want to have the scripted animation callbacks being executed.
> -    syncDisplayState();
> -

You'll need to keep this if you agree with the comment below.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:183
> -    flushPendingLayerChanges();
> +
> +    performScheduledLayerFlush();

Please move the m_waitingForUIProcess check at the top of flushPendingLayerChange instead to make sure that we keep the behavior relatively to m_isSuspended and didPerformScheduledLayerFlush().
It will also make sure that we check this variable in the same method that we reset it.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:192
> +bool LayerTreeCoordinator::forceRepaintAsync(uint64_t callbackID)
> +{
> +    // We expect the UI process to not require a new repaint until the previous one has finished.
> +    ASSERT(!m_forceRepaintAsyncCallbackID);
> +    m_forceRepaintAsyncCallbackID = callbackID;
> +    return true;
>  }

You're missing a scheduleLayerFlush() in here somewhere, no?
Also, I would prefer safely doing this before assigning m_forceRepaintAsyncCallbackID and comment that this is just a clumsy safety measure instead of asserting:

if (m_forceRepaintAsyncCallbackID)
    m_webPage->send(Messages::WebPageProxy::VoidCallback(m_forceRepaintAsyncCallbackID));

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:206
> +    if (!m_layerTreeHost) {

Place this at the beginning, you don't want to invalidate and layout if you're not going to do anything with it here.

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:207
> +        // FIXME: this could be implemented for the non composited path as well.

It's a FIXME that might never be fixed and we have no need for it now, I think it would be better to avoid it.

[Noam Rosenthal]

Comment on attachment 156946 [details]
proposed fix

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

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:205
> +    m_webPage->layoutIfNeeded();

This doesn't seem right here. We layout anyway when we perform the layer flush.

[Balazs Kelemen]

(In reply to comment #9)
> (From update of attachment 156946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156946&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:-180
> > -    // This is necessary for running layout tests. Since in this case we are not waiting for a UIProcess to reply nicely.
> > -    // Instead we are just triggering forceRepaint. But we still want to have the scripted animation callbacks being executed.
> > -    syncDisplayState();
> > -
> 
> You'll need to keep this if you agree with the comment below.

But from now we will use forceSyncAsync for testing, and it does a syncDisplayState before sending back the reply (in performScheduledLayerFlush after the timer fires), so I think we can remove it from forceRepaint.

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:183
> > -    flushPendingLayerChanges();
> > +
> > +    performScheduledLayerFlush();
> 
> Please move the m_waitingForUIProcess check at the top of flushPendingLayerChange instead to make sure that we keep the behavior relatively to m_isSuspended and didPerformScheduledLayerFlush().
> It will also make sure that we check this variable in the same method that we reset it.
> 

Ok.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:192
> > +bool LayerTreeCoordinator::forceRepaintAsync(uint64_t callbackID)
> > +{
> > +    // We expect the UI process to not require a new repaint until the previous one has finished.
> > +    ASSERT(!m_forceRepaintAsyncCallbackID);
> > +    m_forceRepaintAsyncCallbackID = callbackID;
> > +    return true;
> >  }
> 
> You're missing a scheduleLayerFlush() in here somewhere, no?

Yes.

> Also, I would prefer safely doing this before assigning m_forceRepaintAsyncCallbackID and comment that this is just a clumsy safety measure instead of asserting:
> 
> if (m_forceRepaintAsyncCallbackID)
>     m_webPage->send(Messages::WebPageProxy::VoidCallback(m_forceRepaintAsyncCallbackID));

Sending back the message without actually performing the repaint? I don't think that makes sense.

> 
> > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:206
> > +    if (!m_layerTreeHost) {
> 
> Place this at the beginning, you don't want to invalidate and layout if you're not going to do anything with it here.

Yep.

> 
> > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:207
> > +        // FIXME: this could be implemented for the non composited path as well.
> 
> It's a FIXME that might never be fixed and we have no need for it now, I think it would be better to avoid it.

Ok.

[Balazs Kelemen]

(In reply to comment #10)
> (From update of attachment 156946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156946&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:205
> > +    m_webPage->layoutIfNeeded();
> 
> This doesn't seem right here. We layout anyway when we perform the layer flush.

Correct.

[Balazs Kelemen]

Created attachment 157147 [details]
Patch

[Jocelyn Turcotte]

(In reply to comment #11)
> But from now we will use forceSyncAsync for testing, and it does a syncDisplayState before sending back the reply (in performScheduledLayerFlush after the timer fires), so I think we can remove it from forceRepaint.

forceRepaintAsync will be called by tst_qrawwebview, but this line was needed to handle layoutTestController.display(), which ends up calling forceRepaint, not forceRepaintAsync. There shouldn't be a forceRepaint "for testing" and the other not, unless their name distinguish them accordingly.

> Sending back the message without actually performing the repaint? I don't think that makes sense.

Ignoring the callback and make any code that was waiting for it to deadlock isn't better, but it's an edge case and I guess we can see if it asserts somewhere first.

[Balazs Kelemen]

(In reply to comment #14)
> (In reply to comment #11)
> > But from now we will use forceSyncAsync for testing, and it does a syncDisplayState before sending back the reply (in performScheduledLayerFlush after the timer fires), so I think we can remove it from forceRepaint.
> 
> forceRepaintAsync will be called by tst_qrawwebview, but this line was needed to handle layoutTestController.display(), which ends up calling forceRepaint, not forceRepaintAsync. There shouldn't be a forceRepaint "for testing" and the other not, unless their name distinguish them accordingly.

WebPage::forceRepaint first calls drawingArea->forceRepaintAsync and falls back to the sync path if async returned false, like the default implementation. So in our case layoutTestController.display() will also use the new async method.

[Jocelyn Turcotte]

(In reply to comment #15)
> WebPage::forceRepaint first calls drawingArea->forceRepaintAsync and falls back to the sync path if async returned false, like the default implementation. So in our case layoutTestController.display() will also use the new async method.

display() calls WKBundlePageForceRepaint which then calls WebPage::forceRepaintWithoutCallback(), not forceRepaint().
My argument is the same regardless, those methods should behave exactly the same except that one is async and not the other. Doing otherwise would break encapsulation and encourage layer violation.

[Balazs Kelemen]

Created attachment 157203 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 157203 [details]
Patch

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

> Source/WebKit2/ChangeLog:12
> +        done by LayerTreeCoordinator that holds the callback and send the

and send*s*

> Source/WebKit2/ChangeLog:19
> +        (WebKit::LayerTreeCoordinator::forceRepaint):

Not sure if it's a glitch, if not just make sure that you regenerate the changelog before pushing.

[Balazs Kelemen]

Comment on attachment 157203 [details]
Patch

Landed in http://trac.webkit.org/changeset/125034