[ios] Do not commit invalid URLs during web load.

[ios] Do not commit invalid URLs during web load.

BUG=622183
Committed: https://crrev.com/c2d2b0f2f74dad0bdef196cf1657f0d584cbe3a7
Cr-Commit-Position: refs/heads/master@{#401761}

[Eugene But (OOO till 7-30), 2016-06-22 20:01:24]

eugenebut@chromium.org changed reviewers:
+ kkhorimoto@chromium.org

[Eugene But (OOO till 7-30), 2016-06-22 20:01:24]

Please see the bug for details about this change.

[kkhorimoto, 2016-06-22 22:20:54]

https://codereview.chromium.org/2086333003/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2086333003/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:5106: // will be "about:blank".
Should we add a DCHECK to ensure that it's about:blank?  Would it be possible
for the committed document URL to be something valid (i.e. gmail.com), then some
kind of redirect occurring between this and |-webView:didFinishNavigation:|
where the URL is reset to about:blank?  That would bypass the DCHECK above, but
would have a valid spoofed URL for the committed item.  I'm not sure that could
happen in a way that would bypass other calls to |-updatePendingEntry:| of the
top of my head though.

[Eugene But (OOO till 7-30), 2016-06-22 22:48:25]

https://codereview.chromium.org/2086333003/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2086333003/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:5106: // will be "about:blank".
On 2016/06/22 22:20:54, kkhorimoto_ wrote:
> Should we add a DCHECK to ensure that it's about:blank?  Would it be possible
> for the committed document URL to be something valid (i.e. http://gmail.com),
then some
> kind of redirect occurring between this and |-webView:didFinishNavigation:|
> where the URL is reset to about:blank?  That would bypass the DCHECK above,
but
> would have a valid spoofed URL for the committed item.  I'm not sure that
could
> happen in a way that would bypass other calls to |-updatePendingEntry:| of the
> top of my head though.
1.) DCHECK below covers that case.
2) I don't think that server side redirect can happen after didCommitNavigation:
(which means that server started responding with data). Client side-redirect
should cause a new navigation. Do you think I'm missing something?

[kkhorimoto, 2016-06-23 23:06:22]

https://codereview.chromium.org/2086333003/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2086333003/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:5106: // will be "about:blank".
On 2016/06/22 22:48:25, Eugene But wrote:
> On 2016/06/22 22:20:54, kkhorimoto_ wrote:
> > Should we add a DCHECK to ensure that it's about:blank?  Would it be
possible
> > for the committed document URL to be something valid (i.e.
http://gmail.com),
> then some
> > kind of redirect occurring between this and |-webView:didFinishNavigation:|
> > where the URL is reset to about:blank?  That would bypass the DCHECK above,
> but
> > would have a valid spoofed URL for the committed item.  I'm not sure that
> could
> > happen in a way that would bypass other calls to |-updatePendingEntry:| of
the
> > top of my head though.
> 1.) DCHECK below covers that case.
> 2) I don't think that server side redirect can happen after
didCommitNavigation:
> (which means that server started responding with data). Client side-redirect
> should cause a new navigation. Do you think I'm missing something? 

Yeah, I checked it out and it looks like all client redirects have an associated
registerLoadRequest, so this will probably be fine.

[kkhorimoto, 2016-06-23 23:24:53]

lgtm

[Eugene But (OOO till 7-30), 2016-06-23 23:25:07]

Thanks!

[Eugene But (OOO till 7-30), 2016-06-23 23:25:11]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2016-06-23 23:25:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2086333003/20001

[commit-bot: I haz the power, 2016-06-24 00:49:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-24 00:52:31]

Description was changed from

==========
[ios] Do not commit invalid URLs during web load.

BUG=622183
==========

to

==========
[ios] Do not commit invalid URLs during web load.

BUG=622183

Committed: https://crrev.com/c2d2b0f2f74dad0bdef196cf1657f0d584cbe3a7
Cr-Commit-Position: refs/heads/master@{#401761}
==========

[commit-bot: I haz the power, 2016-06-24 00:52:32]

Patchset 2 (id:??) landed as
https://crrev.com/c2d2b0f2f74dad0bdef196cf1657f0d584cbe3a7
Cr-Commit-Position: refs/heads/master@{#401761}