Remove WebContentsImpl::OnDidRedirectProvisionalLoad.

Remove WebContentsImpl::OnDidRedirectProvisionalLoad.

Also remove the associated ViewHostMsg, and remove
RenderViewImpl::didReceiveServerRedirectForProvisonalLoad, since it only
served to send that message.

BUG=78512
TEST=builds; cbentzel will try


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141066

[cbentzel, 2012-05-03 19:12:27]

You have to do "Publish+Mail Comments" to add reviewers. Otherwise it won't show
up in the reviewer's queue.

[Deprecated (see juliatuttle), 2012-05-03 19:13:57]

Done.

On Thu, May 3, 2012 at 3:12 PM,  <cbentzel@chromium.org> wrote:
> You have to do "Publish+Mail Comments" to add reviewers. Otherwise it won't
> show
> up in the reviewer's queue.
>
> http://codereview.chromium.org/10316020/

[cbentzel, 2012-05-03 19:56:45]

Initial thought - it looks like you did remove the OnDidRedirectProvisionalLoad
hooks correctly functionality, but didn't change the prerender code to listen to
the RESOURCE_RECEIVED_REDIRECT notification. This likely means that prerenders
won't work correctly. I'll run a try job and this should show up. Alternately
you could build the browser_tests target and run it with
--gtest_filter=*PrerenderBrowserTest

[cbentzel, 2012-05-03 19:57:14]

One other nit - you can just do BUG=78512

[cbentzel, 2012-05-05 00:18:35]

On 2012/05/03 19:57:14, cbentzel wrote:
> One other nit - you can just do BUG=78512

Quick suggestion: add a reply to the code review when you update it with fixes,
otherwise I may forget it.

Overall this looks fine. I'll re-try it before asking you to get OWNERS
approval.

[cbentzel, 2012-05-08 04:29:09]

http://codereview.chromium.org/10316020/diff/6001/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

http://codereview.chromium.org/10316020/diff/6001/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1556:
ProvisionalChangeToMainFrameUrl(validated_target_url,
You should remove ProvisionalChangeToMainFrameUrl in this CL.

[cbentzel, 2012-05-08 04:38:40]

And sorry for my super-slow turnaround time on this CL. You may want to ask
mmenke or gavinp to take over as I don't expect that I'll have much time to look
at it this week.

[Deprecated (see juliatuttle), 2012-05-14 22:24:52]

Hey, I removed ProvisionalChangeToMainFrameUrl.  Please take a look now.

[cbentzel, 2012-05-15 17:33:06]

THe method should also be removed from WebContentsObserver

[Deprecated (see juliatuttle), 2012-05-15 18:16:25]

Argh.  I could've *sworn* I removed that.  I remember removing it.  Oh well. 
Done.

[cbentzel, 2012-05-15 20:26:58]

This comment is for myself. I am not sure if you can get away with removing the
functionality in PrerenderTabHelper. If you can't, you can refactor that class
to listen to RESOURCE_RECEIVED_REDIRECT notifications (essentially, a pub-sub
system). See http://codereview.chromium.org/6824054 for a CL which did that.

However, if we can get rid of some of this functionality, let's do it.

http://codereview.chromium.org/10316020/diff/18001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tab_helper.cc (left):

http://codereview.chromium.org/10316020/diff/18001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.cc:48:
RecordPageviewEvent(PAGEVIEW_EVENT_NEW_URL);
Note to self: I'm not sure we can get rid of this functionality, although it is
not being tested right now. See if this needs to use RESOURCE_RECEIVED_REDIRECT
notification instead.

[cbentzel, 2012-05-16 00:53:01]

OK, as I discussed with you in  person, I think the fastest way to get this in
is to make PrerenderTabHelper work the same as before, but just use the
RESOURCE_RECEIVED_REDIRECT notification. I can do a separate pass to remove some
of the existing functionality.

On 2012/05/15 20:26:58, cbentzel wrote:
> This comment is for myself. I am not sure if you can get away with removing
the
> functionality in PrerenderTabHelper. If you can't, you can refactor that class
> to listen to RESOURCE_RECEIVED_REDIRECT notifications (essentially, a pub-sub
> system). See http://codereview.chromium.org/6824054 for a CL which did that.
> 
> However, if we can get rid of some of this functionality, let's do it.
> 
>
http://codereview.chromium.org/10316020/diff/18001/chrome/browser/prerender/p...
> File chrome/browser/prerender/prerender_tab_helper.cc (left):
> 
>
http://codereview.chromium.org/10316020/diff/18001/chrome/browser/prerender/p...
> chrome/browser/prerender/prerender_tab_helper.cc:48:
> RecordPageviewEvent(PAGEVIEW_EVENT_NEW_URL);
> Note to self: I'm not sure we can get rid of this functionality, although it
is
> not being tested right now. See if this needs to use
RESOURCE_RECEIVED_REDIRECT
> notification instead.

[Deprecated (see juliatuttle), 2012-05-17 17:24:35]

Now listens for RESOURCE_RECEIVED_REDIRET in PrerenderTabHelper.  Not tested,
because there are no tests for it, and I can't think of an easy way to test it
manually.  Suggestions welcome :)

[cbentzel, 2012-05-17 17:42:39]

Thanks. LGTM. You'll also need OWNERS approval from other folks, and I will also
run try jobs.

http://codereview.chromium.org/10316020/diff/13003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

http://codereview.chromium.org/10316020/diff/13003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.cc:281: url_ =
resource_redirect_details->new_url;
Nit: I'd move everything below this into a helper function.

http://codereview.chromium.org/10316020/diff/13003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tab_helper.h (right):

http://codereview.chromium.org/10316020/diff/13003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.h:69: // Adds an alias URL, for
one of the many redirections. If the URL can not
This comment looks like a bad copy-and-paste.

[cbentzel, 2012-05-17 17:44:27]

creis+jam: I already looked through this and it looks good. Will need OWNERS
approval for all the directory changes.

[Deprecated (see juliatuttle), 2012-05-17 18:42:20]

PTAL.

http://codereview.chromium.org/10316020/diff/13003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

http://codereview.chromium.org/10316020/diff/13003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.cc:281: url_ =
resource_redirect_details->new_url;
On 2012/05/17 17:42:39, cbentzel wrote:
> Nit: I'd move everything below this into a helper function.

Done.

http://codereview.chromium.org/10316020/diff/13003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tab_helper.h (right):

http://codereview.chromium.org/10316020/diff/13003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.h:69: // Adds an alias URL, for
one of the many redirections. If the URL can not
On 2012/05/17 17:42:39, cbentzel wrote:
> This comment looks like a bad copy-and-paste.

Done.

[cbentzel, 2012-05-17 18:52:05]

http://codereview.chromium.org/10316020/diff/18002/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

http://codereview.chromium.org/10316020/diff/18002/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.cc:270: void
PrerenderTabHelper::HandleResourceReceivedRedirect(const GURL& new_url) {
Nit: the definitions should be ordered in same way as declarations in the
header. This helper may be in the right place, but Observe no longer is. Of
course, I think it makes sense if Observe is moved to private section and then
you will be OK.

http://codereview.chromium.org/10316020/diff/18002/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.cc:281: }
Nit: this needs to be outdented by 1

http://codereview.chromium.org/10316020/diff/18002/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tab_helper.h (right):

http://codereview.chromium.org/10316020/diff/18002/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.h:46: //
content::NotificationObserver
Why did this move to the public interface in this patchset? It  could still be
private.

[jam, 2012-05-17 23:13:17]

lgtm

[Charlie Reis, 2012-05-17 23:42:55]

Thanks for taking care of this.  Can we keep a possibly updated version of the
test?

http://codereview.chromium.org/10316020/diff/18003/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl_unittest.cc (left):

http://codereview.chromium.org/10316020/diff/18003/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl_unittest.cc:494: //
http://crbug.com/83031.
This is a regression test for a security bug.  It's not ok to delete unless we
have other test coverage to prevent regression.

[cbentzel, 2012-05-18 00:23:57]

http://codereview.chromium.org/10316020/diff/18003/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl_unittest.cc (left):

http://codereview.chromium.org/10316020/diff/18003/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl_unittest.cc:494: //
http://crbug.com/83031.
On 2012/05/17 23:42:55, creis wrote:
> This is a regression test for a security bug.  It's not ok to delete unless we
> have other test coverage to prevent regression.

Sorry about LGTM'ing with this change.

[Deprecated (see juliatuttle), 2012-05-18 19:59:22]

On 2012/05/18 00:23:57, cbentzel wrote:
>
http://codereview.chromium.org/10316020/diff/18003/content/browser/web_conten...
> File content/browser/web_contents/navigation_controller_impl_unittest.cc
(left):
> 
>
http://codereview.chromium.org/10316020/diff/18003/content/browser/web_conten...
> content/browser/web_contents/navigation_controller_impl_unittest.cc:494: //
> http://crbug.com/83031.
> On 2012/05/17 23:42:55, creis wrote:
> > This is a regression test for a security bug.  It's not ok to delete unless
we
> > have other test coverage to prevent regression.
> 
> Sorry about LGTM'ing with this change.

Alright, I'm looking at this to see how I should be signalling the redirect
instead.

[Deprecated (see juliatuttle), 2012-05-30 22:22:47]

PTAL.

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

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
File content/browser/web_contents/navigation_controller_impl_unittest.cc (left):

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
content/browser/web_contents/navigation_controller_impl_unittest.cc:526:
kRedirectURL));  // new url
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...

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
content/browser/web_contents/navigation_controller_impl_unittest.cc:524:
//EXPECT_EQ(kNewURL, controller.GetPendingEntry()->GetURL());
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?

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

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
File content/browser/web_contents/navigation_controller_impl_unittest.cc (left):

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
content/browser/web_contents/navigation_controller_impl_unittest.cc:526:
kRedirectURL));  // new url
On 2012/05/30 22:43:31, creis wrote:
> 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...

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:11:10]

On 2012/05/30 22:50:43, Matt Menke wrote:
> 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.

That doesn't explain whether we will catch regressions of the two vulnerability
fixes.  I'll have to patch this in to a local build to understand how it
interacts with those.  I'll follow up in a bit.

[Charlie Reis, 2012-05-31 16:56:26]

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
File content/browser/web_contents/navigation_controller_impl_unittest.cc (left):

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
content/browser/web_contents/navigation_controller_impl_unittest.cc:526:
kRedirectURL));  // new url
On 2012/05/30 22:50:43, Matt Menke wrote:
> On 2012/05/30 22:43:31, creis wrote:
> > 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...
> 
> 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.

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.

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
content/browser/web_contents/navigation_controller_impl_unittest.cc:524:
//EXPECT_EQ(kNewURL, controller.GetPendingEntry()->GetURL());
On 2012/05/30 22:43:31, creis wrote:
> 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?

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.

[mmenke, 2012-05-31 17:19:38]

One option would be to switch to a BrowserTest, and go through the whole
redirect process, though since the main concern is with the cross-domain case,
that makes the test a little tricky.

On 2012/05/31 16:56:26, creis wrote:
>
https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
> File content/browser/web_contents/navigation_controller_impl_unittest.cc
(left):
> 
>
https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
> content/browser/web_contents/navigation_controller_impl_unittest.cc:526:
> kRedirectURL));  // new url
> On 2012/05/30 22:50:43, Matt Menke wrote:
> > On 2012/05/30 22:43:31, creis wrote:
> > > 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...
> > 
> > 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.
> 
> 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.
> 
>
https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
> File content/browser/web_contents/navigation_controller_impl_unittest.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
> content/browser/web_contents/navigation_controller_impl_unittest.cc:524:
> //EXPECT_EQ(kNewURL, controller.GetPendingEntry()->GetURL());
> On 2012/05/30 22:43:31, creis wrote:
> > 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?
> 
> 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]

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10316020/diff/26001/content/browser/we...
content/browser/web_contents/navigation_controller_impl_unittest.cc:524:
//EXPECT_EQ(kNewURL, controller.GetPendingEntry()->GetURL());
On 2012/05/31 16:56:27, creis wrote:
> On 2012/05/30 22:43:31, creis wrote:
> > 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?
> 
> 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.

Done.

[Charlie Reis, 2012-05-31 18:21:46]

Thanks, LGTM.

[cbentzel, 2012-05-31 18:26:02]

Thanks for confirming this, Charlie.

I'll run a try job first. After that, Thomas, I believe you can just use
the commit queue.

On Thu, May 31, 2012 at 2:21 PM, <creis@chromium.org> wrote:

> Thanks, LGTM.
>
>
https://chromiumcodereview.**appspot.com/10316020/<https://chromiumcodereview...
>

[cbentzel, 2012-06-01 13:50:16]

Thomas - you'll need to merge/rebase relative to trunk.

[Deprecated (see juliatuttle), 2012-06-05 20:24:32]

PTAL.

[cbentzel, 2012-06-06 14:54:42]

LGTM

http://codereview.chromium.org/10316020/diff/41001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

http://codereview.chromium.org/10316020/diff/41001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_tab_helper.cc:259: int type,
Nit: these arguments should only be indented by 4 spaces.

[Deprecated (see juliatuttle), 2012-06-07 02:02:32]

Fixed indentation; am I good to commit this?

[cbentzel, 2012-06-07 02:12:53]

On 2012/06/07 02:02:32, ttuttle wrote:
> Fixed indentation; am I good to commit this?

Yes.

When a LGTM is given, along with some nits, it generally means "You don't need
to ask me for approval again, assuming you address these issues below".

[commit-bot: I haz the power, 2012-06-07 17:14:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/10316020/41004

[commit-bot: I haz the power, 2012-06-07 17:30:52]

Try job failure for 10316020-41004 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-07 18:01:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/10316020/45004

[commit-bot: I haz the power, 2012-06-07 19:49:21]

Change committed as 141066