Bug 184406

Summary: [GTK] WaylandCompositorDisplay leaks its wl_display
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, clopez, commit-queue, csaavedra, mcatanzaro, Ms2ger
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Michael Catanzaro 2018-04-08 18:38:09 PDT
WaylandCompositorDisplay leaks its wl_display. It needs to pass NativeDisplayOwned::Yes when initializing its PlatformDisplayWayland base class, because it allocated its wl_display itself.
Comment 1 Michael Catanzaro 2018-04-08 19:04:54 PDT
Created attachment 337475 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-04-08 23:47:33 PDT
Comment on attachment 337475 [details]
Patch

Good catch!
Comment 3 WebKit Commit Bot 2018-04-09 00:14:20 PDT
Comment on attachment 337475 [details]
Patch

Clearing flags on attachment: 337475

Committed r230390: <https://trac.webkit.org/changeset/230390>
Comment 4 WebKit Commit Bot 2018-04-09 00:14:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Michael Catanzaro 2018-04-09 09:53:26 PDT
Reverted r230390 for reason:

Broke accelerated compositing

Committed r230442: <https://trac.webkit.org/changeset/230442>
Comment 6 Michael Catanzaro 2018-04-09 17:28:44 PDT
Created attachment 337562 [details]
Patch
Comment 7 Michael Catanzaro 2018-04-09 17:29:56 PDT
Carlos, please review it again. I had some fun with this one....
Comment 8 Michael Catanzaro 2018-04-09 17:36:54 PDT
Created attachment 337563 [details]
Patch
Comment 9 Carlos Garcia Campos 2018-04-10 02:59:18 PDT
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.
Comment 10 Ms2ger (he/him; ⌚ UTC+1/+2) 2018-04-10 03:46:09 PDT
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?
Comment 11 Michael Catanzaro 2018-04-10 11:00:16 PDT
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!
Comment 12 Michael Catanzaro 2018-04-10 11:58:51 PDT
(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.
Comment 13 Michael Catanzaro 2018-04-10 12:01:34 PDT
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.
Comment 14 Michael Catanzaro 2018-04-10 12:09:40 PDT
Created attachment 337625 [details]
Patch
Comment 15 Michael Catanzaro 2018-04-10 12:09:56 PDT
Let's see if this version passes EWS.
Comment 16 Michael Catanzaro 2018-04-10 12:11:51 PDT
(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.
Comment 17 Michael Catanzaro 2018-04-10 12:18:26 PDT
Created attachment 337626 [details]
Patch
Comment 18 WebKit Commit Bot 2018-04-11 09:58:28 PDT
Comment on attachment 337626 [details]
Patch

Clearing flags on attachment: 337626

Committed r230529: <https://trac.webkit.org/changeset/230529>
Comment 19 WebKit Commit Bot 2018-04-11 09:58:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Carlos Alberto Lopez Perez 2018-04-12 17:35:50 PDT
(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
Comment 21 Claudio Saavedra 2018-04-13 02:54:56 PDT
Michael, please check the debian stable build failure.
Comment 22 Michael Catanzaro 2018-04-13 07:28:31 PDT
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.
Comment 23 Michael Catanzaro 2018-04-13 07:30:34 PDT
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.
Comment 24 Carlos Alberto Lopez Perez 2018-04-13 08:41:40 PDT
(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.
Comment 25 Michael Catanzaro 2018-04-13 08:51:41 PDT
(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.
Comment 26 Carlos Alberto Lopez Perez 2018-04-16 08:17:14 PDT
(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.
Comment 27 Carlos Alberto Lopez Perez 2018-04-16 08:50:49 PDT
(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
Comment 28 Carlos Alberto Lopez Perez 2018-04-16 08:58:30 PDT
(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.
Comment 29 Carlos Alberto Lopez Perez 2018-04-16 09:20:12 PDT
(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?
Comment 30 Michael Catanzaro 2018-04-16 11:38:21 PDT
(In reply to Carlos Alberto Lopez Perez from comment #29)
> Does it look good to you?

OK, that's not too messy.
Comment 31 Carlos Alberto Lopez Perez 2018-04-17 04:29:35 PDT
Committed r230706: <https://trac.webkit.org/changeset/230706>