[Extensions] Don't allow setting an opener outside of the same window

Extensions can set a tab's opener (as seen by the tab strip model) using
different APIs. However, the TabStripModel only updates openers (such as
when the tab is removed) for tabs that it contains. This meant that if
an extension set an opener to a tab that wasn't within the
TabStripModel, subsequent queries or events would fail when we tried to
look up the opener id.

Don't allow extensions to set an opener outside of the owning
TabStripModel.

Note: this is intentionally a small change, which unfortunately means we
aren't handling all cases as gracefully as possible. However, it's low-
risk, and should stop us from crashing.

BUG=698681

Review-Url: https://codereview.chromium.org/2737103002
Cr-Commit-Position: refs/heads/master@{#455355}
(cherry picked from commit 5b3827b)

Review-Url: https://codereview.chromium.org/2740763002 .
Cr-Commit-Position: refs/branch-heads/3029@{#67}
Cr-Branched-From: 939b32e-refs/heads/master@{#454471}

chrome/browser/extensions/api/tabs/tabs_api.cc

@@ -1262,6 +1262,11 @@ bool TabsUpdateFunction::RunAsync() {
                                       &opener_contents, nullptr))
       return false;
 
+    if (tab_strip->GetIndexOfWebContents(opener_contents) ==
+        TabStripModel::kNoTab) {
+      error_ = "Tab opener must be in the same window as the updated tab.";
+      return false;
+    }
     tab_strip->SetOpenerOfWebContentsAt(tab_index, opener_contents);
   }
 

chrome/browser/extensions/extension_tab_util.cc

@@ -254,8 +254,14 @@ base::DictionaryValue* ExtensionTabUtil::OpenTab(
   tab_strip = navigate_params.browser->tab_strip_model();
   int new_index =
       tab_strip->GetIndexOfWebContents(navigate_params.target_contents);
-  if (opener)
-    tab_strip->SetOpenerOfWebContentsAt(new_index, opener);
+  if (opener) {
+    // Only set the opener if the opener tab is in the same tab strip as the
+    // new tab.
+    // TODO(devlin): We should be a) catching this sooner and b) alerting that
+    // this failed by reporting an error.
+    if (tab_strip->GetIndexOfWebContents(opener) != TabStripModel::kNoTab)
+      tab_strip->SetOpenerOfWebContentsAt(new_index, opener);
+  }
 
   if (active)
     navigate_params.target_contents->SetInitialFocus();

chrome/browser/extensions/extension_tabs_apitest.cc

@@ -273,6 +273,11 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, OnUpdatedDiscardedState) {
   ASSERT_TRUE(RunExtensionSubtest("tabs/basics", "discarded.html")) << message_;
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabOpenerCraziness) {
+  ASSERT_TRUE(StartEmbeddedTestServer());
+  ASSERT_TRUE(RunExtensionTest("tabs/tab_opener_id"));
+}
+
 // Adding a new test? Awesome. But API tests are the old hotness. The new
 // hotness is extension_function_test_utils. See tabs_test.cc for an example.
 // We are trying to phase out many uses of API tests as they tend to be flaky.

chrome/test/data/extensions/api_test/tabs/tab_opener_id/background.js

@@ -0,0 +1,54 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Add a listener just so we dispatch onUpdated events.
+chrome.tabs.onUpdated.addListener(function(update) {});
+
+// Test a series of crazy events where we can set a tab's opener, then close it,
+// and hope the browser does the right thing.
+// Regression test for crbug.com/698681.
+chrome.test.getConfig((config) => {
+  var getUrl = function(url) {
+    return 'http://localhost:' + config.testServer.port + '/' + url;
+  };
+
+  var url1 = getUrl('title1.html');
+  var url2 = getUrl('title2.html');
+  var url3 = getUrl('title3.html');
+
+  // Create a new window with two tabs (url1 and url2).
+  chrome.windows.create({url: [url1, url2]}, (win) => {
+    chrome.test.assertNoLastError();
+    chrome.test.assertTrue(!!win);
+    chrome.test.assertTrue(!!win.tabs);
+    chrome.test.assertEq(2, win.tabs.length);
+    var openerId = win.tabs[0].id;
+
+    // Create a second window with a single tab (url3).
+    chrome.windows.create({url: url3}, (secondWin) => {
+      chrome.test.assertNoLastError();
+      chrome.test.assertTrue(!!secondWin);
+      chrome.test.assertTrue(!!secondWin.tabs);
+      chrome.test.assertEq(1, secondWin.tabs.length);
+
+      // Try to update the tab in the second window to have an opener of a tab
+      // in the first window. This *should* fail.
+      chrome.tabs.update(secondWin.tabs[0].id, {openerTabId: openerId}, () => {
+        chrome.test.assertLastError(
+            'Tab opener must be in the same window as the updated tab.');
+
+        // Next, remove the tab we tried to set as the opener.
+        chrome.tabs.remove(openerId, () => {
+          chrome.test.assertNoLastError();
+
+          // And finally, query the tabs.
+          chrome.tabs.query({}, function(tabs) {
+            chrome.test.assertNoLastError();
+            chrome.test.succeed();
+          });
+        });
+      });
+    });
+  });
+});

chrome/test/data/extensions/api_test/tabs/tab_opener_id/manifest.json

@@ -0,0 +1,11 @@
+{
+  "name": "foo",
+  "description": "foo",
+  "version": "0.1",
+  "manifest_version": 2,
+  "permissions": ["tabs"],
+  "background": {
+    "persistent": true,
+    "scripts": ["background.js"]
+  }
+}