REGRESSION (r244182): RemoteLayerTreeDrawingArea::flushLayers() should not be reentrant

• Component: Animations
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

After r244182, RemoteLayerTreeDrawingArea::flushLayers() can be reentrant when running run-webkit-tests. This can happen when notifyDone() is called from the rAF callback which forces repaint. Here is the problematic call stack:

3   0x1033b3acd WebKit::RemoteLayerTreeDrawingArea::flushLayers()
4   0x1033b64be WebKit::RemoteLayerTreeDrawingArea::forceRepaint()
5   0x104462f85 WebKit::WebPage::forceRepaintWithoutCallback()
6   0x10413ddbd WKBundlePageForceRepaint
7   0x4ec12346e WTR::InjectedBundlePage::dump()
8   0x4ec146afd WTR::TestRunner::notifyDone()
9   0x4ec1390a7 WTR::JSTestRunner::notifyDone(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**)
10  0x4cb554d51 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*)
11  0x23a354601027
12  0x4cb4d18b1 llint_entry
13  0x4cb4d18b1 llint_entry
14  0x4cb4d18b1 llint_entry
15  0x4cb4be500 vmEntryToJavaScript
16  0x4cbe3dace JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
17  0x4cbe3e0ff JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
18  0x4cc115c4c JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
19  0x4cc115d3a JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
20  0x4cc11602e JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
21  0x4d0ec3fdb WebCore::JSExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
22  0x4d0ec3e8f WebCore::JSCallbackData::invokeCallback(WebCore::JSDOMGlobalObject&, JSC::JSObject*, JSC::JSValue, JSC::MarkedArgumentBuffer&, WebCore::JSCallbackData::CallbackType, JSC::PropertyName, WTF::NakedPtr<JSC::Exception>&)
23  0x4cf65d332 WebCore::JSCallbackDataStrong::invokeCallback(JSC::JSValue, JSC::MarkedArgumentBuffer&, WebCore::JSCallbackData::CallbackType, JSC::PropertyName, WTF::NakedPtr<JSC::Exception>&)
24  0x4d020deb9 WebCore::JSRequestAnimationFrameCallback::handleEvent(double)
25  0x4d15d8344 WebCore::ScriptedAnimationController::serviceRequestAnimationFrameCallbacks(double)
26  0x4d14180d6 WebCore::Document::serviceRequestAnimationFrameCallbacks(double)
27  0x4d1f29387 WebCore::Page::updateRendering()
28  0x1044642d4 WebKit::WebPage::updateRendering()
29  0x1033b3ae9 WebKit::RemoteLayerTreeDrawingArea::flushLayers()
30  0x1033bcf91 WTF::Function<void ()>::CallableWrapper<std::__1::__bind<void (WebKit::RemoteLayerTreeDrawingArea::*&)(), WebKit::RemoteLayerTreeDrawingArea*> >::call()
31  0x1032f665d WTF::Function<void ()>::operator()() const

This call stack was caught by the iOS simulator layout tests because RemoteLayerTreeDrawingAreaProxy::commitLayerTree() asserts the transition IDs are sequential.

[Said Abou-Hallawa]

Created attachment 367233 [details]
Patch

[Tim Horton]

Comment on attachment 367233 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3659
> +void WebPage::scheduleCompositingLayerFlush()

I don't think you need this

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:350
> +        m_webPage.scheduleCompositingLayerFlush();

Why doesn't this just call scheduleCOmpositingLayerFlush like the above?

[Simon Fraser (smfr)]

Comment on attachment 367233 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        detected. But scheduleCompositingLayerFlush() to make a final repaint.

But that will be async, which breaks testing. Why bother scheduling it at all?

>> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:350
>> +        m_webPage.scheduleCompositingLayerFlush();
> 
> Why doesn't this just call scheduleCOmpositingLayerFlush like the above?

Is it really necessary?

[Said Abou-Hallawa]

Created attachment 367239 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 367233 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:350
>>> +        m_webPage.scheduleCompositingLayerFlush();
>> 
>> Why doesn't this just call scheduleCOmpositingLayerFlush like the above?
> 
> Is it really necessary?

It will not be necessary if notifyDone() is the only reason to re-enter this function. I will give it a try and upload another patch.

[Said Abou-Hallawa]

Created attachment 367240 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367240 [details]
Patch

Clearing flags on attachment: 367240

Committed r244198: <https://trac.webkit.org/changeset/244198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49831390>