Summary: | [GTK] WaylandCompositorDisplay leaks its wl_display | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||
Component: | WebKitGTK | Assignee: | 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
Michael Catanzaro
2018-04-08 18:38:09 PDT
Created attachment 337475 [details]
Patch
Comment on attachment 337475 [details]
Patch
Good catch!
Comment on attachment 337475 [details] Patch Clearing flags on attachment: 337475 Committed r230390: <https://trac.webkit.org/changeset/230390> All reviewed patches have been landed. Closing bug. Reverted r230390 for reason: Broke accelerated compositing Committed r230442: <https://trac.webkit.org/changeset/230442> Created attachment 337562 [details]
Patch
Carlos, please review it again. I had some fun with this one.... Created attachment 337563 [details]
Patch
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 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? 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! (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. 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. Created attachment 337625 [details]
Patch
Let's see if this version passes EWS. (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. Created attachment 337626 [details]
Patch
Comment on attachment 337626 [details] Patch Clearing flags on attachment: 337626 Committed r230529: <https://trac.webkit.org/changeset/230529> All reviewed patches have been landed. Closing bug. (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 Michael, please check the debian stable build failure. 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. 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. (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. (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. (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. (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 (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. (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? (In reply to Carlos Alberto Lopez Perez from comment #29) > Does it look good to you? OK, that's not too messy. Committed r230706: <https://trac.webkit.org/changeset/230706> |