Cherry pick PR youtube#509: Correct destruction order of NetworkModul…

…e. (youtube#518)

This corrects the destruction order of various classes held in
browser::Application and browser::BrowserModule.

Also move ownership to match lifetime, which in turn fixes destruction
order.

Also remove the obsolete AccountManager without neither members nor
virtual functions nor derived classes.

b/284222421

(cherry picked from commit 44f4844)

Co-authored-by: Jelle Foks <[email protected]>

cobalt/browser/BUILD.gn

@@ -154,7 +154,6 @@ static_library("browser") {
     ":browser_switches",
     ":generated_bindings",
     ":generated_types",
-    "//cobalt/account",
     "//cobalt/audio",
     "//cobalt/base",
     "//cobalt/browser/memory_settings:browser_memory_settings",

cobalt/browser/application.cc

@@ -727,7 +727,6 @@ Application::Application(const base::Closure& quit_closure, bool should_preload,
   unconsumed_deep_link_ = GetInitialDeepLink();
   DLOG(INFO) << "Initial deep link: " << unconsumed_deep_link_;
 
-  storage::StorageManager::Options storage_manager_options;
   network::NetworkModule::Options network_module_options;
   // Create the main components of our browser.
   BrowserModule::Options options;
@@ -764,7 +763,7 @@ Application::Application(const base::Closure& quit_closure, bool should_preload,
 
 #if defined(ENABLE_DEBUG_COMMAND_LINE_SWITCHES)
   if (command_line->HasSwitch(browser::switches::kNullSavegame)) {
-    storage_manager_options.savegame_options.factory =
+    network_module_options.storage_manager_options.savegame_options.factory =
         &storage::SavegameFake::Create;
   }
 
@@ -798,14 +797,16 @@ Application::Application(const base::Closure& quit_closure, bool should_preload,
   } else {
     partition_key = base::GetApplicationKey(initial_url);
   }
-  storage_manager_options.savegame_options.id = partition_key;
+  network_module_options.storage_manager_options.savegame_options.id =
+      partition_key;
 
   base::Optional<std::string> default_key =
       base::GetApplicationKey(GURL(kDefaultURL));
   if (command_line->HasSwitch(
           browser::switches::kForceMigrationForStoragePartitioning) ||
       partition_key == default_key) {
-    storage_manager_options.savegame_options.fallback_to_default_id = true;
+    network_module_options.storage_manager_options.savegame_options
+        .fallback_to_default_id = true;
   }
 
   // User can specify an extra search path entry for files loaded via file://.
@@ -906,15 +907,11 @@ Application::Application(const base::Closure& quit_closure, bool should_preload,
   options.web_module_options.collect_unload_event_time_callback = base::Bind(
       &Application::CollectUnloadEventTimingInfo, base::Unretained(this));
 
-  account_manager_.reset(new account::AccountManager());
-
-  storage_manager_.reset(new storage::StorageManager(storage_manager_options));
-
   cobalt::browser::UserAgentPlatformInfo platform_info;
 
   network_module_.reset(new network::NetworkModule(
       CreateUserAgentString(platform_info), GetClientHintHeaders(platform_info),
-      storage_manager_.get(), &event_dispatcher_, network_module_options));
+      &event_dispatcher_, network_module_options));
 
   AddCrashHandlerAnnotations(platform_info);
 
@@ -938,15 +935,15 @@ Application::Application(const base::Closure& quit_closure, bool should_preload,
                                                      update_check_delay_sec));
   }
 #endif
-  browser_module_.reset(new BrowserModule(
-      initial_url,
-      (should_preload ? base::kApplicationStateConcealed
-                      : base::kApplicationStateStarted),
-      &event_dispatcher_, account_manager_.get(), network_module_.get(),
+  browser_module_.reset(
+      new BrowserModule(initial_url,
+                        (should_preload ? base::kApplicationStateConcealed
+                                        : base::kApplicationStateStarted),
+                        &event_dispatcher_, network_module_.get(),
 #if SB_IS(EVERGREEN)
-      updater_module_.get(),
+                        updater_module_.get(),
 #endif
-      options));
+                        options));
 
   UpdateUserAgent();
 
@@ -1084,6 +1081,8 @@ Application::~Application() {
   event_dispatcher_.RemoveEventCallback(
       base::DateTimeConfigurationChangedEvent::TypeId(),
       on_date_time_configuration_changed_event_callback_);
+  browser_module_.reset();
+  network_module_.reset();
 }
 
 void Application::Start(SbTimeMonotonic timestamp) {

cobalt/browser/application.h

@@ -24,7 +24,6 @@
 #include "base/message_loop/message_loop.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
-#include "cobalt/account/account_manager.h"
 #include "cobalt/base/event_dispatcher.h"
 #include "cobalt/browser/browser_module.h"
 #include "cobalt/browser/memory_tracker/tool.h"
@@ -96,12 +95,6 @@ class Application {
   // A conduit for system events.
   base::EventDispatcher event_dispatcher_;
 
-  // Account manager.
-  std::unique_ptr<account::AccountManager> account_manager_;
-
-  // Storage manager used by the network module below.
-  std::unique_ptr<storage::StorageManager> storage_manager_;
-
   // Sets up the network component for requesting internet resources.
   std::unique_ptr<network::NetworkModule> network_module_;
 

cobalt/browser/browser_module.cc

@@ -214,7 +214,6 @@ renderer::Submission CreateSubmissionFromLayoutResults(
 BrowserModule::BrowserModule(const GURL& url,
                              base::ApplicationState initial_application_state,
                              base::EventDispatcher* event_dispatcher,
-                             account::AccountManager* account_manager,
                              network::NetworkModule* network_module,
 #if SB_IS(EVERGREEN)
                              updater::UpdaterModule* updater_module,
@@ -226,7 +225,6 @@ BrowserModule::BrowserModule(const GURL& url,
       options_(options),
       self_message_loop_(base::MessageLoop::current()),
       event_dispatcher_(event_dispatcher),
-      account_manager_(account_manager),
       is_rendered_(false),
       is_web_module_rendered_(false),
       can_play_type_handler_(media::MediaModule::CreateCanPlayTypeHandler()),
@@ -459,7 +457,17 @@ BrowserModule::~BrowserModule() {
   SbCoreDumpUnregisterHandler(BrowserModule::CoreDumpHandler, this);
 #endif
 
+#if defined(ENABLE_DEBUGGER)
+  if (debug_console_) {
+    lifecycle_observers_.RemoveObserver(debug_console_.get());
+  }
+  debug_console_.reset();
+#endif
+  DestroySplashScreen();
   // Make sure the WebModule is destroyed before the ServiceWorkerRegistry
+  if (web_module_) {
+    lifecycle_observers_.RemoveObserver(web_module_.get());
+  }
   web_module_.reset();
 }
 
@@ -544,7 +552,7 @@ void BrowserModule::Navigate(const GURL& url_reference) {
   // Show a splash screen while we're waiting for the web page to load.
   const ViewportSize viewport_size = GetViewportSize();
 
-  DestroySplashScreen(base::TimeDelta());
+  DestroySplashScreen();
   if (options_.enable_splash_screen_on_reloads ||
       main_web_module_generation_ == 1) {
     base::Optional<std::string> topic = SetSplashScreenTopicFallback(url);
@@ -799,7 +807,7 @@ void BrowserModule::OnRenderTreeProduced(
   if (splash_screen_) {
     if (on_screen_keyboard_show_called_) {
       // Hide the splash screen as quickly as possible.
-      DestroySplashScreen(base::TimeDelta());
+      DestroySplashScreen();
     } else if (!splash_screen_->ShutdownSignaled()) {
       splash_screen_->Shutdown();
     }
@@ -947,7 +955,7 @@ void BrowserModule::OnOnScreenKeyboardShown(
   // Only inject shown events to the main WebModule.
   on_screen_keyboard_show_called_ = true;
   if (splash_screen_ && splash_screen_->ShutdownSignaled()) {
-    DestroySplashScreen(base::TimeDelta());
+    DestroySplashScreen();
   }
   if (web_module_) {
     web_module_->InjectOnScreenKeyboardShownEvent(event->ticket());
@@ -1364,7 +1372,7 @@ void BrowserModule::DestroySplashScreen(base::TimeDelta close_time) {
     }
     splash_screen_layer_->Reset();
     SubmitCurrentRenderTreeToRenderer();
-    splash_screen_.reset(NULL);
+    splash_screen_.reset();
   }
 }
 
@@ -2072,7 +2080,6 @@ scoped_refptr<script::Wrappable> BrowserModule::CreateH5vccCallback(
 #if SB_IS(EVERGREEN)
   h5vcc_settings.updater_module = updater_module_;
 #endif
-  h5vcc_settings.account_manager = account_manager_;
   h5vcc_settings.event_dispatcher = event_dispatcher_;
 
   h5vcc_settings.user_agent_data = settings->context()

cobalt/browser/browser_module.h

@@ -26,7 +26,6 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "base/timer/timer.h"
-#include "cobalt/account/account_manager.h"
 #include "cobalt/base/accessibility_caption_settings_changed_event.h"
 #include "cobalt/base/application_state.h"
 #include "cobalt/base/date_time_configuration_changed_event.h"
@@ -66,7 +65,6 @@
 #include "cobalt/render_tree/resource_provider_stub.h"
 #include "cobalt/renderer/renderer_module.h"
 #include "cobalt/script/array_buffer.h"
-#include "cobalt/storage/storage_manager.h"
 #include "cobalt/system_window/system_window.h"
 #include "cobalt/ui_navigation/scroll_engine/scroll_engine.h"
 #include "cobalt/web/web_settings.h"
@@ -127,7 +125,6 @@ class BrowserModule {
   BrowserModule(const GURL& url,
                 base::ApplicationState initial_application_state,
                 base::EventDispatcher* event_dispatcher,
-                account::AccountManager* account_manager,
                 network::NetworkModule* network_module,
 #if SB_IS(EVERGREEN)
                 updater::UpdaterModule* updater_module,
@@ -342,7 +339,7 @@ class BrowserModule {
   bool TryURLHandlers(const GURL& url);
 
   // Destroys the splash screen, if currently displayed.
-  void DestroySplashScreen(base::TimeDelta close_time);
+  void DestroySplashScreen(base::TimeDelta close_time = base::TimeDelta());
 
   // Called when web module has received window.close().
   void OnWindowClose(base::TimeDelta close_time);
@@ -517,8 +514,6 @@ class BrowserModule {
 
   base::EventDispatcher* event_dispatcher_;
 
-  account::AccountManager* account_manager_;
-
   // Whether the browser module has yet rendered anything. On the very first
   // render, we hide the system splash screen.
   bool is_rendered_;

cobalt/browser/main.cc

@@ -86,6 +86,7 @@ void StopApplication() {
   LOG(INFO) << "Stopping application.";
   delete g_application;
   g_application = NULL;
+  LOG(INFO) << "Application stopped.";
 }
 
 void HandleStarboardEvent(const SbEvent* starboard_event) {

cobalt/dom/user_agent_data_test.cc

@@ -40,11 +40,10 @@ class UserAgentDataTest : public testing::TestWithJavaScript {
 
     // Inject H5vcc interface to make it also accessible via Window
     h5vcc::H5vcc::Settings h5vcc_settings;
-    h5vcc_settings.network_module = NULL;
+    h5vcc_settings.network_module = nullptr;
 #if SB_IS(EVERGREEN)
-    h5vcc_settings.updater_module = NULL;
+    h5vcc_settings.updater_module = nullptr;
 #endif
-    h5vcc_settings.account_manager = NULL;
     h5vcc_settings.event_dispatcher = event_dispatcher();
     h5vcc_settings.user_agent_data = window()->navigator()->user_agent_data();
     h5vcc_settings.global_environment = global_environment();