Remove WebContentsImpl::OnDidRedirectProvisionalLoad.

content/browser/web_contents/navigation_controller_impl_unittest.cc

 // Copyright (c) 2012 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/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
@@ skipping 500 matching lines @@
       kNewURL, content::Referrer(), content::PAGE_TRANSITION_TYPED,
       std::string());
   EXPECT_EQ(0U, notifications.size());
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_TRUE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
   EXPECT_EQ(1, delegate->navigation_state_change_count());
 
   // Now the navigation redirects.
   const GURL kRedirectURL("http://bee");
-  test_rvh()->OnMessageReceived(
-      ViewHostMsg_DidRedirectProvisionalLoad(0,  // routing_id
-                                             -1,  // pending page_id
-                                             GURL(),  // opener
-                                             kNewURL,  // old url
-                                             kRedirectURL));  // new url

[Charlie Reis, 2012/05/30 22:43:31]

I don't understand.  Doesn't this just remove the redirect (which is the
relevant part of the test)?

More specifically, does this modified version of the test fail if you undo the
security fix from the patch below?
http://codereview.chromium.org/6993037/diff/8001/content/browser/tab_contents...

[mmenke, 2012/05/30 22:50:43]

After this CL, the NavigationController will no longer receive any notification
on redirects, it looks to me, and thus can't update its title in response to the
redirect.

[Charlie Reis, 2012/05/31 16:56:27]

Ok, I experimentally verified that this updated test still catches regressions
for 83031.  That is, it fails if I re-introduce the bug into
WebContentsImpl::OnDidFailProvisionalLoadWithError.

Please update the comment on line 519 to also say that the RVH gets no
notification of the redirect, since the code is confusing as it stands.

 
   // We don't want to change the NavigationEntry's url, in case it cancels.
   // Prevents regression of http://crbug.com/77786.
-  EXPECT_EQ(kNewURL, controller.GetPendingEntry()->GetURL());
+  //EXPECT_EQ(kNewURL, controller.GetPendingEntry()->GetURL());

[Charlie Reis, 2012/05/30 22:43:31]

Shouldn't have commented out code.  But again, this is a security bug regression
test you're removing.  Maybe I don't understand what you're trying to achieve. 
Is there a reason it's safe not to have these checks?

[Charlie Reis, 2012/05/31 16:56:27]

This is unfortunate.  I'd like to keep this line in case the bug comes back
elsewhere, but it's kind of meaningless without actually simulating a redirect. 
The only way I know to do that is with
ResourceDispatcherHost::NotifyReceivedRedirect, which isn't available in this
unit test.

I suppose it makes the most sense to remove this line.  You've removed the whole
method that contained the bug, so it's unlikely to regress.

[Deprecated (see juliatuttle), 2012/05/31 18:16:24]

Done.

 
   // It may abort before committing, if it's a download or due to a stop or
   // a new navigation from the user.
   ViewHostMsg_DidFailProvisionalLoadWithError_Params params;
   params.frame_id = 1;
   params.is_main_frame = true;
   params.error_code = net::ERR_ABORTED;
   params.error_description = string16();
   params.url = kRedirectURL;
   params.showing_repost_interstitial = false;
@@ skipping 2078 matching lines @@
   TabNavigation nav(0, url0, GURL(), string16(),
                     webkit_glue::CreateHistoryStateForURL(url0),
                     content::PAGE_TRANSITION_LINK);
   session_helper_.AssertNavigationEquals(nav,
                                          windows_[0]->tabs[0]->navigations[0]);
   nav.set_url(url2);
   session_helper_.AssertNavigationEquals(nav,
                                          windows_[0]->tabs[0]->navigations[1]);
 }
 */