<webview>: Fix an issue with destroying an opener that has unattached guests.

This CL also adds a test that exercises the code path and makes
sure destroying an opener that has unattached window doesn't
cause any undesired side effects.

If we destroy an opener that had a pending newwindow, the
newwindow's GC would try to setPermission to deny the
newwindow later. But the opener is gone and its guestInstanceId
isn't valid anymore, this would throw a JavaScript exception b/c
we call webViewInternal.setPermission with undefined instance id:
"Uncaught Error: Invocation of form ..."
The fix is to ignore calling webViewInternal.setPermission in this
case.

Note that the added test doesn't necessarily check for the above
exception because GC can be delayed arbitrarily. The test checks
the regular code path.

BUG=406616
Test=See the test added in newwindow/embedder.js for details.
Open a chrome app window that has a <webview>.
Add a newwindow event listener to the <webview>.
Now trigger a new window from the <webview> so the listener
fires.
In the listener, destroy the <webview>.
Every thing should work fine and should not exhibit any bad
behavior. Without this change it would throw a JavaScript
exception.

Review URL: https://codereview.chromium.org/558813002

Cr-Commit-Position: refs/heads/master@{#294053}

chrome/browser/apps/web_view_interactive_browsertest.cc

@@ -37,32 +37,58 @@ using extensions::AppWindow;
 
 class TestGuestViewManager : public extensions::GuestViewManager {
  public:
-  explicit TestGuestViewManager(content::BrowserContext* context) :
-      GuestViewManager(context),
-      web_contents_(NULL) {}
+  explicit TestGuestViewManager(content::BrowserContext* context)
+      : GuestViewManager(context),
+        guest_add_count_(0),
+        guest_remove_count_(0),
+        web_contents_(NULL) {}
 
-  content::WebContents* WaitForGuestCreated() {
+  content::WebContents* WaitForGuestAdded() {
     if (web_contents_)
       return web_contents_;
 
-    message_loop_runner_ = new content::MessageLoopRunner;
-    message_loop_runner_->Run();
+    add_message_loop_runner_ = new content::MessageLoopRunner;
+    add_message_loop_runner_->Run();
     return web_contents_;
   }
 
+  // Waits so that at least |expected_remove_count| guests' creation
+  // has been seen by this manager.
+  void WaitForGuestRemoved(size_t expected_remove_count) {
+    if (guest_remove_count_ >= expected_remove_count)
+      return;
+
+    remove_message_loop_runner_ = new content::MessageLoopRunner;
+    remove_message_loop_runner_->Run();
+  }
+
+  size_t guest_add_count() { return guest_add_count_; }
+
  private:
   // GuestViewManager override:
   virtual void AddGuest(int guest_instance_id,
                         content::WebContents* guest_web_contents) OVERRIDE{
     GuestViewManager::AddGuest(guest_instance_id, guest_web_contents);
     web_contents_ = guest_web_contents;
+    ++guest_add_count_;
 
-    if (message_loop_runner_.get())
-      message_loop_runner_->Quit();
+    if (add_message_loop_runner_.get())
+      add_message_loop_runner_->Quit();
   }
 
+  virtual void RemoveGuest(int guest_instance_id) OVERRIDE {
+    GuestViewManager::RemoveGuest(guest_instance_id);
+    ++guest_remove_count_;
+
+    if (remove_message_loop_runner_.get())
+      remove_message_loop_runner_->Quit();
+  }
+
+  size_t guest_add_count_;
+  size_t guest_remove_count_;
   content::WebContents* web_contents_;
-  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  scoped_refptr<content::MessageLoopRunner> add_message_loop_runner_;
+  scoped_refptr<content::MessageLoopRunner> remove_message_loop_runner_;
 };
 
 // Test factory for creating test instances of GuestViewManager.
@@ -258,7 +284,7 @@ class WebViewInteractiveTest
     ASSERT_TRUE(done_listener);
     ASSERT_TRUE(done_listener->WaitUntilSatisfied());
 
-    guest_web_contents_ = GetGuestViewManager()->WaitForGuestCreated();
+    guest_web_contents_ = GetGuestViewManager()->WaitForGuestAdded();
   }
 
   void RunTest(const std::string& app_name) {
@@ -732,6 +758,13 @@ IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, EditCommandsNoMenu) {
   ASSERT_TRUE(start_of_line_listener.WaitUntilSatisfied());
 }
 
+IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest,
+                       NewWindow_AttachAfterOpenerDestroyed) {
+  TestHelper("testNewWindowAttachAfterOpenerDestroyed",
+             "web_view/newwindow",
+             NEEDS_TEST_SERVER);
+}
+
 IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest,
                        NewWindow_NewWindowNameTakesPrecedence) {
   TestHelper("testNewWindowNameTakesPrecedence",
@@ -741,13 +774,13 @@ IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest,
 
 IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest,
                        NewWindow_WebViewNameTakesPrecedence) {
-  TestHelper("testWebViewNameTakesPrecedence",
+  TestHelper("testNewWindowWebViewNameTakesPrecedence",
              "web_view/newwindow",
              NEEDS_TEST_SERVER);
 }
 
 IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, NewWindow_NoName) {
-  TestHelper("testNoName",
+  TestHelper("testNewWindowNoName",
              "web_view/newwindow",
              NEEDS_TEST_SERVER);
 }
@@ -783,6 +816,13 @@ IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest,
              NEEDS_TEST_SERVER);
 }
 
+IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest,
+                       NewWindow_DiscardAfterOpenerDestroyed) {
+  TestHelper("testNewWindowDiscardAfterOpenerDestroyed",
+             "web_view/newwindow",
+             NEEDS_TEST_SERVER);
+}
+
 IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, NewWindow_WebRequest) {
   TestHelper("testNewWindowWebRequest",
              "web_view/newwindow",
@@ -831,6 +871,19 @@ IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, NewWindow_OpenInNewTab) {
   ASSERT_TRUE(done_listener->WaitUntilSatisfied());
 }
 
+IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest,
+                       NewWindow_OpenerDestroyedWhileUnattached) {
+  TestHelper("testNewWindowOpenerDestroyedWhileUnattached",
+             "web_view/newwindow",
+             NEEDS_TEST_SERVER);
+  ASSERT_EQ(2u, GetGuestViewManager()->guest_add_count());
+
+  // We have two guests in this test, one is the intial one, the other
+  // is the newwindow one.
+  // Before the embedder goes away, both the guests should go away.
+  // This ensures that unattached guests are gone if opener is gone.
+  GetGuestViewManager()->WaitForGuestRemoved(2u);
+}
 
 IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, ExecuteCode) {
   ASSERT_TRUE(RunPlatformAppTestWithArg(

chrome/renderer/resources/extensions/web_view_events.js

@@ -456,17 +456,31 @@ WebViewEvents.prototype.handleNewWindowEvent = function(event, webViewEvent) {
         if (!attached) {
           window.console.error(ERROR_MSG_NEWWINDOW_UNABLE_TO_ATTACH);
         }
+
+        var guestInstanceId = getGuestInstanceId();
+        if (!guestInstanceId) {
+          // If the opener is already gone, then we won't have its
+          // guestInstanceId.
+          return;
+        }
+
         // If the object being passed into attach is not a valid <webview>
         // then we will fail and it will be treated as if the new window
         // was rejected. The permission API plumbing is used here to clean
         // up the state created for the new window if attaching fails.
         WebView.setPermission(
-            getGuestInstanceId(), requestId, attached ? 'allow' : 'deny');
+            guestInstanceId, requestId, attached ? 'allow' : 'deny');
       }, 0);
     },
     discard: function() {
       validateCall();
-      WebView.setPermission(getGuestInstanceId(), requestId, 'deny');
+      var guestInstanceId = getGuestInstanceId();
+      if (!guestInstanceId) {
+        // If the opener is already gone, then we won't have its
+        // guestInstanceId.
+        return;
+      }
+      WebView.setPermission(guestInstanceId, requestId, 'deny');
     }
   };
   webViewEvent.window = windowObj;
@@ -483,13 +497,21 @@ WebViewEvents.prototype.handleNewWindowEvent = function(event, webViewEvent) {
       if (actionTaken) {
         return;
       }
+
+      var guestInstanceId = getGuestInstanceId();
+      if (!guestInstanceId) {
+        // If the opener is already gone, then we won't have its
+        // guestInstanceId.
+        return;
+      }
+
       WebView.setPermission(
-          getGuestInstanceId(), requestId, 'default', '', function(allowed) {
-        if (allowed) {
-          return;
-        }
-        showWarningMessage();
-      });
+          guestInstanceId, requestId, 'default', '', function(allowed) {
+            if (allowed) {
+              return;
+            }
+            showWarningMessage();
+          });
     });
   } else {
     actionTaken = true;

chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js

@@ -200,21 +200,21 @@ function testNewWindowNameTakesPrecedence() {
                     webViewName, guestName, partitionName, expectedName);
 }
 
-function testWebViewNameTakesPrecedence() {
+function testNewWindowWebViewNameTakesPrecedence() {
   var webViewName = 'foo';
   var guestName = '';
   var partitionName = 'persist:foobar';
   var expectedName = webViewName;
-  testNewWindowName('testWebViewNameTakesPrecedence',
+  testNewWindowName('testNewWindowWebViewNameTakesPrecedence',
                     webViewName, guestName, partitionName, expectedName);
 }
 
-function testNoName() {
+function testNewWindowNoName() {
   var webViewName = '';
   var guestName = '';
   var partitionName = '';
   var expectedName = '';
-  testNewWindowName('testNoName',
+  testNewWindowName('testNewWindowNoName',
                     webViewName, guestName, partitionName, expectedName);
 }
 
@@ -235,6 +235,34 @@ function testNewWindowRedirect() {
                     webViewName, guestName, partitionName, expectedName);
 }
 
+// Tests that we fail gracefully if we try to attach() a <webview> on a
+// newwindow event after the opener has been destroyed.
+function testNewWindowAttachAfterOpenerDestroyed() {
+  var testName = 'testNewWindowAttachAfterOpenerDestroyed';
+  var webview = embedder.setUpGuest_('foobar');
+
+  var onNewWindow = function(e) {
+    embedder.assertCorrectEvent_(e, '');
+
+    // Remove the opener.
+    webview.parentNode.removeChild(webview);
+    // Pass in a timeout so we ensure the newwindow disposal codepath
+    // works properly.
+    window.setTimeout(function() {
+      // At this point the opener <webview> is gone.
+      // Trying to discard() will fail silently.
+      e.window.attach(document.createElement('webview'));
+      window.setTimeout(function() { embedder.test.succeed(); }, 0);
+    }, 0);
+
+    e.preventDefault();
+  };
+  webview.addEventListener('newwindow', onNewWindow);
+
+  // Load a new window with the given name.
+  embedder.setUpNewWindowRequest_(webview, 'guest.html', '', testName);
+}
+
 function testNewWindowClose() {
   var testName = 'testNewWindowClose';
   var webview = embedder.setUpGuest_('foobar');
@@ -291,7 +319,35 @@ function testNewWindowDeferredAttachment() {
 
   // Load a new window with the given name.
   embedder.setUpNewWindowRequest_(webview, 'guest.html', '', testName);
-};
+}
+
+// Tests that we fail gracefully if we try to discard() a <webview> on a
+// newwindow event after the opener has been destroyed.
+function testNewWindowDiscardAfterOpenerDestroyed() {
+  var testName = 'testNewWindowDiscardAfterOpenerDestroyed';
+  var webview = embedder.setUpGuest_('foobar');
+
+  var onNewWindow = function(e) {
+    embedder.assertCorrectEvent_(e, '');
+
+    // Remove the opener.
+    webview.parentNode.removeChild(webview);
+    // Pass in a timeout so we ensure the newwindow disposal codepath
+    // works properly.
+    window.setTimeout(function() {
+      // At this point the opener <webview> is gone.
+      // Trying to discard() will fail silently.
+      e.window.discard();
+      window.setTimeout(function() { embedder.test.succeed(); }, 0);
+    }, 0);
+
+    e.preventDefault();
+  };
+  webview.addEventListener('newwindow', onNewWindow);
+
+  // Load a new window with the given name.
+  embedder.setUpNewWindowRequest_(webview, 'guest.html', '', testName);
+}
 
 function testNewWindowExecuteScript() {
   var testName = 'testNewWindowExecuteScript';
@@ -333,6 +389,30 @@ function testNewWindowOpenInNewTab() {
   webview.src = embedder.guestWithLinkURL;
 }
 
+// Tests that if opener <webview> is gone with unattached guest, we
+// don't see any error.
+// This test also makes sure we destroy the unattached guests properly.
+function testNewWindowOpenerDestroyedWhileUnattached() {
+  var testName = 'testNewWindowOpenerDestroyedBeforeAttach';
+  var webview = embedder.setUpGuest_('foobar');
+
+  var onNewWindow = function(e) {
+    embedder.assertCorrectEvent_(e, '');
+
+    // Remove the opener.
+    webview.parentNode.removeChild(webview);
+    // Pass in a timeout so we ensure the newwindow disposal codepath
+    // works properly.
+    window.setTimeout(function() { embedder.test.succeed(); }, 0);
+
+    e.preventDefault();
+  };
+  webview.addEventListener('newwindow', onNewWindow);
+
+  // Load a new window with the given name.
+  embedder.setUpNewWindowRequest_(webview, 'guest.html', '', testName);
+}
+
 function testNewWindowWebRequest() {
   var testName = 'testNewWindowWebRequest';
   var webview = embedder.setUpGuest_('foobar');
@@ -491,18 +571,25 @@ function testNewWindowWebRequestRemoveElement() {
 }
 
 embedder.test.testList = {
-  'testNewWindowNameTakesPrecedence': testNewWindowNameTakesPrecedence,
-  'testWebViewNameTakesPrecedence': testWebViewNameTakesPrecedence,
-  'testNoName': testNoName,
-  'testNewWindowRedirect':  testNewWindowRedirect,
+  'testNewWindowAttachAfterOpenerDestroyed':
+      testNewWindowAttachAfterOpenerDestroyed,
   'testNewWindowClose': testNewWindowClose,
+  'testNewWindowDeclarativeWebRequest': testNewWindowDeclarativeWebRequest,
   'testNewWindowDeferredAttachment': testNewWindowDeferredAttachment,
+  'testNewWindowDiscardAfterOpenerDestroyed':
+      testNewWindowDiscardAfterOpenerDestroyed,
   'testNewWindowExecuteScript': testNewWindowExecuteScript,
+  'testNewWindowNameTakesPrecedence': testNewWindowNameTakesPrecedence,
+  'testNewWindowNoName': testNewWindowNoName,
   'testNewWindowOpenInNewTab': testNewWindowOpenInNewTab,
-  'testNewWindowDeclarativeWebRequest': testNewWindowDeclarativeWebRequest,
+  'testNewWindowOpenerDestroyedWhileUnattached':
+      testNewWindowOpenerDestroyedWhileUnattached,
+  'testNewWindowRedirect':  testNewWindowRedirect,
   'testNewWindowWebRequest': testNewWindowWebRequest,
   'testNewWindowWebRequestCloseWindow': testNewWindowWebRequestCloseWindow,
-  'testNewWindowWebRequestRemoveElement': testNewWindowWebRequestRemoveElement
+  'testNewWindowWebRequestRemoveElement': testNewWindowWebRequestRemoveElement,
+  'testNewWindowWebViewNameTakesPrecedence':
+      testNewWindowWebViewNameTakesPrecedence
 };
 
 onload = function() {