Changeset 230529 in webkit


Ignore:
Timestamp:
Apr 11, 2018 9:58:23 AM (6 years ago)
Author:
Michael Catanzaro
Message:

[GTK] WaylandCompositorDisplay leaks its wl_display
​https://bugs.webkit.org/show_bug.cgi?id=184406

Reviewed by Carlos Garcia Campos.

Source/WebCore:

Well, this was harder than expected. We really just want to fix a small leak in the WebKit
layer, but that requires a change in how WaylandCompositorDisplay calls the
PlatformDisplayWayland constructor, to pass NativeDisplayOwned::Yes. That means
WaylandCompositorDisplay can no longer use PlatformDisplayWayland's protected default
constructor. Problem is that the normal PlatformDisplayWayland constructor calls
PlatformDisplayWayland::initialize, which calls PlatformDisplayWayland::registryGlobal,
which is a virtual function. The WaylandCompositorDisplay portion of the object is not
constructed yet at this point, so WaylandCompositorDisplay::registryGlobal will never be
called if we do that. I had to revert the previous version of this fix due to this problem.
It had broken accelerated compositing.

I'm reminded of Effective C++ item #9: Never call virtual functions during construction or
destruction ("because such calls will never go to a more derived class than that of the
currently executing constructor or destructor"). This code is fragile and likely to break
again in the future, so let's refactor it a bit. Instead of calling initialize in the
constructor, we'll call it from create functions. We'll have to add a couple create
functions, and make the constructor protected to ensure it's not possible to create a
PlatformDisplayWayland without initializing it. For good parallelism, do the same for the
other PlatformDisplay classes.

This commit additionally removes PlatformDisplayWayland's protected default constructor,
since it's not needed anymore.

The NativeDisplayOwned arguments to the PlatformDisplay constructors are now mandatory,
instead of using NativeDisplayOwned::No as the default value, since that was dangerously
close to being the cause of this leak, and the constructors are now accessed from private
create functions anyway. Some more caution when using default parameter values is warranted
in the future.

Lastly, since we have to change PlatformDisplay::createPlatformDisplay to use the new create
functions, take the opportunity to move things around a bit for clarity. There should be no
change in behavior. I was just disappointed that the PlatformDisplayWPE creation was at the
bottom of the function, after a comment indicating that normal display creation has failed,
which is not the case for WPE.

This all might have been a bit overkill, since the leak could probably have been fixed by
passing nullptr to the PlatformDisplayWayland constructor for the wl_display and not
removing WaylandCompositorDisplay's call to PlatformDisplayWayland::initialize. But the
correctness of that code would then rely on implementation details of initialize, so this
refactor seems better.

No new tests since there *should* be no behavior change. Then again, I'm touching
PlatformDisplay, and history shows we don't have the greatest track record of touching this
code without introducing problems.

  • platform/graphics/PlatformDisplay.cpp:

(WebCore::PlatformDisplay::createPlatformDisplay):

  • platform/graphics/PlatformDisplay.h:
  • platform/graphics/wayland/PlatformDisplayWayland.cpp:

(WebCore::PlatformDisplayWayland::create):
(WebCore::PlatformDisplayWayland::create):
(WebCore::PlatformDisplayWayland::createHeadless):
(WebCore::PlatformDisplayWayland::PlatformDisplayWayland):
(WebCore::PlatformDisplayWayland::initialize):

  • platform/graphics/wayland/PlatformDisplayWayland.h:
  • platform/graphics/win/PlatformDisplayWin.h:
  • platform/graphics/wpe/PlatformDisplayWPE.cpp:

(WebCore::create):

  • platform/graphics/wpe/PlatformDisplayWPE.h:
  • platform/graphics/x11/PlatformDisplayX11.cpp:

(WebCore::PlatformDisplayX11::create):
(WebCore::PlatformDisplayX11::create):
(WebCore::PlatformDisplayX11::createHeadless):

  • platform/graphics/x11/PlatformDisplayX11.h:

Source/WebKit:

Since we allocate our own wl_display here, need to chain up to the parent constructor
passing NativeDisplayOwned::Yes, or it won't ever be released. Move the initialize call to
the create function to ensure it's called after the constructor completes.

  • WebProcess/gtk/WaylandCompositorDisplay.cpp:

(WebKit::WaylandCompositorDisplay::create): Fix a log message (drive-by).
(WebKit::WaylandCompositorDisplay::WaylandCompositorDisplay):

Location:
trunk/Source
Files:
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r230524 r230529  
     12018-04-11  Michael Catanzaro  <mcatanzaro@igalia.com>
     2
     3        [GTK] WaylandCompositorDisplay leaks its wl_display
     4        https://bugs.webkit.org/show_bug.cgi?id=184406
     5
     6        Reviewed by Carlos Garcia Campos.
     7
     8        Well, this was harder than expected. We really just want to fix a small leak in the WebKit
     9        layer, but that requires a change in how WaylandCompositorDisplay calls the
     10        PlatformDisplayWayland constructor, to pass NativeDisplayOwned::Yes. That means
     11        WaylandCompositorDisplay can no longer use PlatformDisplayWayland's protected default
     12        constructor. Problem is that the normal PlatformDisplayWayland constructor calls
     13        PlatformDisplayWayland::initialize, which calls PlatformDisplayWayland::registryGlobal,
     14        which is a virtual function. The WaylandCompositorDisplay portion of the object is not
     15        constructed yet at this point, so WaylandCompositorDisplay::registryGlobal will never be
     16        called if we do that. I had to revert the previous version of this fix due to this problem.
     17        It had broken accelerated compositing.
     18
     19        I'm reminded of Effective C++ item #9: Never call virtual functions during construction or
     20        destruction ("because such calls will never go to a more derived class than that of the
     21        currently executing constructor or destructor"). This code is fragile and likely to break
     22        again in the future, so let's refactor it a bit. Instead of calling initialize in the
     23        constructor, we'll call it from create functions. We'll have to add a couple create
     24        functions, and make the constructor protected to ensure it's not possible to create a
     25        PlatformDisplayWayland without initializing it. For good parallelism, do the same for the
     26        other PlatformDisplay classes.
     27
     28        This commit additionally removes PlatformDisplayWayland's protected default constructor,
     29        since it's not needed anymore.
     30
     31        The NativeDisplayOwned arguments to the PlatformDisplay constructors are now mandatory,
     32        instead of using NativeDisplayOwned::No as the default value, since that was dangerously
     33        close to being the cause of this leak, and the constructors are now accessed from private
     34        create functions anyway. Some more caution when using default parameter values is warranted
     35        in the future.
     36
     37        Lastly, since we have to change PlatformDisplay::createPlatformDisplay to use the new create
     38        functions, take the opportunity to move things around a bit for clarity. There should be no
     39        change in behavior. I was just disappointed that the PlatformDisplayWPE creation was at the
     40        bottom of the function, after a comment indicating that normal display creation has failed,
     41        which is not the case for WPE.
     42
     43        This all might have been a bit overkill, since the leak could probably have been fixed by
     44        passing nullptr to the PlatformDisplayWayland constructor for the wl_display and not
     45        removing WaylandCompositorDisplay's call to PlatformDisplayWayland::initialize. But the
     46        correctness of that code would then rely on implementation details of initialize, so this
     47        refactor seems better.
     48
     49        No new tests since there *should* be no behavior change. Then again, I'm touching
     50        PlatformDisplay, and history shows we don't have the greatest track record of touching this
     51        code without introducing problems.
     52
     53        * platform/graphics/PlatformDisplay.cpp:
     54        (WebCore::PlatformDisplay::createPlatformDisplay):
     55        * platform/graphics/PlatformDisplay.h:
     56        * platform/graphics/wayland/PlatformDisplayWayland.cpp:
     57        (WebCore::PlatformDisplayWayland::create):
     58        (WebCore::PlatformDisplayWayland::create):
     59        (WebCore::PlatformDisplayWayland::createHeadless):
     60        (WebCore::PlatformDisplayWayland::PlatformDisplayWayland):
     61        (WebCore::PlatformDisplayWayland::initialize):
     62        * platform/graphics/wayland/PlatformDisplayWayland.h:
     63        * platform/graphics/win/PlatformDisplayWin.h:
     64        * platform/graphics/wpe/PlatformDisplayWPE.cpp:
     65        (WebCore::create):
     66        * platform/graphics/wpe/PlatformDisplayWPE.h:
     67        * platform/graphics/x11/PlatformDisplayX11.cpp:
     68        (WebCore::PlatformDisplayX11::create):
     69        (WebCore::PlatformDisplayX11::create):
     70        (WebCore::PlatformDisplayX11::createHeadless):
     71        * platform/graphics/x11/PlatformDisplayX11.h:
     72
    1732018-04-11  Jianjun Zhu  <jianjun.zhu@intel.com>
    274

  • trunk/Source/WebCore/platform/graphics/PlatformDisplay.cpp

    r224347 r230529  
    7676#if PLATFORM(GTK)
    7777#if defined(GTK_API_VERSION_2)
    78     return std::make_unique<PlatformDisplayX11>(GDK_DISPLAY_XDISPLAY(gdk_display_get_default()));
     78    return PlatformDisplayX11::create(GDK_DISPLAY_XDISPLAY(gdk_display_get_default()));
    7979#else
    8080    GdkDisplay* display = gdk_display_manager_get_default_display(gdk_display_manager_get());
    8181#if PLATFORM(X11)
    8282    if (GDK_IS_X11_DISPLAY(display))
    83         return std::make_unique<PlatformDisplayX11>(GDK_DISPLAY_XDISPLAY(display));
     83        return PlatformDisplayX11::create(GDK_DISPLAY_XDISPLAY(display));
    8484#endif
    8585#if PLATFORM(WAYLAND)
    8686    if (GDK_IS_WAYLAND_DISPLAY(display))
    87         return std::make_unique<PlatformDisplayWayland>(gdk_wayland_display_get_wl_display(display));
    88 #endif
    89 #endif
     87        return PlatformDisplayWayland::create(gdk_wayland_display_get_wl_display(display));
     88#endif
     89#endif
     90#endif // PLATFORM(GTK)
     91
     92#if PLATFORM(WPE)
     93    return PlatformDisplayWPE::create();
    9094#elif PLATFORM(WIN)
    91     return std::make_unique<PlatformDisplayWin>();
     95    return PlatformDisplayWin::create();
    9296#endif
    9397
    … …  
    104108    // If at this point we still don't have a display, just create a fake display with no native.
    105109#if PLATFORM(WAYLAND)
    106     return std::make_unique<PlatformDisplayWayland>(nullptr);
    107 #endif
    108 #if PLATFORM(X11)
    109     return std::make_unique<PlatformDisplayX11>(nullptr);
    110 #endif
    111 
    112 #if PLATFORM(WPE)
    113     return std::make_unique<PlatformDisplayWPE>();
    114 #endif
    115 
    116     ASSERT_NOT_REACHED();
    117     return nullptr;
     110    return PlatformDisplayWayland::create(nullptr);
     111#elif PLATFORM(X11)
     112    return PlatformDisplayX11::create(nullptr);
     113#endif
     114
     115    RELEASE_ASSERT_NOT_REACHED();
    118116}
    119117

  • trunk/Source/WebCore/platform/graphics/PlatformDisplay.h

    r230229 r230529  
    7474protected:
    7575    enum class NativeDisplayOwned { No, Yes };
    76     explicit PlatformDisplay(NativeDisplayOwned = NativeDisplayOwned::No);
     76    explicit PlatformDisplay(NativeDisplayOwned);
    7777
    7878    static void setSharedDisplayForCompositing(PlatformDisplay&);

  • trunk/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp

    r229525 r230529  
    5757        return nullptr;
    5858
    59     return std::make_unique<PlatformDisplayWayland>(display, NativeDisplayOwned::Yes);
     59    auto platformDisplay = std::unique_ptr<PlatformDisplayWayland>(new PlatformDisplayWayland(display, NativeDisplayOwned::Yes));
     60    platformDisplay->initialize();
     61    return platformDisplay;
     62}
     63
     64std::unique_ptr<PlatformDisplay> PlatformDisplayWayland::create(struct wl_display* display)
     65{
     66    auto platformDisplay = std::unique_ptr<PlatformDisplayWayland>(new PlatformDisplayWayland(display, NativeDisplayOwned::No));
     67    platformDisplay->initialize();
     68    return platformDisplay;
    6069}
    6170
    6271PlatformDisplayWayland::PlatformDisplayWayland(struct wl_display* display, NativeDisplayOwned displayOwned)
    6372    : PlatformDisplay(displayOwned)
     73    , m_display(display)
    6474{
    65     initialize(display);
    6675}
    6776
    … …  
    7584}
    7685
    77 void PlatformDisplayWayland::initialize(wl_display* display)
     86void PlatformDisplayWayland::initialize()
    7887{
    79     m_display = display;
    8088    if (!m_display)
    8189        return;

  • trunk/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h

    r230442 r230529  
    3838public:
    3939    static std::unique_ptr<PlatformDisplay> create();
    40     PlatformDisplayWayland(struct wl_display*, NativeDisplayOwned = NativeDisplayOwned::No);
     40    static std::unique_ptr<PlatformDisplay> create(struct wl_display*);
     41
    4142    virtual ~PlatformDisplayWayland();
    4243
    … …  
    4849    static const struct wl_registry_listener s_registryListener;
    4950
    50     Type type() const override { return PlatformDisplay::Type::Wayland; }
     51    Type type() const final { return PlatformDisplay::Type::Wayland; }
    5152
    5253protected:
    53     PlatformDisplayWayland() = default;
    54     void initialize(struct wl_display*);
     54    PlatformDisplayWayland(struct wl_display*, NativeDisplayOwned);
     55
     56    void initialize();
    5557
    5658    virtual void registryGlobal(const char* interface, uint32_t name);

  • trunk/Source/WebCore/platform/graphics/win/PlatformDisplayWin.h

    r197563 r230529  
    3434
    3535class PlatformDisplayWin final : public PlatformDisplay {
     36public:
     37    static std::unique_ptr<PlatformDisplayWin> create()
     38    {
     39        return std::unique_ptr<PlatformDisplayWin>(new PlatformDisplayWin());
     40    }
     41
     42    virtual ~PlatformDisplayWin() = default;
     43
    3644private:
     45    PlatformDisplayWin()
     46        : PlatformDisplay(NativeDisplayOwned::No)
     47    {
     48    }
     49
    3750    Type type() const override { return PlatformDisplay::Type::Windows; }
    3851};

  • trunk/Source/WebCore/platform/graphics/wpe/PlatformDisplayWPE.cpp

    r224412 r230529  
    3838namespace WebCore {
    3939
    40 PlatformDisplayWPE::PlatformDisplayWPE() = default;
     40std::unique_ptr<PlatformDisplayWPE> PlatformDisplayWPE::create()
     41{
     42    return std::unique_ptr<PlatformDisplayWPE>(new PlatformDisplayWPE());
     43}
     44
     45PlatformDisplayWPE::PlatformDisplayWPE()
     46    : PlatformDisplay(NativeDisplayOwned::No)
     47{
     48}
    4149
    4250PlatformDisplayWPE::~PlatformDisplayWPE()

  • trunk/Source/WebCore/platform/graphics/wpe/PlatformDisplayWPE.h

    r217208 r230529  
    3636class PlatformDisplayWPE final : public PlatformDisplay {
    3737public:
    38     PlatformDisplayWPE();
     38    static std::unique_ptr<PlatformDisplayWPE> create();
     39
    3940    virtual ~PlatformDisplayWPE();
    4041
    … …  
    4445
    4546private:
     47    PlatformDisplayWPE();
     48
    4649    Type type() const override { return PlatformDisplay::Type::WPE; }
    4750

  • trunk/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp

    r211145 r230529  
    4949        return nullptr;
    5050
    51     return std::make_unique<PlatformDisplayX11>(display, NativeDisplayOwned::Yes);
     51    return std::unique_ptr<PlatformDisplayX11>(new PlatformDisplayX11(display, NativeDisplayOwned::Yes));
     52}
     53
     54std::unique_ptr<PlatformDisplay> PlatformDisplayX11::create(Display* display)
     55{
     56    return std::unique_ptr<PlatformDisplayX11>(new PlatformDisplayX11(display, NativeDisplayOwned::No));
    5257}
    5358

  • trunk/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.h

    r211145 r230529  
    3939public:
    4040    static std::unique_ptr<PlatformDisplay> create();
    41     PlatformDisplayX11(Display*, NativeDisplayOwned = NativeDisplayOwned::No);
     41    static std::unique_ptr<PlatformDisplay> create(Display*);
     42
    4243    virtual ~PlatformDisplayX11();
    4344
    … …  
    4748
    4849private:
     50    PlatformDisplayX11(Display*, NativeDisplayOwned);
     51
    4952    Type type() const override { return PlatformDisplay::Type::X11; }
    5053

  • trunk/Source/WebKit/ChangeLog

    r230528 r230529  
     12018-04-11  Michael Catanzaro  <mcatanzaro@igalia.com>
     2
     3        [GTK] WaylandCompositorDisplay leaks its wl_display
     4        https://bugs.webkit.org/show_bug.cgi?id=184406
     5
     6        Reviewed by Carlos Garcia Campos.
     7
     8        Since we allocate our own wl_display here, need to chain up to the parent constructor
     9        passing NativeDisplayOwned::Yes, or it won't ever be released. Move the initialize call to
     10        the create function to ensure it's called after the constructor completes.
     11
     12        * WebProcess/gtk/WaylandCompositorDisplay.cpp:
     13        (WebKit::WaylandCompositorDisplay::create): Fix a log message (drive-by).
     14        (WebKit::WaylandCompositorDisplay::WaylandCompositorDisplay):
     15
    1162018-04-11  Youenn Fablet  <youenn@apple.com>
    217

  • trunk/Source/WebKit/WebProcess/gtk/WaylandCompositorDisplay.cpp

    r230442 r230529  
    4545    struct wl_display* display = wl_display_connect(displayName.utf8().data());
    4646    if (!display) {
    47         WTFLogAlways("PlatformDisplayWayland initialization: failed to connect to the Wayland display: %s", displayName.utf8().data());
     47        WTFLogAlways("WaylandCompositorDisplay initialization: failed to connect to the Wayland display: %s", displayName.utf8().data());
    4848        return nullptr;
    4949    }
    5050
    51     return std::unique_ptr<WaylandCompositorDisplay>(new WaylandCompositorDisplay(display));
     51    auto compositorDisplay = std::unique_ptr<WaylandCompositorDisplay>(new WaylandCompositorDisplay(display));
     52    compositorDisplay->initialize();
     53    return compositorDisplay;
    5254}
    5355
    … …  
    6264
    6365WaylandCompositorDisplay::WaylandCompositorDisplay(struct wl_display* display)
     66    : PlatformDisplayWayland(display, NativeDisplayOwned::Yes)
    6467{
    65     initialize(display);
    6668    PlatformDisplay::setSharedDisplayForCompositing(*this);
    6769}
Note: See TracChangeset for help on using the changeset viewer.