From e97d4edaae46e5bce1a00c0ac5a67859edfb155d Mon Sep 17 00:00:00 2001
From: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Fri, 29 Jun 2018 01:38:50 +0000
Subject: [PATCH] [Extensions Bindings] Gracefully handle invalidated contexts
 in app hooks

Handle the case of a context being invalidated before accessing
chrome.app.isInstalled. This can happen in cases where the owning frame
is removed.

Add a regression test for the same.

Bug: 855853
Change-Id: Ic9e5f5a45fc2a833c5c1a45d3b7c1e8c04c71a18
Reviewed-on: https://chromium-review.googlesource.com/1119066
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571355}
---
 .../extensions/chrome_app_api_browsertest.cc  | 28 +++++++++++++++++++
 .../renderer/extensions/app_hooks_delegate.cc |  7 ++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/chrome/browser/extensions/chrome_app_api_browsertest.cc b/chrome/browser/extensions/chrome_app_api_browsertest.cc
index db24543bc13032..02436f38cb0411 100644
--- a/chrome/browser/extensions/chrome_app_api_browsertest.cc
+++ b/chrome/browser/extensions/chrome_app_api_browsertest.cc
@@ -171,6 +171,34 @@ IN_PROC_BROWSER_TEST_F(ChromeAppAPITest, IsInstalled) {
   EXPECT_EQ("true", result);
 }
 
+// Test accessing app.isInstalled when the context has been invalidated (e.g.
+// by removing the frame). Regression test for https://crbug.com/855853.
+IN_PROC_BROWSER_TEST_F(ChromeAppAPITest, IsInstalledFromRemovedFrame) {
+  GURL app_url =
+      embedded_test_server()->GetURL("app.com", "/extensions/test_file.html");
+  const Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("app_dot_com_app"));
+  ASSERT_TRUE(extension);
+  ui_test_utils::NavigateToURL(browser(), app_url);
+
+  constexpr char kScript[] =
+      R"(var i = document.createElement('iframe');
+         i.onload = function() {
+           var frameApp = i.contentWindow.chrome.app;
+           document.body.removeChild(i);
+           var isInstalled = frameApp.isInstalled;
+           window.domAutomationController.send(
+               isInstalled === undefined);
+         };
+         i.src = '%s';
+         document.body.appendChild(i);)";
+  bool result = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      base::StringPrintf(kScript, app_url.spec().c_str()), &result));
+  EXPECT_TRUE(result);
+}
+
 IN_PROC_BROWSER_TEST_F(ChromeAppAPITest, InstallAndRunningState) {
   GURL app_url = embedded_test_server()->GetURL(
       "app.com", "/extensions/get_app_details_for_frame.html");
diff --git a/chrome/renderer/extensions/app_hooks_delegate.cc b/chrome/renderer/extensions/app_hooks_delegate.cc
index 4abb1475054ed1..58f7b73bc26f81 100644
--- a/chrome/renderer/extensions/app_hooks_delegate.cc
+++ b/chrome/renderer/extensions/app_hooks_delegate.cc
@@ -21,7 +21,12 @@ void IsInstalledGetterCallback(
   v8::Local<v8::Context> context = info.Holder()->CreationContext();
   ScriptContext* script_context =
       ScriptContextSet::GetContextByV8Context(context);
-  DCHECK(script_context);
+
+  // The ScriptContext may have been invalidated if e.g. the frame was removed.
+  // Return undefined in this case.
+  if (!script_context)
+    return;
+
   auto* core =
       static_cast<AppBindingsCore*>(info.Data().As<v8::External>()->Value());
   // Since this is more-or-less an API, log it as an API call.