Add an ability to call WebKit's WebFrame::loadData via NavigationController.

Add an ability to call WebKit's WebFrame::loadData via NavigationController.

WebFrame::loadData accepts HTML that is wrapped into a 'data:' scheme URL
and two URLs: the base URL is used for resolving relative URLs in the HTML,
the history URL is put into the navigation history.

WebKit has all the required support for this method. What is required is
to pass base and history URLs from RVH to WebKit's WebFrame. Also, as
Chromium contains additional security checks when loading page resources,
we need to add the base URL into the list of request URLs for the security
policy.

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148090

[mnaganov (inactive), 2012-06-15 14:08:20]

Hi Charlie and John,

This CL should be considered together with Dan's
https://chromiumcodereview.appspot.com/10450002. We need this for WebView, as it
supports loading data: URLs with a specified base URL for resolving relative
URLs. WebKit has all the support for this, we just need to pass URLs through the
content layer.

[jam, 2012-06-15 16:17:05]

Just so I understand, does the shape of how 10450002 evolves affect this change?
or will this be the same regardless?

my one nit is that in two places you comment that this base url is only used in
android. but given that it's not behind an ifdef, any platform can call it now.
so i would just leave out the android comments.

[mnaganov (inactive), 2012-06-15 16:24:33]

This is not related to 10450002. But we also need to extend
NavigationController in it. Dan's concern is that the
NavigationController's interface is going to get complicated, and he
has proposed to consider these two changes together.

On Fri, Jun 15, 2012 at 5:17 PM,  <jam@chromium.org> wrote:
> Just so I understand, does the shape of how 10450002 evolves affect this
> change?
> or will this be the same regardless?
>
> my one nit is that in two places you comment that this base url is only used
> in
> android. but given that it's not behind an ifdef, any platform can call it
> now.
> so i would just leave out the android comments.
>
> http://codereview.chromium.org/10544175/

[gone, 2012-06-15 17:09:10]

Yeah, it seems like both of our CLs are adding functions to the
NavigationController to make changes to the NavigationEntry before it's
sent off to be loaded.  TransferURL (which is already in there) does the
same thing: it seems to exist just to set the global request ID.  Do you
think there's a cleaner way to do what we need to do?

On Fri, Jun 15, 2012 at 9:24 AM, Mikhail Naganov <mnaganov@chromium.org>wrote:

> This is not related to 10450002. But we also need to extend
> NavigationController in it. Dan's concern is that the
> NavigationController's interface is going to get complicated, and he
> has proposed to consider these two changes together.
>
> On Fri, Jun 15, 2012 at 5:17 PM,  <jam@chromium.org> wrote:
> > Just so I understand, does the shape of how 10450002 evolves affect this
> > change?
> > or will this be the same regardless?
> >
> > my one nit is that in two places you comment that this base url is only
> used
> > in
> > android. but given that it's not behind an ifdef, any platform can call
> it
> > now.
> > so i would just leave out the android comments.
> >
> > http://codereview.chromium.org/10544175/
>

[Charlie Reis, 2012-06-15 23:10:02]

https://chromiumcodereview.appspot.com/10544175/diff/1/content/browser/web_co...
File content/browser/web_contents/navigation_controller_impl.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/1/content/browser/web_co...
content/browser/web_contents/navigation_controller_impl.h:66: virtual void
LoadDataWithBaseURL(const GURL& data_url,
This seems to conflict with the LoadURL vs LoadURLFromRenderer distinction,
which take in the same parameters but differ in whether they show pending
navigations in the omnibox (to prevent URL spoofs).

If it's ever possible for a renderer to send this type of navigation request to
the browser (via RenderViewImpl::decidePolicyForNavigation and OpenURL), then
this approach won't work, and you'll need to somehow pass the new parameters
into both LoadURL and LoadURLFromRenderer.

Do you know that this can only be used for browser-initiated navigations?

[mnaganov (inactive), 2012-06-18 10:39:08]

https://chromiumcodereview.appspot.com/10544175/diff/1/content/browser/web_co...
File content/browser/web_contents/navigation_controller_impl.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/1/content/browser/web_co...
content/browser/web_contents/navigation_controller_impl.h:66: virtual void
LoadDataWithBaseURL(const GURL& data_url,
On 2012/06/15 23:10:02, creis wrote:
> This seems to conflict with the LoadURL vs LoadURLFromRenderer distinction,
> which take in the same parameters but differ in whether they show pending
> navigations in the omnibox (to prevent URL spoofs).
> 
> If it's ever possible for a renderer to send this type of navigation request
to
> the browser (via RenderViewImpl::decidePolicyForNavigation and OpenURL), then
> this approach won't work, and you'll need to somehow pass the new parameters
> into both LoadURL and LoadURLFromRenderer.
> 
> Do you know that this can only be used for browser-initiated navigations?

Yes, for sure. This way of page loading can be only initiated from the browser
side. Clicks in the page loaded via this method are handled in an usual way --
they don't inherit base or history urls specified here.

[Charlie Reis, 2012-06-18 18:34:29]

Seems reasonable, though I'd like to better distinguish these extra fields as
being specific to data URLs.  It's too easy to think they apply to every
navigation.

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/common/view...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/common/view...
content/common/view_messages.h:620: // Base URL for using in WebKit's
SubstituteData.
Please mention that these two are only needed for navigations to data URLs.

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/common/view...
content/common/view_messages.h:623: // History URL for using in WebKit's
SubstituteData.
Again, display_url seems to convey what it's being used for more clearly. 
Either way, it's important to note that it's only used for data URLs, since it
could easily be interpreted as applying to all navigations.

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
content/public/browser/navigation_controller.h:170: // Loads a 'data:' scheme
URL with specified base URL and a history entry URL.
"history entry URL" isn't clear to me.  It appears you're using this for the
virtual URL that gets displayed in the omnibox.  Perhaps display_url?

Also, please note in the comment that this can only be used for
browser-initiated navigations.

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
File content/public/browser/navigation_entry.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
content/public/browser/navigation_entry.h:53: virtual void SetBaseURL(const
GURL& url) = 0;
If this isn't used for any other type of navigation, what would you think about
{Get,Set}BaseURLForData?

[mnaganov (inactive), 2012-06-19 12:52:32]

Thanks for your comments, Charlie!

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/common/view...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/common/view...
content/common/view_messages.h:620: // Base URL for using in WebKit's
SubstituteData.
On 2012/06/18 18:34:29, creis wrote:
> Please mention that these two are only needed for navigations to data URLs.

Renamed to base_url_for_data

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/common/view...
content/common/view_messages.h:623: // History URL for using in WebKit's
SubstituteData.
On 2012/06/18 18:34:29, creis wrote:
> Again, display_url seems to convey what it's being used for more clearly. 
> Either way, it's important to note that it's only used for data URLs, since it
> could easily be interpreted as applying to all navigations.

Changed to history_url_for_data

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
content/public/browser/navigation_controller.h:170: // Loads a 'data:' scheme
URL with specified base URL and a history entry URL.
On 2012/06/18 18:34:29, creis wrote:
> "history entry URL" isn't clear to me.  It appears you're using this for the
> virtual URL that gets displayed in the omnibox.  Perhaps display_url?
> 
> Also, please note in the comment that this can only be used for
> browser-initiated navigations.

A comment added.

This URL isn't only displayed in the omnibox, it is actually used when the
client calls NavigationController::goBack.

You can see it yourself. This is a small patch for the content shell to use the
new method: http://pastebin.com/fACmV2gh

If you run patched shell like this:

./content_shell 'data:text/html,<html><body>Hello, World!</body></html>'
about:blank http://www.google.com

you'll see a "Hello, World!" page with google.com URL in the address bar. Then
if you'll navigate to some other page, and then press the "Go back" button,
you'll get to google.com page. This is the intended behavior, what Android's
WebView.loadDataWithBaseURL does.

I'm not sure how this is exactly implemented in WebKit, I only know that the
backwards navigation backwards goes through WebFrame::loadHistoryItem and it
seems to do the right thing.

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
File content/public/browser/navigation_entry.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
content/public/browser/navigation_entry.h:53: virtual void SetBaseURL(const
GURL& url) = 0;
On 2012/06/18 18:34:29, creis wrote:
> If this isn't used for any other type of navigation, what would you think
about
> {Get,Set}BaseURLForData?

Done. Also renamed the field from |base_url_| to |base_url_for_data_|

[Charlie Reis, 2012-06-19 17:17:24]

Thanks for the updates and explanation.

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
content/public/browser/navigation_controller.h:170: // Loads a 'data:' scheme
URL with specified base URL and a history entry URL.
On 2012/06/19 12:52:32, Mikhail Naganov (Chromium) wrote:
> This URL isn't only displayed in the omnibox, it is actually used when the
> client calls NavigationController::goBack.
> 
> You can see it yourself. This is a small patch for the content shell to use
the
> new method: http://pastebin.com/fACmV2gh
> 
> If you run patched shell like this:
> 
> ./content_shell 'data:text/html,<html><body>Hello, World!</body></html>'
> about:blank http://www.google.com
> 
> you'll see a "Hello, World!" page with http://google.com URL in the address
bar. Then
> if you'll navigate to some other page, and then press the "Go back" button,
> you'll get to http://google.com page. This is the intended behavior, what
Android's
> WebView.loadDataWithBaseURL does.
> 
> I'm not sure how this is exactly implemented in WebKit, I only know that the
> backwards navigation backwards goes through WebFrame::loadHistoryItem and it
> seems to do the right thing.

Hmm.  The name is fine, then, but this does scare me.  This is a recipe for URL
spoof vulnerabilities if used improperly.

What can we do to enforce that "history_url" can't be misused?  Can we put a
CHECK into RenderViewHostImpl::Navigate that params.history_url_for_data is only
present if params.url has a data scheme?

More importantly, are there any limitations on what can be put into history_url?
 (I'm quite uncomfortable with putting up fake content for arbitrary URLs like
http://www.google.com.)

https://chromiumcodereview.appspot.com/10544175/diff/14002/content/public/bro...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/14002/content/public/bro...
content/public/browser/navigation_controller.h:171: // This can only be used for
browser-initiated navigations.
Thanks.  Now that I know a bit more, I think we need to provide a stronger
warning here.  Something like "This is only safe to be used for
browser-initiated data: URL navigations, since it shows arbitrary content as if
it comes from |history_url|."

[mnaganov (inactive), 2012-07-03 08:07:51]

https://chromiumcodereview.appspot.com/10544175/diff/14002/content/public/bro...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/14002/content/public/bro...
content/public/browser/navigation_controller.h:171: // This can only be used for
browser-initiated navigations.
On 2012/06/19 17:17:25, creis wrote:
> Thanks.  Now that I know a bit more, I think we need to provide a stronger
> warning here.  Something like "This is only safe to be used for
> browser-initiated data: URL navigations, since it shows arbitrary content as
if
> it comes from |history_url|."

Done. Sorry, I was swamped by other tasks, now I'm returning back to this issue.

[Charlie Reis, 2012-07-03 17:54:36]

Thanks for the updates.  Still have a question about how to limit what can be
put in history_url, plus a few comments.

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/5001/content/public/brow...
content/public/browser/navigation_controller.h:170: // Loads a 'data:' scheme
URL with specified base URL and a history entry URL.
On 2012/06/19 17:17:24, creis wrote:
> Hmm.  The name is fine, then, but this does scare me.  This is a recipe for
URL
> spoof vulnerabilities if used improperly.
> 
> What can we do to enforce that "history_url" can't be misused?  Can we put a
> CHECK into RenderViewHostImpl::Navigate that params.history_url_for_data is
only
> present if params.url has a data scheme?

Thanks, I see that you added a check for params.url having a data scheme.

> More importantly, are there any limitations on what can be put into
history_url?
>  (I'm quite uncomfortable with putting up fake content for arbitrary URLs like
> http://www.google.com.)

What about this part?  I'm still concerned about showing fake content for
otherwise trusted web URLs.  At the moment, it looks like this could even be
used to spoof HTTPS URLs.

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:310: // 'ftp', 'about' or
'javascript'.
This doesn't make sense to me.  Why would we grant access to file URLs if it
doesn't match this list?  What if additional schemes are added in the future?

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/we...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/we...
content/browser/web_contents/navigation_controller_impl.cc:634:
DCHECK(data_url.SchemeIs(chrome::kDataScheme));
Alternatively, just put a NOTREACHED inside the "if" below.

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/renderer/r...
content/renderer/render_view_impl.cc:1058: if (net::DataURL::Parse(params.url,
&mime_type, &charset, &data)) {
If this parsing fails (e.g., someone later tries to set base_url_for_data_url on
the wrong type of URL), the navigation will just be ignored.  Speaking from
experience, that leads to bug reports that are very hard to diagnose.

If we expect this to never fail, can we crash or do something similar that will
make such a bug easier to track down?

[mnaganov (inactive), 2012-07-04 15:25:57]

Thank you very much for your comments, Charlie!

As for the history URL -- we can't impose any restrictions on it, since we are
implementing an existing API, which doesn't restrict anything. What we have
decided is to separate this method into an Android-specific interface. What do
you think about this idea?

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:310: // 'ftp', 'about' or
'javascript'.
On 2012/07/03 17:54:37, creis wrote:
> This doesn't make sense to me.  Why would we grant access to file URLs if it
> doesn't match this list?  What if additional schemes are added in the future?

This is according to http://goo.gl/X8HOs. As the main URL uses the 'data:'
scheme, client security policy forbids it from loading 'file:' URLs. I think the
main use case for this is allowing to access 'file:' URLs when the base URL is
also 'file:', although the documentation doesn't state this directly.

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/we...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/we...
content/browser/web_contents/navigation_controller_impl.cc:634:
DCHECK(data_url.SchemeIs(chrome::kDataScheme));
On 2012/07/03 17:54:37, creis wrote:
> Alternatively, just put a NOTREACHED inside the "if" below.

Done, thanks for the suggestion.

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/renderer/r...
content/renderer/render_view_impl.cc:1058: if (net::DataURL::Parse(params.url,
&mime_type, &charset, &data)) {
On 2012/07/03 17:54:37, creis wrote:
> If this parsing fails (e.g., someone later tries to set base_url_for_data_url
on
> the wrong type of URL), the navigation will just be ignored.  Speaking from
> experience, that leads to bug reports that are very hard to diagnose.
> 
> If we expect this to never fail, can we crash or do something similar that
will
> make such a bug easier to track down?

Added an explicit bailout.

[Charlie Reis, 2012-07-10 20:54:19]

Sorry for the delay-- just got back from vacation.


On 2012/07/04 15:25:57, Mikhail Naganov (Chromium) wrote:
> As for the history URL -- we can't impose any restrictions on it, since we are
> implementing an existing API, which doesn't restrict anything. What we have
> decided is to separate this method into an Android-specific interface. What do
> you think about this idea?

I see.  Yes, I suppose that better indicates the purpose of the method and
somewhat cuts down on the chance it will be misused, though we'll still have to
be careful with it.  If the owners review is ok with the new approach, then I am
too.

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:310: // 'ftp', 'about' or
'javascript'.
On 2012/07/04 15:25:57, Mikhail Naganov (Chromium) wrote:
> On 2012/07/03 17:54:37, creis wrote:
> > This doesn't make sense to me.  Why would we grant access to file URLs if it
> > doesn't match this list?  What if additional schemes are added in the
future?
> 
> This is according to http://goo.gl/X8HOs. As the main URL uses the 'data:'
> scheme, client security policy forbids it from loading 'file:' URLs. I think
the
> main use case for this is allowing to access 'file:' URLs when the base URL is
> also 'file:', although the documentation doesn't state this directly.

Hmm.  This sounds like it's following the letter of the law if not the spirit. 
Again, Chrome may support more schemes than Android's WebView, so this blacklist
approach may grant file:// privileges more often than the doc writers meant to. 
I'd feel much more comfortable only granting file:// privileges if the base URL
is file://.  If that turns out to be a bug in a corner case, we can revisit it.

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/browser/we...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/browser/we...
content/browser/web_contents/navigation_entry_impl.h:192: // Used for specifying
base URL for pages loaded via data URLs. Not persisted.
Sanity check: why is this safe to not persist?  Is it because Android WebViews
never use session or tab restore?  Will that always be true?

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/public/bro...
File content/public/browser/android/navigation_controller_webview.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/public/bro...
content/public/browser/android/navigation_controller_webview.h:15: // An
extension of NavigationController adding methods required to implement
nit: Let's use a different word than "extension," which has a more concrete
meaning in Chrome.

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/public/bro...
content/public/browser/android/navigation_controller_webview.h:21: // since it
shows arbitrary content as if it comes from |history_url|.
Is there any clean way to point to the Android WebView documentation here, or at
least make a reference to it?  I found the API description there helpful for
understanding why this is needed.

[mnaganov (inactive), 2012-07-23 14:02:29]

I was on vacation as well, just have returned.

Charlie, thank you very much for your comments, I've addressed them.

John (jam@), can you please take another look to comment on API changes?

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10544175/diff/25001/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:310: // 'ftp', 'about' or
'javascript'.
On 2012/07/10 20:54:19, creis wrote:
> On 2012/07/04 15:25:57, Mikhail Naganov (Chromium) wrote:
> > On 2012/07/03 17:54:37, creis wrote:
> > > This doesn't make sense to me.  Why would we grant access to file URLs if
it
> > > doesn't match this list?  What if additional schemes are added in the
> future?
> > 
> > This is according to http://goo.gl/X8HOs. As the main URL uses the 'data:'
> > scheme, client security policy forbids it from loading 'file:' URLs. I think
> the
> > main use case for this is allowing to access 'file:' URLs when the base URL
is
> > also 'file:', although the documentation doesn't state this directly.
> 
> Hmm.  This sounds like it's following the letter of the law if not the spirit.

> Again, Chrome may support more schemes than Android's WebView, so this
blacklist
> approach may grant file:// privileges more often than the doc writers meant
to. 
> I'd feel much more comfortable only granting file:// privileges if the base
URL
> is file://.  If that turns out to be a bug in a corner case, we can revisit
it.

I agree -- whitelisting allowed schemes is more secure than blacklisting. I
myself was surprised with the wording in the documentation.

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/browser/we...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/browser/we...
content/browser/web_contents/navigation_entry_impl.h:192: // Used for specifying
base URL for pages loaded via data URLs. Not persisted.
On 2012/07/10 20:54:19, creis wrote:
> Sanity check: why is this safe to not persist?  Is it because Android WebViews
> never use session or tab restore?  Will that always be true?

WebView currently uses the single-process model (for compatibility), so a crash
in renderer will kick out the whole application that uses WebView. And Android
WebView doesn't offer renderer state saving, so I think this should be fine.

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/public/bro...
File content/public/browser/android/navigation_controller_webview.h (right):

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/public/bro...
content/public/browser/android/navigation_controller_webview.h:15: // An
extension of NavigationController adding methods required to implement
On 2012/07/10 20:54:19, creis wrote:
> nit: Let's use a different word than "extension," which has a more concrete
> meaning in Chrome.

Done.

https://chromiumcodereview.appspot.com/10544175/diff/19011/content/public/bro...
content/public/browser/android/navigation_controller_webview.h:21: // since it
shows arbitrary content as if it comes from |history_url|.
On 2012/07/10 20:54:19, creis wrote:
> Is there any clean way to point to the Android WebView documentation here, or
at
> least make a reference to it?  I found the API description there helpful for
> understanding why this is needed.

I've added a reference to the class-level comment.

[Charlie Reis, 2012-07-23 21:05:21]

Thanks Mikhail!  LGTM.

[jam, 2012-07-23 22:52:44]

lgtm