SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.63k stars 3.19k forks source link

REGISTER_WIDGET does not work across library boundaries. #4161

Closed asynts closed 3 years ago

asynts commented 3 years ago

The following snippet should register Web::InProcessWebView in GUI::WidgetClassRegistration, but doesn't:

https://github.com/SerenityOS/serenity/blob/54ade31d84c8078fd0113dd9b0d188c04a4dea0c/Libraries/LibWeb/InProcessWebView.cpp#L62

The following example application prints out all registered classes, Web::InProcessWebView is missing from that list, even if LibWeb was linked.

#include <LibGUI/Widget.h>

int main()
{
    GUI::WidgetClassRegistration::for_each([](const auto& registration) {
        dbgln("{}", registration.class_name());
    });
}

Here is a list that is produced by that application:

GUI::TextBox
GUI::TextEditor
GUI::HorizontalSplitter
GUI::Slider
GUI::ToolBar
GUI::Widget
GUI::Frame
GUI::TabWidget
GUI::RadioButton
GUI::CheckBox
GUI::ColorInput
GUI::StatusBar
GUI::GroupBox
GUI::SpinBox
GUI::Button
GUI::ScrollBar
GUI::ToolBarContainer
GUI::Label

Note that GUI::TabWidget is present in that list even though it is defined in a different compilation unit than say GUI::Button:

https://github.com/SerenityOS/serenity/blob/54ade31d84c8078fd0113dd9b0d188c04a4dea0c/Libraries/LibGUI/Widget.cpp#L60

https://github.com/SerenityOS/serenity/blob/54ade31d84c8078fd0113dd9b0d188c04a4dea0c/Libraries/LibGUI/TabWidget.cpp#L38

At first I though this is an "static initialization fiasco", but this does not appear to be the case because it worked for GUI::TabWidget and the helper function seems well defined.

My second thought was that maybe we have two widget_classes functions defined because that function was marked static, but moving it into the header didn't change a thing.

https://github.com/SerenityOS/serenity/blob/54ade31d84c8078fd0113dd9b0d188c04a4dea0c/Libraries/LibGUI/Widget.cpp#L78-L84

I don't understand this. Any ideas?


Here is a reproducible example: asynts@05af80fc036739f303be8ff0aa99efec5ff72457

linusg commented 3 years ago

Huh - It certainly works with LibWeb's OOPWV:

https://github.com/SerenityOS/serenity/blob/b97a900595af4ca19d2eaf334a758387fc977557/Libraries/LibWeb/OutOfProcessWebView.cpp#L36

https://github.com/SerenityOS/serenity/blob/f6af2d747e236e938f0015ba2a0b4a968f9f8dd1/Applications%2FTextEditor%2FMainWindow.json#L27-L31

Also Spreadsheet's ConditionsView.

Extending your example like this works:

#include <LibGUI/Widget.h>
#include <LibWeb/InProcessWebView.h>

int main(int argc, char** argv)
{
    // Constructing a `GUI::Application` is technically not required but not doing so makes `Web::InProcessWebView`'s constructor crash.
    auto app = GUI::Application::construct(argc, argv);
    GUI::WidgetClassRegistration::for_each([](const auto& registration) {
        dbgln("{}", registration.class_name());
    });

    Web::InProcessWebView::construct();
}

Including LibWeb/InProcessWebView.h alone is not enough, but once InProcessWebView is constructed (anywhere), it also appears in the list of registered widgets.

Why? No clue.

asynts commented 3 years ago

"Deferred dynamic initialization" goes on my list with the worst C++ anti-features:

If no variable or function is odr-used from a given translation unit, the non-local variables defined in that translation unit may never be initialized (this models the behavior of an on-demand dynamic library).

https://en.cppreference.com/w/cpp/language/initialization#Deferred_dynamic_initialization

Basically, if you don't touch a translation unit, static variables might never be created.


Not sure how to fix it though.

asynts commented 3 years ago

I spend some time thinking about this and this isn't really an issue, it's just a bit unintuitive. In my opinion it is acceptable if a registration is missing provided that the class isn't being used. This should allow the compiler to not link the library at all unless it is actually used.