Changeset 250717 in webkit


Ignore:
Timestamp:
Oct 4, 2019 6:39:21 AM (5 years ago)
Author:
Carlos Garcia Campos
Message:

[GTK] Crash in WebChromeClient::createDisplayRefreshMonitor
​https://bugs.webkit.org/show_bug.cgi?id=202551

Reviewed by Žan Doberšek.

The crash happens when the drawing area is destroyed due to a page close. The layer tree host is invalidated
causing a layer flush that ends up trying to create a display refresh monitor, which requires the drawing
area. We need to null-check the drawing area in WebChromeClient::createDisplayRefreshMonitor() but we should
also ensure that layer flush is not performed after layer tree host is destroyed.

  • WebProcess/WebCoreSupport/WebChromeClient.cpp:

(WebKit::WebChromeClient::createDisplayRefreshMonitor const): Null-check drawing area before using it.

  • WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:

(WebKit::CompositingCoordinator::CompositingCoordinator): Receive a WebPage instead of a WebCore::Page and
create the root layer here.
(WebKit::CompositingCoordinator::~CompositingCoordinator): Do not purge backing stores again, invalidate should
always be called right before the object is destroyed.
(WebKit::CompositingCoordinator::flushPendingLayerChanges): Get WebCore::Page from WebPage.
(WebKit::CompositingCoordinator::timestamp const): Ditto.
(WebKit::CompositingCoordinator::syncDisplayState): Ditto.
(WebKit::CompositingCoordinator::notifyFlushRequired): Do not continue if m_rootLayer is nullptr.
(WebKit::CompositingCoordinator::deviceScaleFactor const): Get WebCore::Page from WebPage.
(WebKit::CompositingCoordinator::pageScaleFactor const): Ditto.
(WebKit::CompositingCoordinator::createGraphicsLayer): Call attachLayer() instead of duplicating the code.
(WebKit::CompositingCoordinator::setVisibleContentsRect): Get WebCore::Page from WebPage.

  • WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.h:
  • WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp:

(WebKit::DrawingAreaCoordinatedGraphics::discardPreviousLayerTreeHost): Do not call LayerTreeHost::invalidate()
that has been removed.

  • WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp:

(WebKit::LayerTreeHost::LayerTreeHost): Construct the coordinator after the sceneIntegration.
(WebKit::LayerTreeHost::~LayerTreeHost): Invalidate everything here now. We don't really need invalidate()
method since LayerTreeHost is not refcounted and we always called invalidate right before deleting the object.
(WebKit::LayerTreeHost::layerFlushTimerFired): This can't happen on invalid state anymore.

  • WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.h:
Location:
trunk/Source/WebKit
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r250713 r250717  
     12019-10-04  Carlos Garcia Campos  <cgarcia@igalia.com>
     2
     3        [GTK] Crash in WebChromeClient::createDisplayRefreshMonitor
     4        https://bugs.webkit.org/show_bug.cgi?id=202551
     5
     6        Reviewed by Žan Doberšek.
     7
     8        The crash happens when the drawing area is destroyed due to a page close. The layer tree host is invalidated
     9        causing a layer flush that ends up trying to create a display refresh monitor, which requires the drawing
     10        area. We need to null-check the drawing area in WebChromeClient::createDisplayRefreshMonitor() but we should
     11        also ensure that layer flush is not performed after layer tree host is destroyed.
     12
     13        * WebProcess/WebCoreSupport/WebChromeClient.cpp:
     14        (WebKit::WebChromeClient::createDisplayRefreshMonitor const): Null-check drawing area before using it.
     15        * WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:
     16        (WebKit::CompositingCoordinator::CompositingCoordinator): Receive a WebPage instead of a WebCore::Page and
     17        create the root layer here.
     18        (WebKit::CompositingCoordinator::~CompositingCoordinator): Do not purge backing stores again, invalidate should
     19        always be called right before the object is destroyed.
     20        (WebKit::CompositingCoordinator::flushPendingLayerChanges): Get WebCore::Page from WebPage.
     21        (WebKit::CompositingCoordinator::timestamp const): Ditto.
     22        (WebKit::CompositingCoordinator::syncDisplayState): Ditto.
     23        (WebKit::CompositingCoordinator::notifyFlushRequired): Do not continue if m_rootLayer is nullptr.
     24        (WebKit::CompositingCoordinator::deviceScaleFactor const): Get WebCore::Page from WebPage.
     25        (WebKit::CompositingCoordinator::pageScaleFactor const): Ditto.
     26        (WebKit::CompositingCoordinator::createGraphicsLayer): Call attachLayer() instead of duplicating the code.
     27        (WebKit::CompositingCoordinator::setVisibleContentsRect): Get WebCore::Page from WebPage.
     28        * WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.h:
     29        * WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp:
     30        (WebKit::DrawingAreaCoordinatedGraphics::discardPreviousLayerTreeHost): Do not call LayerTreeHost::invalidate()
     31        that has been removed.
     32        * WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp:
     33        (WebKit::LayerTreeHost::LayerTreeHost): Construct the coordinator after the sceneIntegration.
     34        (WebKit::LayerTreeHost::~LayerTreeHost): Invalidate everything here now. We don't really need invalidate()
     35        method since LayerTreeHost is not refcounted and we always called invalidate right before deleting the object.
     36        (WebKit::LayerTreeHost::layerFlushTimerFired): This can't happen on invalid state anymore.
     37        * WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.h:
     38
    1392019-10-04  Carlos Garcia Campos  <cgarcia@igalia.com>
    240

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp

    r249939 r250717  
    892892RefPtr<DisplayRefreshMonitor> WebChromeClient::createDisplayRefreshMonitor(PlatformDisplayID displayID) const
    893893{
    894     return m_page.drawingArea()->createDisplayRefreshMonitor(displayID);
     894    if (auto* drawingArea = m_page.drawingArea())
     895        return drawingArea->createDisplayRefreshMonitor(displayID);
     896    return nullptr;
    895897}
    896898

  • trunk/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp

    r246400 r250717  
    5151using namespace WebCore;
    5252
    53 CompositingCoordinator::CompositingCoordinator(Page* page, CompositingCoordinator::Client& client)
     53CompositingCoordinator::CompositingCoordinator(WebPage& page, CompositingCoordinator::Client& client)
    5454    : m_page(page)
    5555    , m_client(client)
    … …  
    5858    m_nicosia.scene = Nicosia::Scene::create();
    5959    m_state.nicosia.scene = m_nicosia.scene;
     60
     61    m_rootLayer = GraphicsLayer::create(this, *this);
     62#ifndef NDEBUG
     63    m_rootLayer->setName("CompositingCoordinator root layer");
     64#endif
     65    m_rootLayer->setDrawsContent(false);
     66    m_rootLayer->setSize(m_page.size());
    6067}
    6168
    6269CompositingCoordinator::~CompositingCoordinator()
    6370{
    64     m_isDestructing = true;
    65 
    66     purgeBackingStores();
     71    ASSERT(!m_rootLayer);
    6772
    6873    for (auto& registeredLayer : m_registeredLayers.values())
    … …  
    120125        m_overlayCompositingLayer->flushCompositingState(FloatRect(FloatPoint(), m_rootLayer->size()));
    121126
    122     bool didSync = m_page->mainFrame().view()->flushCompositingStateIncludingSubframes();
     127    bool didSync = m_page.corePage()->mainFrame().view()->flushCompositingStateIncludingSubframes();
    123128
    124129    auto& coordinatedLayer = downcast<CoordinatedGraphicsLayer>(*m_rootLayer);
    … …  
    170175double CompositingCoordinator::timestamp() const
    171176{
    172     auto* document = m_page->mainFrame().document();
     177    auto* document = m_page.corePage()->mainFrame().document();
    173178    if (!document)
    174179        return 0;
    … …  
    178183void CompositingCoordinator::syncDisplayState()
    179184{
    180     m_page->mainFrame().view()->updateLayoutAndStyleIfNeededRecursive();
     185    m_page.corePage()->mainFrame().view()->updateLayoutAndStyleIfNeededRecursive();
    181186}
    182187
    … …  
    199204}
    200205
    201 void CompositingCoordinator::createRootLayer(const IntSize& size)
    202 {
    203     ASSERT(!m_rootLayer);
    204     // Create a root layer.
    205     m_rootLayer = GraphicsLayer::create(this, *this);
    206 #ifndef NDEBUG
    207     m_rootLayer->setName("CompositingCoordinator root layer");
    208 #endif
    209     m_rootLayer->setDrawsContent(false);
    210     m_rootLayer->setSize(size);
    211 }
    212 
    213206void CompositingCoordinator::syncLayerState()
    214207{
    … …  
    218211void CompositingCoordinator::notifyFlushRequired(const GraphicsLayer*)
    219212{
    220     if (!m_isDestructing && !isFlushingLayerChanges())
     213    if (m_rootLayer && !isFlushingLayerChanges())
    221214        m_client.notifyFlushRequired();
    222215}
    … …  
    224217float CompositingCoordinator::deviceScaleFactor() const
    225218{
    226     return m_page->deviceScaleFactor();
     219    return m_page.corePage()->deviceScaleFactor();
    227220}
    228221
    229222float CompositingCoordinator::pageScaleFactor() const
    230223{
    231     return m_page->pageScaleFactor();
     224    return m_page.corePage()->pageScaleFactor();
    232225}
    233226
    … …  
    235228{
    236229    auto layer = adoptRef(*new CoordinatedGraphicsLayer(layerType, client));
     230    attachLayer(layer.ptr());
     231    return layer;
     232}
     233
     234FloatRect CompositingCoordinator::visibleContentsRect() const
     235{
     236    return m_visibleContentsRect;
     237}
     238
     239void CompositingCoordinator::setVisibleContentsRect(const FloatRect& rect)
     240{
     241    bool contentsRectDidChange = rect != m_visibleContentsRect;
     242    if (contentsRectDidChange) {
     243        m_visibleContentsRect = rect;
     244
     245        for (auto& registeredLayer : m_registeredLayers.values())
     246            registeredLayer->setNeedsVisibleRectAdjustment();
     247    }
     248
     249    FrameView* view = m_page.corePage()->mainFrame().view();
     250    if (view->useFixedLayout() && contentsRectDidChange) {
     251        // Round the rect instead of enclosing it to make sure that its size stays
     252        // the same while panning. This can have nasty effects on layout.
     253        view->setFixedVisibleContentRect(roundedIntRect(rect));
     254    }
     255}
     256
     257void CompositingCoordinator::deviceOrPageScaleFactorChanged()
     258{
     259    m_rootLayer->deviceOrPageScaleFactorChanged();
     260}
     261
     262void CompositingCoordinator::detachLayer(CoordinatedGraphicsLayer* layer)
     263{
     264    if (m_isPurging)
     265        return;
     266
     267    {
     268        auto& compositionLayer = layer->compositionLayer();
     269        m_nicosia.state.layers.remove(compositionLayer);
     270        compositionLayer->setSceneIntegration(nullptr);
     271    }
     272    m_registeredLayers.remove(layer->id());
     273    notifyFlushRequired(layer);
     274}
     275
     276void CompositingCoordinator::attachLayer(CoordinatedGraphicsLayer* layer)
     277{
    237278    layer->setCoordinator(this);
    238279    {
    … …  
    241282        compositionLayer->setSceneIntegration(m_client.sceneIntegration());
    242283    }
    243     m_registeredLayers.add(layer->id(), layer.ptr());
    244     layer->setNeedsVisibleRectAdjustment();
    245     notifyFlushRequired(layer.ptr());
    246     return layer;
    247 }
    248 
    249 FloatRect CompositingCoordinator::visibleContentsRect() const
    250 {
    251     return m_visibleContentsRect;
    252 }
    253 
    254 void CompositingCoordinator::setVisibleContentsRect(const FloatRect& rect)
    255 {
    256     bool contentsRectDidChange = rect != m_visibleContentsRect;
    257     if (contentsRectDidChange) {
    258         m_visibleContentsRect = rect;
    259 
    260         for (auto& registeredLayer : m_registeredLayers.values())
    261             registeredLayer->setNeedsVisibleRectAdjustment();
    262     }
    263 
    264     FrameView* view = m_page->mainFrame().view();
    265     if (view->useFixedLayout() && contentsRectDidChange) {
    266         // Round the rect instead of enclosing it to make sure that its size stays
    267         // the same while panning. This can have nasty effects on layout.
    268         view->setFixedVisibleContentRect(roundedIntRect(rect));
    269     }
    270 }
    271 
    272 void CompositingCoordinator::deviceOrPageScaleFactorChanged()
    273 {
    274     m_rootLayer->deviceOrPageScaleFactorChanged();
    275 }
    276 
    277 void CompositingCoordinator::detachLayer(CoordinatedGraphicsLayer* layer)
    278 {
    279     if (m_isPurging)
    280         return;
    281 
    282     {
    283         auto& compositionLayer = layer->compositionLayer();
    284         m_nicosia.state.layers.remove(compositionLayer);
    285         compositionLayer->setSceneIntegration(nullptr);
    286     }
    287     m_registeredLayers.remove(layer->id());
    288     notifyFlushRequired(layer);
    289 }
    290 
    291 void CompositingCoordinator::attachLayer(CoordinatedGraphicsLayer* layer)
    292 {
    293     layer->setCoordinator(this);
    294     {
    295         auto& compositionLayer = layer->compositionLayer();
    296         m_nicosia.state.layers.add(compositionLayer);
    297         compositionLayer->setSceneIntegration(m_client.sceneIntegration());
    298     }
    299284    m_registeredLayers.add(layer->id(), layer);
    300285    layer->setNeedsVisibleRectAdjustment();

  • trunk/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.h

    r246400 r250717  
    2525 */
    2626
    27 #ifndef CompositingCoordinator_h
    28 #define CompositingCoordinator_h
     27#pragma once
    2928
    3029#if USE(COORDINATED_GRAPHICS)
    3130
     31#include "WebPage.h"
    3232#include <WebCore/CoordinatedGraphicsLayer.h>
    3333#include <WebCore/CoordinatedGraphicsState.h>
    … …  
    4747class GraphicsContext;
    4848class GraphicsLayer;
    49 class Page;
    5049}
    5150
    … …  
    6564    };
    6665
    67     CompositingCoordinator(WebCore::Page*, CompositingCoordinator::Client&);
     66    CompositingCoordinator(WebPage&, CompositingCoordinator::Client&);
    6867    virtual ~CompositingCoordinator();
    6968
    … …  
    7877    void renderNextFrame();
    7978
    80     void createRootLayer(const WebCore::IntSize&);
    8179    WebCore::GraphicsLayer* rootLayer() const { return m_rootLayer.get(); }
    8280    WebCore::GraphicsLayer* rootCompositingLayer() const { return m_rootCompositingLayer; }
    … …  
    114112    double timestamp() const;
    115113
    116     WebCore::Page* m_page;
     114    WebPage& m_page;
    117115    CompositingCoordinator::Client& m_client;
    118116
    … …  
    132130
    133131    // We don't send the messages related to releasing resources to renderer during purging, because renderer already had removed all resources.
    134     bool m_isDestructing { false };
    135132    bool m_isPurging { false };
    136133    bool m_isFlushingLayerChanges { false };
    … …  
    146143
    147144#endif // namespace WebKit
    148 
    149 #endif // CompositingCoordinator_h

  • trunk/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp

    r250651 r250717  
    8080}
    8181
    82 DrawingAreaCoordinatedGraphics::~DrawingAreaCoordinatedGraphics()
    83 {
    84     discardPreviousLayerTreeHost();
    85     if (m_layerTreeHost)
    86         m_layerTreeHost->invalidate();
    87 }
     82DrawingAreaCoordinatedGraphics::~DrawingAreaCoordinatedGraphics() = default;
    8883
    8984void DrawingAreaCoordinatedGraphics::setNeedsDisplay()
    … …  
    518513{
    519514    m_discardPreviousLayerTreeHostTimer.stop();
    520     if (!m_previousLayerTreeHost)
    521         return;
    522 
    523     m_previousLayerTreeHost->invalidate();
    524515    m_previousLayerTreeHost = nullptr;
    525516}

  • trunk/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp

    r246585 r250717  
    4949LayerTreeHost::LayerTreeHost(WebPage& webPage)
    5050    : m_webPage(webPage)
    51     , m_coordinator(webPage.corePage(), *this)
    5251    , m_compositorClient(*this)
    5352    , m_surface(AcceleratedSurface::create(webPage, *this))
    … …  
    5554    , m_layerFlushTimer(RunLoop::main(), this, &LayerTreeHost::layerFlushTimerFired)
    5655    , m_sceneIntegration(Nicosia::SceneIntegration::create(*this))
     56    , m_coordinator(webPage, *this)
    5757{
    5858#if USE(GLIB_EVENT_LOOP)
    … …  
    6060    m_layerFlushTimer.setName("[WebKit] LayerTreeHost");
    6161#endif
    62     m_coordinator.createRootLayer(m_webPage.size());
    6362    scheduleLayerFlush();
    6463
    … …  
    8584LayerTreeHost::~LayerTreeHost()
    8685{
    87     ASSERT(!m_isValid);
     86    cancelPendingLayerFlush();
     87
     88    m_sceneIntegration->invalidate();
     89    m_coordinator.invalidate();
     90    m_compositor->invalidate();
     91    m_surface = nullptr;
    8892}
    8993
    … …  
    138142    m_webPage.flushPendingEditorStateUpdate();
    139143
    140     if (!m_isValid || !m_coordinator.rootCompositingLayer())
     144    if (!m_coordinator.rootCompositingLayer())
    141145        return;
    142146
    … …  
    163167    m_viewOverlayRootLayer = viewOverlayRootLayer;
    164168    m_coordinator.setViewOverlayRootLayer(viewOverlayRootLayer);
    165 }
    166 
    167 void LayerTreeHost::invalidate()
    168 {
    169     ASSERT(m_isValid);
    170     m_isValid = false;
    171 
    172     cancelPendingLayerFlush();
    173 
    174     m_sceneIntegration->invalidate();
    175     m_coordinator.invalidate();
    176     m_compositor->invalidate();
    177     m_surface = nullptr;
    178169}
    179170

  • trunk/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.h

    r246400 r250717  
    7575    void setRootCompositingLayer(WebCore::GraphicsLayer*);
    7676    void setViewOverlayRootLayer(WebCore::GraphicsLayer*);
    77     void invalidate();
    7877
    7978    void scrollNonCompositedContents(const WebCore::IntRect&);
    … …  
    184183    bool m_notifyAfterScheduledLayerFlush { false };
    185184    bool m_isSuspended { false };
    186     bool m_isValid { true };
    187185    bool m_isWaitingForRenderer { false };
    188186    bool m_scheduledWhileWaitingForRenderer { false };
    … …  
    192190    OptionSet<DiscardableSyncActions> m_discardableSyncActions;
    193191    WebCore::GraphicsLayer* m_viewOverlayRootLayer { nullptr };
    194     CompositingCoordinator m_coordinator;
    195192    CompositorClient m_compositorClient;
    196193    std::unique_ptr<AcceleratedSurface> m_surface;
    … …  
    203200    RunLoop::Timer<LayerTreeHost> m_layerFlushTimer;
    204201    Ref<Nicosia::SceneIntegration> m_sceneIntegration;
     202    CompositingCoordinator m_coordinator;
    205203#endif // USE(COORDINATED_GRAPHICS)
    206204};
Note: See TracChangeset for help on using the changeset viewer.