When a pending contents is created but deleted before it is shown, remove it from the list of pending contents

This happens during running layout tests when all newly opened windows are
closed, however, an IPC message to show one of the new windows is already in
flight

The other direction, when the opener is deleted, is already covered by existing
code.

BUG=180969
R=creis@chromium.org
TEST=content_unittests:WebContentsImplTest.PendingContents

Review URL: https://chromiumcodereview.appspot.com/12602003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@186893 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 40 insertions, 0 deletions

diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 37e11c7..28d9ef2 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1225,7 +1225,20 @@ void WebContentsImpl::OnWebContentsDestroyed(WebContents* web_contents) {
     registrar_.Remove(this, NOTIFICATION_WEB_CONTENTS_DESTROYED,
                       Source<WebContents>(opener_));
     opener_ = NULL;
+    return;
   }
+  // Clear a pending contents that has been closed before being shown.
+  for (PendingContents::iterator iter = pending_contents_.begin();
+       iter != pending_contents_.end();
+       ++iter) {
+    if (iter->second != web_contents)
+      continue;
+    pending_contents_.erase(iter);
+    registrar_.Remove(this, NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                      Source<WebContents>(web_contents));
+    return;
+  }
+  NOTREACHED();
 }
 
 void WebContentsImpl::AddObserver(WebContentsObserver* observer) {
@@ -1407,6 +1420,8 @@ void WebContentsImpl::CreateNewWindow(
     // later.
     DCHECK_NE(MSG_ROUTING_NONE, route_id);
     pending_contents_[route_id] = new_contents;
+    registrar_.Add(this, NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                   Source<WebContents>(new_contents));
   }
 
   if (delegate_) {
@@ -1540,6 +1555,8 @@ WebContentsImpl* WebContentsImpl::GetCreatedWindow(int route_id) {
 
   WebContentsImpl* new_contents = iter->second;
   pending_contents_.erase(route_id);
+  registrar_.Remove(this, NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                    Source<WebContents>(new_contents));
 
   if (!new_contents->GetRenderProcessHost()->HasConnection() ||
       !new_contents->GetRenderViewHost()->GetView())
@@ -2982,6 +2999,8 @@ void WebContentsImpl::DidChangeLoadProgress(double progress) {
 void WebContentsImpl::DidDisownOpener(RenderViewHost* rvh) {
   // Clear our opener so that future cross-process navigations don't have an
   // opener assigned.
+  registrar_.Remove(this, NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                    Source<WebContents>(opener_));
   opener_ = NULL;
 
   // Notify all swapped out RenderViewHosts for this tab.  This is important
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index e15395e..4f535e2 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -478,6 +478,7 @@ class CONTENT_EXPORT WebContentsImpl
   FRIEND_TEST_ALL_PREFIXES(WebContentsImplTest, FindOpenerRVHWhenPending);
   FRIEND_TEST_ALL_PREFIXES(WebContentsImplTest,
                            CrossSiteCantPreemptAfterUnload);
+  FRIEND_TEST_ALL_PREFIXES(WebContentsImplTest, PendingContents);
   FRIEND_TEST_ALL_PREFIXES(FormStructureBrowserTest, HTMLFiles);
   FRIEND_TEST_ALL_PREFIXES(NavigationControllerTest, HistoryNavigate);
   FRIEND_TEST_ALL_PREFIXES(RenderViewHostManagerTest, PageDoesBackAndReload);
diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc
index 6ec8320..80b8713 100644
--- a/content/browser/web_contents/web_contents_impl_unittest.cc
+++ b/content/browser/web_contents/web_contents_impl_unittest.cc
@@ -2037,5 +2037,15 @@ TEST_F(WebContentsImplTest, FilterURLs) {
   EXPECT_EQ(url_normalized, other_observer.last_url());
 }
 
+// Test that if a pending contents is deleted before it is shown, we don't
+// crash.
+TEST_F(WebContentsImplTest, PendingContents) {
+  scoped_ptr<TestWebContents> other_contents(
+      static_cast<TestWebContents*>(CreateTestWebContents()));
+  contents()->AddPendingContents(other_contents.get());
+  int route_id = other_contents->GetRenderViewHost()->GetRoutingID();
+  other_contents.reset();
+  EXPECT_EQ(NULL, contents()->GetCreatedWindow(route_id));
+}
 
 }  // namespace content
diff --git a/content/test/test_web_contents.cc b/content/test/test_web_contents.cc
index 3a79e9e..678aa1f 100644
--- a/content/test/test_web_contents.cc
+++ b/content/test/test_web_contents.cc
@@ -174,6 +174,13 @@ void TestWebContents::SetOpener(TestWebContents* opener) {
                  Source<WebContents>(opener_));
 }
 
+void TestWebContents::AddPendingContents(TestWebContents* contents) {
+  // This is normally only done in WebContentsImpl::CreateNewWindow.
+  pending_contents_[contents->GetRenderViewHost()->GetRoutingID()] = contents;
+  registrar_.Add(this, NOTIFICATION_WEB_CONTENTS_DESTROYED,
+                 Source<WebContents>(contents));
+}
+
 void TestWebContents::ExpectSetHistoryLengthAndPrune(
     const SiteInstance* site_instance,
     int history_length,
diff --git a/content/test/test_web_contents.h b/content/test/test_web_contents.h
index 4ae4434..644c45d 100644
--- a/content/test/test_web_contents.h
+++ b/content/test/test_web_contents.h
@@ -76,6 +76,9 @@ class TestWebContents : public WebContentsImpl, public WebContentsTester {
   // Allows us to simulate this tab having an opener.
   void SetOpener(TestWebContents* opener);
 
+  // Allows us to simulate that a contents was created via CreateNewWindow.
+  void AddPendingContents(TestWebContents* contents);
+
   // Establish expected arguments for |SetHistoryLengthAndPrune()|. When
   // |SetHistoryLengthAndPrune()| is called, the arguments are compared
   // with the expected arguments specified here.