Bug 184406 – [GTK] WaylandCompositorDisplay leaks its wl_display

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

WaylandCompositorDisplay leaks its wl_display. It needs to pass NativeDisplayOwned::Yes when initializing its PlatformDisplayWayland base class, because it allocated its wl_display itself.

[Michael Catanzaro]

Created attachment 337475 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 337475 [details]
Patch

Good catch!

[WebKit Commit Bot]

Comment on attachment 337475 [details]
Patch

Clearing flags on attachment: 337475

Committed r230390: <https://trac.webkit.org/changeset/230390>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Reverted r230390 for reason:

Broke accelerated compositing

Committed r230442: <https://trac.webkit.org/changeset/230442>

[Michael Catanzaro]

Created attachment 337562 [details]
Patch

[Michael Catanzaro]

Carlos, please review it again. I had some fun with this one....

[Michael Catanzaro]

Created attachment 337563 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 337563 [details]
Patch

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

Good catch! It seems WPE doesn't build with this.

> Source/WebCore/ChangeLog:17
> +        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.

Wow, great analysis.

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:78
> +    return PlatformDisplayX11::createForNativeDisplay(GDK_DISPLAY_XDISPLAY(gdk_display_get_default()));

I don't think we need to rename the create() method, the parameter is self-explanatory, create() with no parameter creates a new with connection to the default display, create that receives a native display used that one. If the native display is nullptr it's "headless". Note that headless is not really a valid config, I think, because nothing will work, maybe we should just crash there and never allow to create a display with a null native.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 337563 [details]
Patch

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

> Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:59
> +    auto platformDisplay = std::unique_ptr<PlatformDisplayWayland>(new PlatformDisplayWayland(display, NativeDisplayOwned::Yes));

There's a number of places where it seems you could use std::make_unique(); why not use it?

[Michael Catanzaro]

I'll fix WPE and Windows.

(In reply to Carlos Garcia Campos from comment #9)
> I don't think we need to rename the create() method, the parameter is
> self-explanatory, create() with no parameter creates a new with connection
> to the default display, create that receives a native display used that one.
> If the native display is nullptr it's "headless".

OK, good idea.

(In reply to Carlos Garcia Campos from comment #9) 
> Note that headless is not
> really a valid config, I think, because nothing will work, maybe we should
> just crash there and never allow to create a display with a null native.

I think stuff breaks if we do that, remember the crash when building yelp?

(In reply to Ms2ger from comment #10)
> There's a number of places where it seems you could use std::make_unique();
> why not use it?

Can't because the constructor is private. If you're able to use std::make_unique from a create function, that's bad because it means you forgot to make the constructor private!

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #11)
> (In reply to Carlos Garcia Campos from comment #9)
> > I don't think we need to rename the create() method, the parameter is
> > self-explanatory, create() with no parameter creates a new with connection
> > to the default display, create that receives a native display used that one.
> > If the native display is nullptr it's "headless".
> 
> OK, good idea.

Actually this didn't turn out as well as I'd like. Then we have, for example:

    static std::unique_ptr<PlatformDisplay> create();
    static std::unique_ptr<PlatformDisplay> create(Display*);

where the distinction between calling create with no arguments ("connect to default display") and calling it with nullptr (headless mode) is really unclear. So I'll keep the separate createHeadless. But renaming from createForNativeDisplay to create seems fine.

[Michael Catanzaro]

Also: it's true that in the X11 case, createHeadless is exactly the same as calling create with nullptr, but in the Wayland case the code is slightly different.

[Michael Catanzaro]

Created attachment 337625 [details]
Patch

[Michael Catanzaro]

Let's see if this version passes EWS.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #13)
> Also: it's true that in the X11 case, createHeadless is exactly the same as
> calling create with nullptr, but in the Wayland case the code is slightly
> different.

I keep changing my mind on this. I wound up making create safe in case nullptr is passed for the display, but now there's really no good reason to keep the headless version, so I think I'll go with your suggestion after all.

[Michael Catanzaro]

Created attachment 337626 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337626 [details]
Patch

Clearing flags on attachment: 337626

Committed r230529: <https://trac.webkit.org/changeset/230529>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(In reply to WebKit Commit Bot from comment #18)
> Comment on attachment 337626 [details]
> Patch
> 
> Clearing flags on attachment: 337626
> 
> Committed r230529: <https://trac.webkit.org/changeset/230529>

Seems this has broken Debian stable (still oldstable: jessie) bot build (clang-3.8 with libstdc++-4.9)

Good build (r230528): https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/11502
Bad  build (r230529): https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/11503

[Claudio Saavedra]

Michael, please check the debian stable build failure.

[Michael Catanzaro]

I don't think it's reasonable to be using libstdc++-4.9 on the buildbot, when we dropped support for the 4.9 toolchain over a year ago. The bot could be upgraded to use libstdc++-6 built with --with-default-libstdcxx-abi=gcc4-compatible, but we promised Apple that we would require GCC 6 two weeks from now, so might as well skip ahead and just upgrade the buildbot to Stretch now. Then we can switch it back to using GCC, and we don't need to do any additional work. The standard library upgrades are half the point of requiring newer GCC, after all.

It's been a very useful bot, btw. I'm glad we added it.

[Michael Catanzaro]

BTW, the problem is:

../../Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:61:12: error: no viable conversion from returned value of type 'unique_ptr<WebCore::PlatformDisplayWayland, default_delete<WebCore::PlatformDisplayWayland>>' to function return type 'unique_ptr<WebCore::PlatformDisplay, default_delete<WebCore::PlatformDisplay>>'
    return platformDisplay;
           ^~~~~~~~~~~~~~~

which is a standard library bug, unique_ptr is supposed to support polymorphism, but the GCC 4.9 unique_ptr does not.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #22)
> I don't think it's reasonable to be using libstdc++-4.9 on the buildbot,
> when we dropped support for the 4.9 toolchain over a year ago. 

> The bot could
> be upgraded to use libstdc++-6 built with
> --with-default-libstdcxx-abi=gcc4-compatible, 

No. That's not an option.

Because, its basically impossible to upgrade libstdc++ on a standard linux Distro without dist-upgrading (using the official distro packages).

So what we can do is to upgrade the bot to Debian stretch and give up on the promise of keeping WebKit buildable in the year after the next stable release.

At this point I'm fine with just doing that: we managed to keep the promise of the extra year of support for 9 months, and given the upcoming GCC 6 version raise this seems reasonable.

Being realistic: I don't think any developer is still running Debian Jessie. And Debian is still not taking our security updates in any case.

But fixing this build failure if its easy to do that's also a preferred option.

In any case, likely we will have to upgrade this anyway when people start using c++17 features in May after the GCC raise.

> but we promised Apple that we
> would require GCC 6 two weeks from now, so might as well skip ahead and just
> upgrade the buildbot to Stretch now. Then we can switch it back to using
> GCC, and we don't need to do any additional work. The standard library
> upgrades are half the point of requiring newer GCC, after all.
> 
> It's been a very useful bot, btw. I'm glad we added it.

Agreed.

I'm OK with upgrading the bot to stretch and switch it back to GCC.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #24)
> So what we can do is to upgrade the bot to Debian stretch and give up on the
> promise of keeping WebKit buildable in the year after the next stable
> release.

Well, we don't have to discuss this again until the next time we have this problem. libstdc++ ABI breaks are unusual and it's possible we won't have to figure this out again for a long time. So for now, let's just upgrade the bot, and hopefully we won't have to deal with this problem again for a long time.

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #23)
> BTW, the problem is:
> 
> ../../Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:61:
> 12: error: no viable conversion from returned value of type
> 'unique_ptr<WebCore::PlatformDisplayWayland,
> default_delete<WebCore::PlatformDisplayWayland>>' to function return type
> 'unique_ptr<WebCore::PlatformDisplay,
> default_delete<WebCore::PlatformDisplay>>'
>     return platformDisplay;
>            ^~~~~~~~~~~~~~~
> 
> which is a standard library bug, unique_ptr is supposed to support
> polymorphism, but the GCC 4.9 unique_ptr does not.

It seems this is no a libstdc++_4.9 bug, but an issue with Clang itself maybe?.

I'm able to reproduce the very same build failure with Clang_3.8 and libstdc++_6.3.0 (Debian stretch).

../../Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:61:12: error: no viable conversion from returned value of type 'unique_ptr<WebCore::PlatformDisplayWayland, default_delete<WebCore::PlatformDisplayWayland>>' to function return type 'unique_ptr<WebCore::PlatformDisplay, default_delete<WebCore::PlatformDisplay>>'
    return platformDisplay;
           ^~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:204:17: note: candidate constructor not viable: no known conversion from 'std::unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> >' to 'nullptr_t' for 1st argument
      constexpr unique_ptr(nullptr_t) noexcept : unique_ptr() { }
                ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:209:7: note: candidate constructor not viable: no known conversion from 'std::unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> >' to 'std::unique_ptr<WebCore::PlatformDisplay, std::default_delete<WebCore::PlatformDisplay> > &&' for 1st argument
      unique_ptr(unique_ptr&& __u) noexcept
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:223:2: note: candidate constructor [with _Up = WebCore::PlatformDisplayWayland, _Ep = std::default_delete<WebCore::PlatformDisplayWayland>, $2 = void] not viable: no known conversion from 'std::unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> >' to 'unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> > &&' for 1st argument
        unique_ptr(unique_ptr<_Up, _Ep>&& __u) noexcept
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:359:7: note: candidate constructor not viable: no known conversion from 'std::unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> >' to 'const std::unique_ptr<WebCore::PlatformDisplay, std::default_delete<WebCore::PlatformDisplay> > &' for 1st argument
      unique_ptr(const unique_ptr&) = delete;
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:231:2: note: candidate template ignored: could not match 'auto_ptr' against 'unique_ptr'
        unique_ptr(auto_ptr<_Up>&& __u) noexcept;
        ^
In file included from /home/igalia/clopez/webkit/webkit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource364.cpp:1:
../../Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:68:12: error: no viable conversion from returned value of type 'unique_ptr<WebCore::PlatformDisplayWayland, default_delete<WebCore::PlatformDisplayWayland>>' to function return type 'unique_ptr<WebCore::PlatformDisplay, default_delete<WebCore::PlatformDisplay>>'
    return platformDisplay;
           ^~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:204:17: note: candidate constructor not viable: no known conversion from 'std::unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> >' to 'nullptr_t' for 1st argument
      constexpr unique_ptr(nullptr_t) noexcept : unique_ptr() { }
                ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:209:7: note: candidate constructor not viable: no known conversion from 'std::unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> >' to 'std::unique_ptr<WebCore::PlatformDisplay, std::default_delete<WebCore::PlatformDisplay> > &&' for 1st argument
      unique_ptr(unique_ptr&& __u) noexcept
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:223:2: note: candidate constructor [with _Up = WebCore::PlatformDisplayWayland, _Ep = std::default_delete<WebCore::PlatformDisplayWayland>, $2 = void] not viable: no known conversion from 'std::unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> >' to 'unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> > &&' for 1st argument
        unique_ptr(unique_ptr<_Up, _Ep>&& __u) noexcept
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:359:7: note: candidate constructor not viable: no known conversion from 'std::unique_ptr<WebCore::PlatformDisplayWayland, std::default_delete<WebCore::PlatformDisplayWayland> >' to 'const std::unique_ptr<WebCore::PlatformDisplay, std::default_delete<WebCore::PlatformDisplay> > &' for 1st argument
      unique_ptr(const unique_ptr&) = delete;
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/unique_ptr.h:231:2: note: candidate template ignored: could not match 'auto_ptr' against 'unique_ptr'
        unique_ptr(auto_ptr<_Up>&& __u) noexcept;
        ^
2 errors generated.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #26)
> (In reply to Michael Catanzaro from comment #23)
> > BTW, the problem is:
> > 
> > ../../Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:61:
> > 12: error: no viable conversion from returned value of type
> > 'unique_ptr<WebCore::PlatformDisplayWayland,
> > default_delete<WebCore::PlatformDisplayWayland>>' to function return type
> > 'unique_ptr<WebCore::PlatformDisplay,
> > default_delete<WebCore::PlatformDisplay>>'
> >     return platformDisplay;
> >            ^~~~~~~~~~~~~~~
> > 
> > which is a standard library bug, unique_ptr is supposed to support
> > polymorphism, but the GCC 4.9 unique_ptr does not.
> 
> It seems this is no a libstdc++_4.9 bug, but an issue with Clang itself
> maybe?.
> 
> I'm able to reproduce the very same build failure with Clang_3.8 and
> libstdc++_6.3.0 (Debian stretch).
> 

With clang_4.0 works, so its something affecting clang < 4.0

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #27)
> (In reply to Carlos Alberto Lopez Perez from comment #26)
> > (In reply to Michael Catanzaro from comment #23)
> > > BTW, the problem is:
> > > 
> > > ../../Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:61:
> > > 12: error: no viable conversion from returned value of type
> > > 'unique_ptr<WebCore::PlatformDisplayWayland,
> > > default_delete<WebCore::PlatformDisplayWayland>>' to function return type
> > > 'unique_ptr<WebCore::PlatformDisplay,
> > > default_delete<WebCore::PlatformDisplay>>'
> > >     return platformDisplay;
> > >            ^~~~~~~~~~~~~~~
> > > 
> > > which is a standard library bug, unique_ptr is supposed to support
> > > polymorphism, but the GCC 4.9 unique_ptr does not.
> > 
> > It seems this is no a libstdc++_4.9 bug, but an issue with Clang itself
> > maybe?.
> > 
> > I'm able to reproduce the very same build failure with Clang_3.8 and
> > libstdc++_6.3.0 (Debian stretch).
> > 
> 
> With clang_4.0 works, so its something affecting clang < 4.0

It seems its related to this issue: https://stackoverflow.com/questions/48098327/polymorphic-unique-ptr-copy-elision

So, either we fix this to build with clang-3.8 or we declare a minimum compiler version on clang-4.0 when building with clang. Clang-3.8 was released on 2016/03.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #28)
> (In reply to Carlos Alberto Lopez Perez from comment #27)
> > (In reply to Carlos Alberto Lopez Perez from comment #26)
> > > (In reply to Michael Catanzaro from comment #23)
> > > > BTW, the problem is:
> > > > 
> > > > ../../Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:61:
> > > > 12: error: no viable conversion from returned value of type
> > > > 'unique_ptr<WebCore::PlatformDisplayWayland,
> > > > default_delete<WebCore::PlatformDisplayWayland>>' to function return type
> > > > 'unique_ptr<WebCore::PlatformDisplay,
> > > > default_delete<WebCore::PlatformDisplay>>'
> > > >     return platformDisplay;
> > > >            ^~~~~~~~~~~~~~~
> > > > 
> > > > which is a standard library bug, unique_ptr is supposed to support
> > > > polymorphism, but the GCC 4.9 unique_ptr does not.
> > > 
> > > It seems this is no a libstdc++_4.9 bug, but an issue with Clang itself
> > > maybe?.
> > > 
> > > I'm able to reproduce the very same build failure with Clang_3.8 and
> > > libstdc++_6.3.0 (Debian stretch).
> > > 
> > 
> > With clang_4.0 works, so its something affecting clang < 4.0
> 
> It seems its related to this issue:
> https://stackoverflow.com/questions/48098327/polymorphic-unique-ptr-copy-
> elision
> 
> So, either we fix this to build with clang-3.8 or we declare a minimum
> compiler version on clang-4.0 when building with clang. Clang-3.8 was
> released on 2016/03.

This will fix the build:


$ git diff
diff --git a/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp b/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp
index 05f92c228fe..d62d054e7d7 100644
--- a/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp
+++ b/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp
@@ -58,14 +58,14 @@ std::unique_ptr<PlatformDisplay> PlatformDisplayWayland::create()
 
     auto platformDisplay = std::unique_ptr<PlatformDisplayWayland>(new PlatformDisplayWayland(display, NativeDisplayOwned::Yes));
     platformDisplay->initialize();
-    return platformDisplay;
+    return WTFMove(platformDisplay);
 }
 
 std::unique_ptr<PlatformDisplay> PlatformDisplayWayland::create(struct wl_display* display)
 {
     auto platformDisplay = std::unique_ptr<PlatformDisplayWayland>(new PlatformDisplayWayland(display, NativeDisplayOwned::No));
     platformDisplay->initialize();
-    return platformDisplay;
+    return WTFMove(platformDisplay);
 }
 
 PlatformDisplayWayland::PlatformDisplayWayland(struct wl_display* display, NativeDisplayOwned displayOwned)




Does it look good to you?

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #29)
> Does it look good to you?

OK, that's not too messy.

[Carlos Alberto Lopez Perez]

Committed r230706: <https://trac.webkit.org/changeset/230706>