[history] Fix regression in Open All for History Tabs from Other Devices

In this CL, I fix a security issue, but caused a functionality
regression:
https://chromium-review.googlesource.com/c/chromium/src/+/4226314

This CL keeps the security fix, but restores the functionality.

It turns out the `window_num` argument wasn't used at all anyways.

When opening a single tab: It was ignored.
When opening all tabs: The argument never existed, so it was always -1.

This CL just deletes the argument everywhere, and also fixes the
functionality.

I manually tested it.

Bug: 1418862, 1408120
Change-Id: I778da7aa311881b340bceb2bf7ae20ab7692ba15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4544909
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1146757}

chrome/browser/resources/history/browser_service.ts

@@ -26,8 +26,7 @@ export interface BrowserService {
   removeBookmark(url: string): void;
   removeVisits(removalList: RemoveVisitsRequest): Promise<void>;
   openForeignSessionAllTabs(sessionTag: string): void;
-  openForeignSessionTab(
-      sessionTag: string, windowId: number, tabId: number, e: MouseEvent): void;
+  openForeignSessionTab(sessionTag: string, tabId: number, e: MouseEvent): void;
   deleteForeignSession(sessionTag: string): void;
   openClearBrowsingData(): void;
   recordHistogram(histogram: string, value: number, max: number): void;
@@ -59,14 +58,12 @@ export class BrowserServiceImpl implements BrowserService {
   }
 
   openForeignSessionAllTabs(sessionTag: string) {
-    chrome.send('openForeignSession', [sessionTag]);
+    chrome.send('openForeignSessionAllTabs', [sessionTag]);
   }
 
-  openForeignSessionTab(
-      sessionTag: string, windowId: number, tabId: number, e: MouseEvent) {
-    chrome.send('openForeignSession', [
+  openForeignSessionTab(sessionTag: string, tabId: number, e: MouseEvent) {
+    chrome.send('openForeignSessionTab', [
       sessionTag,
-      String(windowId),
       String(tabId),
       e.button || 0,
       e.altKey,

chrome/browser/resources/history/synced_device_card.ts

@@ -123,8 +123,7 @@ export class HistorySyncedDeviceCardElement extends PolymerElement {
     browserService.recordHistogram(
         SYNCED_TABS_HISTOGRAM_NAME, SyncedTabsHistogram.LINK_CLICKED,
         SyncedTabsHistogram.LIMIT);
-    browserService.openForeignSessionTab(
-        this.sessionTag, tab.windowId, tab.sessionId, e);
+    browserService.openForeignSessionTab(this.sessionTag, tab.sessionId, e);
     e.preventDefault();
   }
 

chrome/browser/ui/webui/history/foreign_session_handler.cc

@@ -151,8 +151,7 @@ void ForeignSessionHandler::OpenForeignSessionTab(
 // static
 void ForeignSessionHandler::OpenForeignSessionWindows(
     content::WebUI* web_ui,
-    const std::string& session_string_value,
-    size_t window_num) {
+    const std::string& session_string_value) {
   sync_sessions::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate(web_ui);
   if (!open_tabs)
     return;
@@ -165,14 +164,8 @@ void ForeignSessionHandler::OpenForeignSessionWindows(
     return;
   }
 
-  // Bounds check `window_num` before using it anywhere. crbug.com/1408120
-  CHECK_LT(window_num, windows.size());
-  auto iter_begin = windows.begin() + window_num;
-  DCHECK(iter_begin != windows.end())
-      << "Because we CHECKed that windows_num is less than the size.";
-  auto iter_end = iter_begin + 1;
   SessionRestore::RestoreForeignSessionWindows(Profile::FromWebUI(web_ui),
-                                               iter_begin, iter_end);
+                                               windows.begin(), windows.end());
 }
 
 // static
@@ -194,8 +187,13 @@ void ForeignSessionHandler::RegisterMessages() {
       base::BindRepeating(&ForeignSessionHandler::HandleGetForeignSessions,
                           base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
-      "openForeignSession",
-      base::BindRepeating(&ForeignSessionHandler::HandleOpenForeignSession,
+      "openForeignSessionAllTabs",
+      base::BindRepeating(
+          &ForeignSessionHandler::HandleOpenForeignSessionAllTabs,
+          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "openForeignSessionTab",
+      base::BindRepeating(&ForeignSessionHandler::HandleOpenForeignSessionTab,
                           base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "setForeignSessionCollapsed",
@@ -317,52 +315,46 @@ base::Value::List ForeignSessionHandler::GetForeignSessions() {
   return session_list;
 }
 
-void ForeignSessionHandler::HandleOpenForeignSession(
+void ForeignSessionHandler::HandleOpenForeignSessionAllTabs(
     const base::Value::List& args) {
-  size_t num_args = args.size();
-  // Expect either 1 or 8 args. For restoring an entire session, only
-  // one argument is required -- the session tag. To restore a tab,
-  // the additional args required are the window id, the tab id,
-  // and 4 properties of the event object (button, altKey, ctrlKey,
-  // metaKey, shiftKey) for determining how to open the tab.
-  if (num_args != 8U && num_args != 1U) {
-    LOG(ERROR) << "openForeignSession called with " << args.size()
-               << " arguments.";
-    return;
-  }
+  CHECK_EQ(args.size(), 1U);
 
   // Extract the session tag (always provided).
   if (!args[0].is_string()) {
     LOG(ERROR) << "Failed to extract session tag.";
     return;
   }
   const std::string& session_string_value = args[0].GetString();
+  OpenForeignSessionWindows(web_ui(), session_string_value);
+}
 
-  // Extract window number.
-  size_t window_num = 0;
-  if (num_args >= 2 &&
-      (!args[1].is_string() ||
-       !base::StringToSizeT(args[1].GetString(), &window_num))) {
-    LOG(ERROR) << "Failed to extract window number.";
+void ForeignSessionHandler::HandleOpenForeignSessionTab(
+    const base::Value::List& args) {
+  CHECK_EQ(args.size(), 7U);
+
+  // Extract the session tag (always provided).
+  if (!args[0].is_string()) {
+    LOG(ERROR) << "Failed to extract session tag.";
     return;
   }
+  const std::string& session_string_value = args[0].GetString();
 
   // Extract tab id.
   SessionID::id_type tab_id_value = 0;
-  if (num_args >= 3 &&
-      (!args[2].is_string() ||
-       !base::StringToInt(args[2].GetString(), &tab_id_value))) {
+  if (!args[1].is_string() ||
+      !base::StringToInt(args[1].GetString(), &tab_id_value)) {
     LOG(ERROR) << "Failed to extract tab SessionID.";
     return;
   }
 
   SessionID tab_id = SessionID::FromSerializedValue(tab_id_value);
-  if (tab_id.is_valid()) {
-    WindowOpenDisposition disposition = webui::GetDispositionFromClick(args, 3);
-    OpenForeignSessionTab(web_ui(), session_string_value, tab_id, disposition);
-  } else {
-    OpenForeignSessionWindows(web_ui(), session_string_value, window_num);
+  if (!tab_id.is_valid()) {
+    LOG(ERROR) << "Failed to deserialize tab ID.";
+    return;
   }
+
+  WindowOpenDisposition disposition = webui::GetDispositionFromClick(args, 2);
+  OpenForeignSessionTab(web_ui(), session_string_value, tab_id, disposition);
 }
 
 void ForeignSessionHandler::HandleDeleteForeignSession(

chrome/browser/ui/webui/history/foreign_session_handler.h

@@ -68,9 +68,9 @@ class ForeignSessionHandler : public content::WebUIMessageHandler {
                                     SessionID tab_id,
                                     const WindowOpenDisposition& disposition);
 
-  static void OpenForeignSessionWindows(content::WebUI* web_ui,
-                                        const std::string& session_string_value,
-                                        size_t window_num);
+  static void OpenForeignSessionWindows(
+      content::WebUI* web_ui,
+      const std::string& session_string_value);
 
   // Returns a pointer to the current session model associator or nullptr.
   static sync_sessions::OpenTabsUIDelegate* GetOpenTabsUIDelegate(
@@ -79,17 +79,24 @@ class ForeignSessionHandler : public content::WebUIMessageHandler {
   void SetWebUIForTesting(content::WebUI* web_ui) { set_web_ui(web_ui); }
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(ForeignSessionHandlerTest,
+                           HandleOpenForeignSessionAllTabs);
+  FRIEND_TEST_ALL_PREFIXES(ForeignSessionHandlerTest,
+                           HandleOpenForeignSessionTab);
+
   void OnForeignSessionUpdated();
 
   base::Value::List GetForeignSessions();
 
   // Returns a string used to show the user when a session was last modified.
   std::u16string FormatSessionTime(const base::Time& time);
 
-  // Determines which session is to be opened, and then calls
-  // OpenForeignSession, to begin the process of opening a new browser window.
-  // This is a javascript callback handler.
-  void HandleOpenForeignSession(const base::Value::List& args);
+  // Opens all the tabs of a foreign session, potentially spanning multiple
+  // windows. This as a javascript callback handler.
+  void HandleOpenForeignSessionAllTabs(const base::Value::List& args);
+
+  // Opens a single foreign session tab. This is a javascript callback handler.
+  void HandleOpenForeignSessionTab(const base::Value::List& args);
 
   // Determines whether foreign sessions should be obtained from the sync model.
   // This is a javascript callback handler, and it is also called when the sync

chrome/browser/ui/webui/history/foreign_session_handler_unittest.cc

@@ -7,13 +7,40 @@
 #include "base/callback_list.h"
 #include "chrome/browser/sync/session_sync_service_factory.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "components/sessions/core/session_id.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "content/public/test/test_web_ui.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace browser_sync {
 
-namespace {
+class MockOpenTabsUIDelegate : public sync_sessions::OpenTabsUIDelegate {
+ public:
+  MockOpenTabsUIDelegate() = default;
+
+  MOCK_METHOD1(
+      GetAllForeignSessions,
+      bool(std::vector<const sync_sessions::SyncedSession*>* sessions));
+
+  MOCK_METHOD3(GetForeignTab,
+               bool(const std::string& tag,
+                    const SessionID tab_id,
+                    const sessions::SessionTab** tab));
+
+  MOCK_METHOD1(DeleteForeignSession, void(const std::string& tag));
+
+  MOCK_METHOD2(GetForeignSession,
+               bool(const std::string& tag,
+                    std::vector<const sessions::SessionWindow*>* windows));
+
+  MOCK_METHOD2(GetForeignSessionTabs,
+               bool(const std::string& tag,
+                    std::vector<const sessions::SessionTab*>* tabs));
+
+  MOCK_METHOD1(GetLocalSession,
+               bool(const sync_sessions::SyncedSession** local_session));
+};
 
 // Partial SessionSyncService that can fake behavior for
 // SubscribeToForeignSessionsChanged() including the notification to
@@ -28,8 +55,8 @@ class FakeSessionSyncService : public sync_sessions::SessionSyncService {
   // SessionSyncService overrides.
   syncer::GlobalIdMapper* GetGlobalIdMapper() const override { return nullptr; }
 
-  sync_sessions::OpenTabsUIDelegate* GetOpenTabsUIDelegate() override {
-    return nullptr;
+  MockOpenTabsUIDelegate* GetOpenTabsUIDelegate() override {
+    return &mock_open_tabs_ui_delegate_;
   }
 
   base::CallbackListSubscription SubscribeToForeignSessionsChanged(
@@ -47,6 +74,7 @@ class FakeSessionSyncService : public sync_sessions::SessionSyncService {
 
  private:
   base::RepeatingClosureList subscriber_list_;
+  MockOpenTabsUIDelegate mock_open_tabs_ui_delegate_;
 };
 
 class ForeignSessionHandlerTest : public ChromeRenderViewHostTestHarness {
@@ -129,6 +157,31 @@ TEST_F(ForeignSessionHandlerTest,
   EXPECT_EQ(0U, web_ui()->call_data().size());
 }
 
-}  // namespace
+TEST_F(ForeignSessionHandlerTest, HandleOpenForeignSessionAllTabs) {
+  EXPECT_CALL(*session_sync_service()->GetOpenTabsUIDelegate(),
+              GetForeignSession("my_session_tag", testing::_))
+      .Times(testing::AtLeast(1));
+
+  base::Value::List list_args;
+  list_args.Append("my_session_tag");
+  handler()->HandleOpenForeignSessionAllTabs(list_args);
+}
+
+TEST_F(ForeignSessionHandlerTest, HandleOpenForeignSessionTab) {
+  EXPECT_CALL(*session_sync_service()->GetOpenTabsUIDelegate(),
+              GetForeignTab("my_session_tag",
+                            SessionID::FromSerializedValue(456), testing::_))
+      .Times(testing::AtLeast(1));
+
+  base::Value::List list_args;
+  list_args.Append("my_session_tag");
+  list_args.Append("456");
+  list_args.Append(1.0);
+  list_args.Append(false);
+  list_args.Append(false);
+  list_args.Append(false);
+  list_args.Append(false);
+  handler()->HandleOpenForeignSessionTab(list_args);
+}
 
 }  // namespace browser_sync

chrome/test/data/webui/history/history_synced_tabs_test.ts

@@ -264,7 +264,6 @@ suite('<history-synced-device-manager>', function() {
         })
         .then(args => {
           assertEquals('Chromebook', args.sessionTag, 'sessionTag is correct');
-          assertEquals(123, args.windowId, 'windowId is correct');
           assertEquals(456, args.tabId, 'tabId is correct');
           assertFalse(args.e.altKey, 'altKey is defined');
           assertFalse(args.e.ctrlKey, 'ctrlKey is defined');

chrome/test/data/webui/history/test_browser_service.ts

@@ -101,11 +101,9 @@ export class TestBrowserService extends TestBrowserProxy implements
 
   openForeignSessionAllTabs() {}
 
-  openForeignSessionTab(
-      sessionTag: string, windowId: number, tabId: number, e: MouseEvent) {
+  openForeignSessionTab(sessionTag: string, tabId: number, e: MouseEvent) {
     this.methodCalled('openForeignSessionTab', {
       sessionTag: sessionTag,
-      windowId: windowId,
       tabId: tabId,
       e: e,
     });