[SessionService] Adding debug information & fixing app reparenting.

This fixes the app reparenting issue introduced by this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/5899682

That CL deleted some session restore stuff that seemed unnecessary.
Unfortunately there are no tests in this area. So this CL adds that
logic back due to the below bug being found.

This CL also adds a lot more helpful information to
chrome://internals/session-service.

Bug: 373461941
Change-Id: I701e0588c02a2ef5beb59516ad5ffc79dc93f585
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5938537
Auto-Submit: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369633}

chrome/browser/sessions/session_service_base.cc

@@ -14,6 +14,7 @@
 #include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
+#include "base/strings/to_string.h"
 #include "base/task/sequenced_task_runner.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
@@ -35,6 +36,7 @@
 #include "components/sessions/core/command_storage_manager.h"
 #include "components/sessions/core/session_command.h"
 #include "components/sessions/core/session_constants.h"
+#include "components/sessions/core/session_id.h"
 #include "components/sessions/core/session_types.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
@@ -466,6 +468,37 @@ void SessionServiceBase::TabNavigationPathEntriesDeleted(SessionID window_id,
   command_storage_manager_->StartSaveTimer();
 }
 
+#if DCHECK_IS_ON()
+base::Value SessionServiceBase::ToDebugValue() const {
+  base::Value::Dict result;
+  result.Set("profile", base::ToString(profile_.get()));
+  if (command_storage_manager_) {
+    result.Set("command_storage_manager",
+               command_storage_manager_->ToDebugValue());
+  }
+  for (const auto& [id, range] : tab_to_available_range_) {
+    base::Value::Dict* range_dict =
+        result.EnsureDict("tab_to_available_range")
+            ->EnsureDict(base::NumberToString(id.id()));
+    range_dict->Set("first", range.first);
+    range_dict->Set("second", range.first);
+  }
+  result.Set("rebuild_on_next_save", rebuild_on_next_save_);
+  for (const auto& [id, tab_index] : last_selected_tab_in_window_) {
+    result.EnsureDict("last_selected_tab_in_window")
+        ->Set(base::NumberToString(id.id()), tab_index);
+  }
+  for (const SessionID& window_tracking : windows_tracking_) {
+    result.EnsureList("windows_tracking")
+        ->Append(base::NumberToString(window_tracking.id()));
+  }
+  result.Set("is_saving_enabled", is_saving_enabled_);
+  result.Set("did_save_commands_at_least_once",
+             did_save_commands_at_least_once_);
+  return base::Value(std::move(result));
+}
+#endif  // DCHECK_IS_ON()
+
 void SessionServiceBase::DestroyCommandStorageManager() {
   command_storage_manager_.reset(nullptr);
 }

chrome/browser/sessions/session_service_base.h

@@ -31,6 +31,10 @@
 
 class Profile;
 
+namespace base {
+class Value;
+}
+
 namespace content {
 class WebContents;
 }  // namespace content
@@ -170,6 +174,14 @@ class SessionServiceBase : public sessions::CommandStorageManagerDelegate,
   void TabNavigationPathEntriesDeleted(SessionID window_id,
                                        SessionID tab_id) override;
 
+#if DCHECK_IS_ON()
+  // Returns the state of this class and logs for the
+  // chrome://internals/session-service debug page. The logs are in reverse
+  // order for truncation ease. This value is NOT STABLE -
+  // do not rely on it's contents for anything.
+  virtual base::Value ToDebugValue() const;
+#endif  // DCHECK_IS_ON()
+
  protected:
   // Creates a SessionService for the specified profile.
   SessionServiceBase(Profile* profile, SessionServiceType type);

chrome/browser/ui/web_applications/web_app_launch_utils.cc

@@ -316,6 +316,13 @@ void ReparentWebContentsIntoBrowserImpl(Browser* source_browser,
   CHECK(source_browser);
   CHECK(web_contents);
   CHECK(target_browser);
+
+  // In a reparent, the owning session service needs to be told it's tab
+  // has been removed, otherwise it will reopen the tab on restoration.
+  SessionServiceBase* service =
+      GetAppropriateSessionServiceForProfile(source_browser);
+  service->TabClosing(web_contents);
+
   // Check-fail if the web contents given is not part of the source browser.
   std::optional<int> found_tab_index;
   for (int i = 0; i < source_browser->tab_strip_model()->count(); ++i) {
@@ -376,6 +383,13 @@ void ReparentWebContentsIntoBrowserImpl(Browser* source_browser,
   }
 #endif
   target_browser->window()->Show();
+
+  // The window will be registered correctly, however the tab will not be
+  // correctly tracked. We need to do a reset to get the tab correctly tracked
+  // by either the app service or the regular service
+  SessionServiceBase* target_service =
+      GetAppropriateSessionServiceForProfile(target_browser);
+  target_service->ResetFromCurrentBrowsers();
 }
 
 std::optional<webapps::AppId> GetWebAppForActiveTab(const Browser* browser) {

chrome/browser/ui/webui/internals/sessions/session_service_internals_handler.cc

@@ -12,7 +12,11 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
+#include "base/values.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sessions/app_session_service.h"
+#include "chrome/browser/sessions/app_session_service_factory.h"
+#include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/sessions/session_service_log.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/common/webui_url_constants.h"
@@ -67,14 +71,31 @@ std::string EventToString(const SessionServiceEvent& event) {
   }
 }
 
-std::string GetEventLogAsString(Profile* profile) {
+std::string GetSessionServiceInternalsAsString(Profile* profile) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  std::vector<std::string> results;
-  results.push_back("<pre>");
-  for (const auto& event : GetSessionServiceEvents(profile))
-    results.push_back(EventToString(event));
-  results.push_back("</pre>");
-  return base::JoinString(results, "\n");
+  std::vector<std::string> events;
+  events.push_back("<pre>");
+  for (const auto& event : GetSessionServiceEvents(profile)) {
+    events.push_back(EventToString(event));
+  }
+  events.push_back("</pre>");
+
+  base::Value::Dict internals_output;
+#if DCHECK_IS_ON()
+  internals_output.Set(
+      "AppSessionService",
+      AppSessionServiceFactory::GetForProfile(profile)->ToDebugValue());
+  internals_output.Set(
+      "SessionService",
+      SessionServiceFactory::GetForProfile(profile)->ToDebugValue());
+#else
+  internals_output.Set("",
+                       "Build with DCHECKs enabled to see information here");
+#endif  // DCHECK_IS_ON()
+  return base::StrCat({"<h3>Events:</h3>\n<pre>\n",
+                       base::JoinString(events, "\n"),
+                       "</pre>\n<h3>Service States</h3>\n<pre>\n",
+                       internals_output.DebugString(), "</pre>\n"});
 }
 
 }  // namespace
@@ -93,5 +114,5 @@ void SessionServiceInternalsHandler::HandleWebUIRequestCallback(
   DCHECK(ShouldHandleWebUIRequestCallback(path));
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   std::move(callback).Run(base::MakeRefCounted<base::RefCountedString>(
-      GetEventLogAsString(profile)));
+      GetSessionServiceInternalsAsString(profile)));
 }

components/sessions/core/command_storage_manager.cc

@@ -4,6 +4,7 @@
 
 #include "components/sessions/core/command_storage_manager.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/functional/bind.h"
@@ -14,8 +15,10 @@
 #include "base/task/single_thread_task_runner.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/thread.h"
+#include "base/values.h"
 #include "components/sessions/core/command_storage_backend.h"
 #include "components/sessions/core/command_storage_manager_delegate.h"
+#include "components/sessions/core/session_command.h"
 #include "crypto/random.h"
 
 namespace sessions {
@@ -31,6 +34,14 @@ void AdaptGetLastSessionCommands(
   std::move(callback).Run(std::move(result.commands), result.error_reading);
 }
 
+#if DCHECK_IS_ON()
+base::Value CommandToDebugValue(const SessionCommand& command) {
+  // There is no convenience function to transform the pickled contents into the
+  // payload struct itself, so just output the id for now.
+  return base::Value(command.id());
+}
+#endif  // DCHECK_IS_ON()
+
 }  // namespace
 
 CommandStorageManager::CommandStorageManager(
@@ -133,6 +144,16 @@ void CommandStorageManager::Save() {
   if (pending_commands_.empty())
     return;
 
+#if DCHECK_IS_ON()
+  for (const std::unique_ptr<SessionCommand>& command : pending_commands_) {
+    written_commands_reverse_debug_log_.push_front(
+        CommandToDebugValue(*command));
+  }
+  if (written_commands_reverse_debug_log_.size() > kMaxLogSize) {
+    written_commands_reverse_debug_log_.resize(kMaxLogSize);
+  }
+#endif  // DCHECK_IS_ON()
+
   std::vector<uint8_t> crypto_key;
   if (use_crypto_ && pending_reset_) {
     crypto_key = CreateCryptoKey();
@@ -178,6 +199,24 @@ void CommandStorageManager::GetLastSessionCommands(
       base::BindOnce(&AdaptGetLastSessionCommands, std::move(callback)));
 }
 
+#if DCHECK_IS_ON()
+base::Value CommandStorageManager::ToDebugValue() const {
+  base::Value::Dict debug_value;
+  debug_value.Set("use_crypto", use_crypto_);
+  for (const std::unique_ptr<SessionCommand>& command : pending_commands_) {
+    debug_value.EnsureList("pending_commands")
+        ->Append(CommandToDebugValue(*command));
+  }
+  debug_value.Set("pending_reset", pending_reset_);
+  debug_value.Set("commands_since_reset", commands_since_reset_);
+  for (const base::Value& log_item : written_commands_reverse_debug_log_) {
+    debug_value.EnsureList("written_commands_reverse_debug_log")
+        ->Append(log_item.Clone());
+  }
+  return base::Value(std::move(debug_value));
+}
+#endif  // DCHECK_IS_ON()
+
 void CommandStorageManager::OnErrorWritingToFile() {
   delegate_->OnErrorWritingSessionCommands();
 }

components/sessions/core/command_storage_manager.h

@@ -7,6 +7,7 @@
 
 #include <stddef.h>
 
+#include <list>
 #include <memory>
 #include <vector>
 
@@ -18,6 +19,10 @@
 #include "base/task/sequenced_task_runner.h"
 #include "components/sessions/core/sessions_export.h"
 
+namespace base {
+class Value;
+}
+
 namespace sessions {
 class CommandStorageManagerDelegate;
 class SessionCommand;
@@ -130,6 +135,14 @@ class SESSIONS_EXPORT CommandStorageManager {
   // deleted.
   void GetLastSessionCommands(GetCommandsCallback callback);
 
+#if DCHECK_IS_ON()
+  // Returns the state of this class and logs for the
+  // chrome://internals/session-service debug page. The logs are in reverse
+  // order for truncation ease. This value is NOT STABLE - do not rely on it's
+  // contents for anything.
+  base::Value ToDebugValue() const;
+#endif  // DCHECK_IS_ON()
+
  private:
   friend class CommandStorageManagerTestHelper;
 
@@ -160,6 +173,12 @@ class SESSIONS_EXPORT CommandStorageManager {
   // all tasks *must* be processed in the order they are scheduled.
   scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
 
+#if DCHECK_IS_ON()
+  // Used to store debug log entries for this command manager.
+  constexpr static int kMaxLogSize = 100;
+  std::list<base::Value> written_commands_reverse_debug_log_;
+#endif  // DCHECK_IS_ON()
+
   base::WeakPtrFactory<CommandStorageManager> weak_factory_{this};
 
   // Used solely for saving after a delay, and not to be used for any other