From 6a6c56aabeeb1d31339613cf87143a78b6b86822 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Wed, 23 Apr 2025 12:37:05 -0400 Subject: [PATCH] LibWebView: Create a dedicated settings observer for the Application We had a bit of an awkward setup where we want the Application to be a SettingsObserver, but neither the Settings object nor the Application itself was fully initialized by the time the observer was created. So we invented a deferred observer initializer specifically for the Application. Instead, let's just create a dedicated observer subclass that is owned by the Application. We can then create it once we have the singleton Application appropriately set up. --- Libraries/LibWebView/Application.cpp | 55 ++++++++++---------- Libraries/LibWebView/Application.h | 10 ++-- Libraries/LibWebView/Settings.cpp | 77 +++++++++++----------------- Libraries/LibWebView/Settings.h | 13 +---- 4 files changed, 65 insertions(+), 90 deletions(-) diff --git a/Libraries/LibWebView/Application.cpp b/Libraries/LibWebView/Application.cpp index c3f9a60d18c..10d474dcaf0 100644 --- a/Libraries/LibWebView/Application.cpp +++ b/Libraries/LibWebView/Application.cpp @@ -26,13 +26,32 @@ namespace WebView { Application* Application::s_the = nullptr; +struct ApplicationSettingsObserver : public SettingsObserver { + virtual void dns_settings_changed() override + { + Application::settings().dns_settings().visit( + [](SystemDNS) { + Application::request_server_client().async_set_use_system_dns(); + }, + [](DNSOverTLS const& dns_over_tls) { + dbgln("Setting DNS server to {}:{} with TLS", dns_over_tls.server_address, dns_over_tls.port); + Application::request_server_client().async_set_dns_server(dns_over_tls.server_address, dns_over_tls.port, true); + }, + [](DNSOverUDP const& dns_over_udp) { + dbgln("Setting DNS server to {}:{}", dns_over_udp.server_address, dns_over_udp.port); + Application::request_server_client().async_set_dns_server(dns_over_udp.server_address, dns_over_udp.port, false); + }); + } +}; + Application::Application() - : SettingsObserver(AddToObservers::No) // Application::the() is not set yet. - , m_settings(Settings::create({})) + : m_settings(Settings::create({})) { VERIFY(!s_the); s_the = this; + m_settings_observer = make(); + // No need to monitor the system time zone if the TZ environment variable is set, as it overrides system preferences. if (!Core::Environment::has("TZ"sv)) { if (auto time_zone_watcher = Core::TimeZoneWatcher::create(); time_zone_watcher.is_error()) { @@ -52,13 +71,13 @@ Application::Application() m_process_manager.on_process_exited = [this](Process&& process) { process_did_exit(move(process)); }; - - SettingsObserver::complete_delayed_registration(); } Application::~Application() { - SettingsObserver::complete_delayed_unregistration(); + // Explicitly delete the settings observer first, as the observer destructor will refer to Application::the(). + m_settings_observer.clear(); + s_the = nullptr; } @@ -175,9 +194,6 @@ void Application::initialize(Main::Arguments const& arguments) .devtools_port = devtools_port, }; - if (m_browser_options.dns_settings.has_value()) - m_settings.set_dns_settings(m_browser_options.dns_settings.value(), true); - if (webdriver_content_ipc_path.has_value()) m_browser_options.webdriver_content_ipc_path = *webdriver_content_ipc_path; @@ -268,6 +284,10 @@ ErrorOr Application::launch_request_server() { // FIXME: Create an abstraction to re-spawn the RequestServer and re-hook up its client hooks to each tab on crash m_request_server_client = TRY(launch_request_server_process()); + + if (m_browser_options.dns_settings.has_value()) + m_settings.set_dns_settings(m_browser_options.dns_settings.value(), true); + return {}; } @@ -717,23 +737,4 @@ void Application::request_console_messages(DevTools::TabDescription const& descr view->js_console_request_messages(start_index); } -void Application::dns_settings_changed() -{ - if (!m_request_server_client) - return; - auto dns_settings = settings().dns_settings(); - auto& rs_client = *m_request_server_client; - dns_settings.visit( - [&](SystemDNS) { - rs_client.async_set_use_system_dns(); - }, - [&](DNSOverTLS const& dns_over_tls) { - dbgln("Setting DNS server to {}:{} with TLS", dns_over_tls.server_address, dns_over_tls.port); - rs_client.async_set_dns_server(dns_over_tls.server_address, dns_over_tls.port, true); - }, - [&](DNSOverUDP const& dns_over_udp) { - dbgln("Setting DNS server to {}:{}", dns_over_udp.server_address, dns_over_udp.port); - rs_client.async_set_dns_server(dns_over_udp.server_address, dns_over_udp.port, false); - }); -} } diff --git a/Libraries/LibWebView/Application.h b/Libraries/LibWebView/Application.h index f7f09e4dfa2..0d1b0ae84da 100644 --- a/Libraries/LibWebView/Application.h +++ b/Libraries/LibWebView/Application.h @@ -26,8 +26,9 @@ namespace WebView { -class Application : public DevTools::DevToolsDelegate - , public SettingsObserver { +struct ApplicationSettingsObserver; + +class Application : public DevTools::DevToolsDelegate { AK_MAKE_NONCOPYABLE(Application); public: @@ -129,12 +130,10 @@ private: virtual void stop_listening_for_console_messages(DevTools::TabDescription const&) const override; virtual void request_console_messages(DevTools::TabDescription const&, i32) const override; - // ^SettingsObserver - virtual void dns_settings_changed() override; - static Application* s_the; Settings m_settings; + OwnPtr m_settings_observer; BrowserOptions m_browser_options; WebContentOptions m_web_content_options; @@ -156,6 +155,7 @@ private: OwnPtr m_devtools; } SWIFT_IMMORTAL_REFERENCE; + } #define WEB_VIEW_APPLICATION(ApplicationType) \ diff --git a/Libraries/LibWebView/Settings.cpp b/Libraries/LibWebView/Settings.cpp index fdf69a04efe..65fb8e3c0cf 100644 --- a/Libraries/LibWebView/Settings.cpp +++ b/Libraries/LibWebView/Settings.cpp @@ -142,34 +142,6 @@ Settings Settings::create(Badge) return settings; } -DNSSettings Settings::parse_dns_settings(JsonValue const& dns_settings) -{ - if (!dns_settings.is_object()) - return SystemDNS {}; - - auto& dns_settings_object = dns_settings.as_object(); - auto mode = dns_settings_object.get_string("mode"sv); - if (mode.has_value()) { - if (*mode == "system"sv) - return SystemDNS {}; - if (*mode == "custom"sv) { - auto server = dns_settings_object.get_string("server"sv); - auto port = dns_settings_object.get_u16("port"sv); - auto type = dns_settings_object.get_string("type"sv); - - if (server.has_value() && port.has_value() && type.has_value()) { - if (*type == "tls"sv) - return DNSOverTLS { .server_address = server->to_byte_string(), .port = *port }; - if (*type == "udp"sv) - return DNSOverUDP { .server_address = server->to_byte_string(), .port = *port }; - } - } - } - - dbgln("Invalid DNS settings in parse_dns_settings, falling back to system DNS"); - return SystemDNS {}; -} - Settings::Settings(ByteString settings_path) : m_settings_path(move(settings_path)) , m_new_tab_page_url(URL::about_newtab()) @@ -452,6 +424,35 @@ void Settings::set_do_not_track(DoNotTrack do_not_track) observer.do_not_track_changed(); } +DNSSettings Settings::parse_dns_settings(JsonValue const& dns_settings) +{ + if (!dns_settings.is_object()) + return SystemDNS {}; + + auto const& dns_settings_object = dns_settings.as_object(); + + if (auto mode = dns_settings_object.get_string("mode"sv); mode.has_value()) { + if (*mode == "system"sv) + return SystemDNS {}; + + if (*mode == "custom"sv) { + auto server = dns_settings_object.get_string("server"sv); + auto port = dns_settings_object.get_u16("port"sv); + auto type = dns_settings_object.get_string("type"sv); + + if (server.has_value() && port.has_value() && type.has_value()) { + if (*type == "tls"sv) + return DNSOverTLS { .server_address = server->to_byte_string(), .port = *port }; + if (*type == "udp"sv) + return DNSOverUDP { .server_address = server->to_byte_string(), .port = *port }; + } + } + } + + dbgln("Invalid DNS settings in parse_dns_settings, falling back to system DNS"); + return SystemDNS {}; +} + void Settings::set_dns_settings(DNSSettings const& dns_settings, bool override_by_command_line) { m_dns_settings = dns_settings; @@ -485,29 +486,13 @@ void Settings::remove_observer(Badge, SettingsObserver& observ VERIFY(was_removed); } -SettingsObserver::SettingsObserver(AddToObservers add_to_observers) +SettingsObserver::SettingsObserver() { - if (add_to_observers == AddToObservers::Yes) - Settings::add_observer({}, *this); - else - m_registration_was_delayed = true; + Settings::add_observer({}, *this); } SettingsObserver::~SettingsObserver() { - if (!m_registration_was_delayed) - Settings::remove_observer({}, *this); -} - -void SettingsObserver::complete_delayed_registration() -{ - VERIFY(m_registration_was_delayed); - Settings::add_observer({}, *this); -} - -void SettingsObserver::complete_delayed_unregistration() -{ - VERIFY(m_registration_was_delayed); Settings::remove_observer({}, *this); } diff --git a/Libraries/LibWebView/Settings.h b/Libraries/LibWebView/Settings.h index deefe1afb76..2c3f457fc59 100644 --- a/Libraries/LibWebView/Settings.h +++ b/Libraries/LibWebView/Settings.h @@ -32,11 +32,7 @@ enum class DoNotTrack { class SettingsObserver { public: - enum class AddToObservers { - No, - Yes, - }; - explicit SettingsObserver(AddToObservers = AddToObservers::Yes); + explicit SettingsObserver(); virtual ~SettingsObserver(); virtual void new_tab_page_url_changed() { } @@ -46,13 +42,6 @@ public: virtual void autoplay_settings_changed() { } virtual void do_not_track_changed() { } virtual void dns_settings_changed() { } - -protected: - void complete_delayed_registration(); - void complete_delayed_unregistration(); - -private: - bool m_registration_was_delayed { false }; }; class Settings {