Bug 195615 – [CoordinatedGraphics] Null dereference in CoordinatedGraphicsLayer::setCoordinatorIncludingSubLayersIfNeeded

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

[CoordinatedGraphics] Null dereference in CoordinatedGraphicsLayer::setCoordinatorIncludingSubLayersIfNeeded

Testing with: GTK port, trunk@242761, Debug build

1) Start MiniBrowser
2) Go to https://youtube.com (to enter AC mode)
3) Right click menu -> Inspect Element
4) Close Web Inspector
5) Go back
6) Wait for five seconds (to destruct LayerTreeHost)
7) Go forward (to YouTube again)
8) Right click menu -> Inspect Element
9) Web Process crashes


> Thread 1 "WebKitWebProces" received signal SIGSEGV, Segmentation fault.
> 0x00007fe61cf21b11 in WebCore::CoordinatedGraphicsLayer::setCoordinatorIncludingSubLayersIfNeeded (
>     this=0x7fe4540ec000, coordinator=0x0)
>     at ../../Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1001
> 1001	    coordinator->attachLayer(this);
> (gdb) bt
> #0  0x00007fe61cf21b11 in WebCore::CoordinatedGraphicsLayer::setCoordinatorIncludingSubLayersIfNeeded (
>     this=0x7fe4540ec000, coordinator=0x0)
>     at ../../Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1001
> #1  0x00007fe61cf1e49f in WebCore::CoordinatedGraphicsLayer::addChild (this=0x7fe592cc0780, layer=...)
>     at ../../Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:174
> #2  0x00007fe61ebcc122 in WebCore::PageOverlayController::installPageOverlay (this=0x7fe6061d4240, 
>     overlay=..., fadeMode=WebCore::PageOverlay::FadeMode::Fade)
>     at ../../Source/WebCore/page/PageOverlayController.cpp:197
> #3  0x00007fe61cdf19a6 in WebKit::WebInspectorClient::highlight (this=0x56006ab2fa00)
>     at ../../Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.cpp:120
> #4  0x00007fe61e88022b in WebCore::InspectorOverlay::update (this=0x7fe6061c0000)
>     at ../../Source/WebCore/inspector/InspectorOverlay.cpp:501
> #5  0x00007fe61e87ff9b in WebCore::InspectorOverlay::highlightNode (this=0x7fe6061c0000, 
>     node=0x7fe4e6c022f0, highlightConfig=...) at ../../Source/WebCore/inspector/InspectorOverlay.cpp:452
> #6  0x00007fe61e8bf5ef in WebCore::InspectorDOMAgent::highlightNode (this=0x7fe6061c1240, errorString=..., 
>     highlightInspectorObject=..., nodeId=0x7ffed290bb3c, objectId=0x0)
>     at ../../Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1275
> #7  0x00007fe60fb251b0 in Inspector::DOMBackendDispatcher::highlightNode (this=0x7fe6061990a0, 
>     requestId=98, parameters=...)
>     at DerivedSources/JavaScriptCore/inspector/InspectorBackendDispatchers.cpp:2331
> #8  0x00007fe60fb1fb8d in Inspector::DOMBackendDispatcher::dispatch (this=0x7fe6061990a0, requestId=98, 
>     method=..., message=...) at DerivedSources/JavaScriptCore/inspector/InspectorBackendDispatchers.cpp:1497
> #9  0x00007fe60fb16255 in Inspector::BackendDispatcher::dispatch (this=0x7fe6061f8160, message=...)
>     at ../../Source/JavaScriptCore/inspector/InspectorBackendDispatcher.cpp:180
> #10 0x00007fe61e861ec2 in WebCore::InspectorController::dispatchMessageFromFrontend (this=0x7fe6061c1000, 
>     message=...) at ../../Source/WebCore/inspector/InspectorController.cpp:415
> #11 0x00007fe61ce86770 in WebKit::WebPageInspectorTarget::sendMessageToTargetBackend (this=0x7fe6061c6008, 
>     message=...) at ../../Source/WebKit/WebProcess/WebPage/WebPageInspectorTarget.cpp:62
> #12 0x00007fe61ce86e56 in WebKit::WebPageInspectorTargetController::sendMessageToTargetBackend (
>     this=0x7fe6061c6000, targetId=..., message=...)
>     at ../../Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetController.cpp:98
> #13 0x00007fe61ce4983a in WebKit::WebPage::sendMessageToTargetBackend (this=0x7fe6061c8000, targetId=..., 
>     message=...) at ../../Source/WebKit/WebProcess/WebPage/WebPage.cpp:2914
> #14 0x00007fe61c397868 in IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&), std::tuple<WTF::String, WTF::String>, 0ul, 1ul> (object=0x7fe6061c8000, 
>     function=
>     (void (WebKit::WebPage::*)(WebKit::WebPage * const, const WTF::String &, const WTF::String &)) 0x7fe61ce497fe <WebKit::WebPage::sendMessageToTargetBackend(WTF::String const&, WTF::String const&)>, args=...)
>     at ../../Source/WebKit/Platform/IPC/HandleMessage.h:41
> #15 0x00007fe61c39295d in IPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&), std::tuple<WTF::String, WTF::String>, std::integer_sequence<unsigned long, 0ul, 1ul> > (args=..., object=0x7fe6061c8000, function=
>     (void (WebKit::WebPage::*)(WebKit::WebPage * const, const WTF::String &, const WTF::String &)) 0x7fe61ce497fe <WebKit::WebPage::sendMessageToTargetBackend(WTF::String const&, WTF::String const&)>)
>     at ../../Source/WebKit/Platform/IPC/HandleMessage.h:47
> #16 0x00007fe61c38666e in IPC::handleMessage<Messages::WebPage::SendMessageToTargetBackend, WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, WTF::String const&)> (decoder=..., object=0x7fe6061c8000, 
>     function=
>     (void (WebKit::WebPage::*)(WebKit::WebPage * const, const WTF::String &, const WTF::String &)) 0x7fe61ce497fe <WebKit::WebPage::sendMessageToTargetBackend(WTF::String const&, WTF::String const&)>)
>     at ../../Source/WebKit/Platform/IPC/HandleMessage.h:147
> #17 0x00007fe61c37c5c2 in WebKit::WebPage::didReceiveWebPageMessage (this=0x7fe6061c8000, connection=..., 
>     decoder=...) at DerivedSources/WebKit/WebPageMessageReceiver.cpp:751
> #18 0x00007fe61ce4e35d in WebKit::WebPage::didReceiveMessage (this=0x7fe6061c8000, connection=..., 
>     decoder=...) at ../../Source/WebKit/WebProcess/WebPage/WebPage.cpp:4331
> #19 0x00007fe61c5e86b3 in IPC::MessageReceiverMap::dispatchMessage (this=0x56006a91e870, connection=..., 
>     decoder=...) at ../../Source/WebKit/Platform/IPC/MessageReceiverMap.cpp:123
> #20 0x00007fe61cb45f3e in WebKit::WebProcess::didReceiveMessage (this=0x56006a91e800, connection=..., 
>     decoder=...) at ../../Source/WebKit/WebProcess/WebProcess.cpp:729
> #21 0x00007fe61c5c48d0 in IPC::Connection::dispatchMessage (this=0x7fe6061e5000, decoder=...)
>     at ../../Source/WebKit/Platform/IPC/Connection.cpp:983
> #22 0x00007fe61c5c4a5e in IPC::Connection::dispatchMessage (this=0x7fe6061e5000, 
>     message=std::unique_ptr<IPC::Decoder> = {...}) at ../../Source/WebKit/Platform/IPC/Connection.cpp:1010
> #23 0x00007fe61c5c4fbb in IPC::Connection::dispatchOneIncomingMessage (this=0x7fe6061e5000)
>     at ../../Source/WebKit/Platform/IPC/Connection.cpp:1079
> #24 0x00007fe61c5c45d7 in IPC::Connection::<lambda()>::operator()(void) (__closure=0x7fe592e860c8)
>     at ../../Source/WebKit/Platform/IPC/Connection.cpp:961
> #25 0x00007fe61c5cc4c2 in WTF::Function<void()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::unique_ptr<IPC::Decoder>)::<lambda()> >::call(void) (this=0x7fe592e860c0)
>     at DerivedSources/ForwardingHeaders/wtf/Function.h:102
> #26 0x00007fe61c1f7894 in WTF::Function<void ()>::operator()() const (this=0x7ffed290cdc0)
>     at DerivedSources/ForwardingHeaders/wtf/Function.h:57
> #27 0x00007fe61035e3c9 in WTF::RunLoop::performWork (this=0x7fe6061f8000)
>     at ../../Source/WTF/wtf/RunLoop.cpp:123
> #28 0x00007fe6103d63ac in WTF::RunLoop::<lambda(gpointer)>::operator()(gpointer) const (__closure=0x0, 
>     userData=0x7fe6061f8000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:68
> #29 0x00007fe6103d63d0 in WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer) ()
>     at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:70
> #30 0x00007fe6103d634c in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::operator()(GSource *, GSourceFunc, gpointer) const (__closure=0x0, source=0x56006a931230, 
>     callback=0x7fe6103d63b3 <WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer)>, userData=0x7fe6061f8000)
>     at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:45
> #31 0x00007fe6103d637c in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::_FUN(GSource *, GSourceFunc, gpointer) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:46
> #32 0x00007fe611f81a58 in g_main_dispatch () at ../../Source/glib-2.58.1/glib/gmain.c:3182
> #33 g_main_context_dispatch () at ../../Source/glib-2.58.1/glib/gmain.c:3847
> #34 0x00007fe611f81e48 in g_main_context_iterate () at ../../Source/glib-2.58.1/glib/gmain.c:3920
> #35 0x00007fe611f82142 in g_main_loop_run () at ../../Source/glib-2.58.1/glib/gmain.c:4116
> #36 0x00007fe6103d68bb in WTF::RunLoop::run () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:96
> #37 0x00007fe61cebabec in WebKit::AuxiliaryProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> (argc=3, 
>     argv=0x7ffed290d1d8) at ../../Source/WebKit/Shared/unix/AuxiliaryProcessMain.h:66
> #38 0x00007fe61ceba30b in WebKit::WebProcessMainUnix (argc=3, argv=0x7ffed290d1d8)
>     at ../../Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:67
> #39 0x0000560068a189d0 in main (argc=3, argv=0x7ffed290d1d8)
>     at ../../Source/WebKit/WebProcess/EntryPoint/unix/WebProcessMain.cpp:52
> (gdb)

[Fujii Hironori]

PageOverlayController::m_viewOverlayRootLayer->m_coordinator is cleared in ~CompositingCoordinator.

[Carlos Garcia Campos]

I think Miguel fixed this, or a very similar crash. Miguel, could you take a look.

[Miguel Gomez]

> Testing with: GTK port, trunk@242761, Debug build
> 
> 1) Start MiniBrowser
> 2) Go to https://youtube.com (to enter AC mode)
> 3) Right click menu -> Inspect Element
> 4) Close Web Inspector
> 5) Go back
> 6) Wait for five seconds (to destruct LayerTreeHost)
> 7) Go forward (to YouTube again)
> 8) Right click menu -> Inspect Element
> 9) Web Process crashes

I couldn't reproduce it exactly with those steps. For some reason opening the inspector and the closing it avoids leaving AC mode when going to the previous page. We may have a different issue there.
Anyway, I was able to reproduce it by
- start minibrowser
- go to http://youtube.com and then back
- wait 5 seconds
- open the inspector

The issue is the same. The overlay layers inside PageOverlayController are persistent and when AC is left, their coordinator is set to null. This is expected, and when they are added back to a tree with a valid coordinator, the code in CoordinatedGraphicsLayer::setCoordinatorIncludingSubLayersIfNeeded() will set the appropriate coordinator again.

The thing is that, before a valid coordinator is set again, a new GraphicsLayer is added as child of the overlay layers (inside PageOverlayController::installPageOverlay), and that causes the crash cause the coordinator is null at that point.

[Miguel Gomez]

Created attachment 364391 [details]
Patch

[EWS Watchlist]

Comment on attachment 364391 [details]
Patch

Attachment 364391 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11480102

New failing tests:
fast/scrolling/ios/overflow-scroll-overlap-2.html

[EWS Watchlist]

Created attachment 364449 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Fujii Hironori]

Comment on attachment 364391 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:981
> +    if (!coordinator || m_coordinator == coordinator)

You don't need the coordinator? Really?

[Miguel Gomez]

(In reply to Fujii Hironori from comment #7)
> Comment on attachment 364391 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364391&action=review
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:981
> > +    if (!coordinator || m_coordinator == coordinator)
> 
> You don't need the coordinator? Really?

You don't need it as long as the CoordinatedGraphicsLayer subtree is out of the main tree (the one with a valid coordinator). When this subtree with null coordinator is added to a tree with a valid one, the valid coordinator will be set for all the layers inside the subtree.

[WebKit Commit Bot]

Comment on attachment 364391 [details]
Patch

Clearing flags on attachment: 364391

Committed r242864: <https://trac.webkit.org/changeset/242864>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48841063>