Bug 196825 - REGRESSION (r244182): RemoteLayerTreeDrawingArea::flushLayers() should not be reentrant
Summary: REGRESSION (r244182): RemoteLayerTreeDrawingArea::flushLayers() should not be...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-11 12:25 PDT by Said Abou-Hallawa
Modified: 2019-04-11 15:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.44 KB, patch)
2019-04-11 12:31 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (2.85 KB, patch)
2019-04-11 14:18 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (2.60 KB, patch)
2019-04-11 14:29 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2019-04-11 12:25:49 PDT
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.
Comment 1 Said Abou-Hallawa 2019-04-11 12:31:22 PDT
Created attachment 367233 [details]
Patch
Comment 2 Tim Horton 2019-04-11 13:11:40 PDT
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?
Comment 3 Simon Fraser (smfr) 2019-04-11 13:54:16 PDT
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?
Comment 4 Said Abou-Hallawa 2019-04-11 14:18:28 PDT
Created attachment 367239 [details]
Patch
Comment 5 Said Abou-Hallawa 2019-04-11 14:22:33 PDT
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.
Comment 6 Said Abou-Hallawa 2019-04-11 14:29:21 PDT
Created attachment 367240 [details]
Patch
Comment 7 WebKit Commit Bot 2019-04-11 15:51:49 PDT
Comment on attachment 367240 [details]
Patch

Clearing flags on attachment: 367240

Committed r244198: <https://trac.webkit.org/changeset/244198>
Comment 8 WebKit Commit Bot 2019-04-11 15:51:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-04-11 15:52:19 PDT
<rdar://problem/49831390>