Use the last committed entry in DidStopLoading, not the pending entry.

BUG=233053
TEST=WebContentsImplBrowserTest.DidStopLoadingDetailsWithPending

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

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

content/browser/web_contents/web_contents_impl.cc

@@ -2853,7 +2853,10 @@ void WebContentsImpl::DidStartLoading(RenderViewHost* render_view_host) {
 void WebContentsImpl::DidStopLoading(RenderViewHost* render_view_host) {
   scoped_ptr<LoadNotificationDetails> details;
 
-  NavigationEntry* entry = controller_.GetActiveEntry();
+  // Use the last committed entry rather than the active one, in case a
+  // pending entry has been created.
+  NavigationEntry* entry = controller_.GetLastCommittedEntry();
+
   // An entry may not exist for a stop when loading an initial blank page or
   // if an iframe injected by script into a blank page finishes loading.
   if (entry) {

content/browser/web_contents/web_contents_impl_browsertest.cc

+// Copyright 2013 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.
+
+#include "base/values.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/load_notification_details.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+#include "content/shell/shell.h"
+#include "content/test/content_browser_test.h"
+#include "content/test/content_browser_test_utils.h"
+#include "net/test/test_server.h"
+
+namespace content {
+
+class WebContentsImplBrowserTest : public ContentBrowserTest {
+ public:
+  WebContentsImplBrowserTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WebContentsImplBrowserTest);
+};
+
+// Keeps track of data from LoadNotificationDetails so we can later verify that
+// they are correct, after the LoadNotificationDetails object is deleted.
+class LoadStopNotificationObserver : public WindowedNotificationObserver {
+ public:
+  LoadStopNotificationObserver(NavigationController* controller)
+      : WindowedNotificationObserver(NOTIFICATION_LOAD_STOP,
+                                     Source<NavigationController>(controller)),
+        session_index_(-1),
+        controller_(NULL) {
+  }
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) OVERRIDE {
+    if (type == NOTIFICATION_LOAD_STOP) {
+      const Details<LoadNotificationDetails> load_details(details);
+      url_ = load_details->url;
+      session_index_ = load_details->session_index;
+      controller_ = load_details->controller;
+    }
+    WindowedNotificationObserver::Observe(type, source, details);
+  }
+
+  GURL url_;
+  int session_index_;
+  NavigationController* controller_;
+};
+
+// Starts a new navigation as soon as the current one commits, but does not
+// wait for it to complete.  This allows us to observe DidStopLoading while
+// a pending entry is present.
+class NavigateOnCommitObserver : public WindowedNotificationObserver {
+ public:
+  NavigateOnCommitObserver(Shell* shell, GURL url)
+      : WindowedNotificationObserver(
+            NOTIFICATION_NAV_ENTRY_COMMITTED,
+            Source<NavigationController>(
+                &shell->web_contents()->GetController())),
+        shell_(shell),
+        url_(url),
+        done_(false) {
+  }
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) OVERRIDE {
+    if (type == NOTIFICATION_NAV_ENTRY_COMMITTED && !done_) {
+      done_ = true;
+      shell_->LoadURL(url_);
+    }
+    WindowedNotificationObserver::Observe(type, source, details);
+  }
+
+  Shell* shell_;
+  GURL url_;
+  bool done_;
+};
+
+// Test that DidStopLoading includes the correct URL in the details.
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, DidStopLoadingDetails) {
+  ASSERT_TRUE(test_server()->Start());
+
+  LoadStopNotificationObserver load_observer(
+      &shell()->web_contents()->GetController());
+  NavigateToURL(shell(), test_server()->GetURL("files/title1.html"));
+  load_observer.Wait();
+
+  EXPECT_EQ("/files/title1.html", load_observer.url_.path());
+  EXPECT_EQ(0, load_observer.session_index_);
+  EXPECT_EQ(&shell()->web_contents()->GetController(),
+            load_observer.controller_);
+}
+
+// Test that DidStopLoading includes the correct URL in the details when a
+// pending entry is present.
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       DidStopLoadingDetailsWithPending) {
+  ASSERT_TRUE(test_server()->Start());
+
+  // Listen for the first load to stop.
+  LoadStopNotificationObserver load_observer(
+      &shell()->web_contents()->GetController());
+  // Start a new pending navigation as soon as the first load commits.
+  // We will hear a DidStopLoading from the first load as the new load
+  // is started.
+  NavigateOnCommitObserver commit_observer(
+      shell(), test_server()->GetURL("files/title2.html"));
+  NavigateToURL(shell(), test_server()->GetURL("files/title1.html"));
+  commit_observer.Wait();
+  load_observer.Wait();
+
+  EXPECT_EQ("/files/title1.html", load_observer.url_.path());
+  EXPECT_EQ(0, load_observer.session_index_);
+  EXPECT_EQ(&shell()->web_contents()->GetController(),
+            load_observer.controller_);
+}
+
+}  // namespace content

content/content_tests.gypi

@@ -767,6 +767,7 @@
             'browser/session_history_browsertest.cc',
             'browser/site_per_process_browsertest.cc',
             'browser/speech/speech_recognition_browsertest.cc',
+            'browser/web_contents/web_contents_impl_browsertest.cc',
             'browser/web_contents/web_contents_view_aura_browsertest.cc',
             'browser/webkit_browsertest.cc',
             'browser/worker_host/test/worker_browsertest.cc',