Allow huge data: URIs only via WebView.loadDataWithBaseUrl

Allow huge data: URIs only via WebView.loadDataWithBaseUrl

This CL removes WebView hack for allowing 20MB URLs to
be passed via IPC.

To maintain compatibility with legacy apps, passing long
data: URIs is still allowed via WebView.loadDataWithBaseUrl,
which passes them as a string to circumvent the URL
length limit. Data URI only goes from the browser side
to the renderer side, but not back, because "history url"
(or "virtual url" in Chromium terms) is used for representing
the URL in other IPC messages.

BUG=525697
Committed: https://crrev.com/bf08742205e9c6be216135b2466129614b674adf
Cr-Commit-Position: refs/heads/master@{#365454}

[mnaganov (inactive), 2015-12-08 21:51:07]

Description was changed from

==========
[WIP] Pass huge data: URIs via shared memory

BUG=525697
==========

to

==========
Allow huge data: URIs only via WebView.loadDataWithBaseUrl

This CL removes WebView hack for allowing 20MB URLs to
be passed via IPC.

To maintain compatibility with legacy apps, passing long
data: URIs is still allowed via WebView.loadDataWithBaseUrl,
which passes them as a string to circumvent the URL
length limit.

BUG=525697
==========

[mnaganov (inactive), 2015-12-08 23:34:41]

The CQ bit was checked by mnaganov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 23:43:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497743005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497743005/60001

[mnaganov (inactive), 2015-12-08 23:50:43]

Description was changed from

==========
Allow huge data: URIs only via WebView.loadDataWithBaseUrl

This CL removes WebView hack for allowing 20MB URLs to
be passed via IPC.

To maintain compatibility with legacy apps, passing long
data: URIs is still allowed via WebView.loadDataWithBaseUrl,
which passes them as a string to circumvent the URL
length limit.

BUG=525697
==========

to

==========
Allow huge data: URIs only via WebView.loadDataWithBaseUrl

This CL removes WebView hack for allowing 20MB URLs to
be passed via IPC.

To maintain compatibility with legacy apps, passing long
data: URIs is still allowed via WebView.loadDataWithBaseUrl,
which passes them as a string to circumvent the URL
length limit. Data URI only goes from the browser side
to the renderer side, but not back, because "history url"
(or "virtual url" in Chromium terms) is used for representing
the URL in other IPC messages.

BUG=525697
==========

[commit-bot: I haz the power, 2015-12-09 00:13:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 00:13:59]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mnaganov (inactive), 2015-12-09 00:53:58]

The CQ bit was checked by mnaganov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 01:01:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497743005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497743005/80001

[commit-bot: I haz the power, 2015-12-09 03:05:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 03:05:47]

Dry run: This issue passed the CQ dry run.

[mnaganov (inactive), 2015-12-09 17:35:54]

mnaganov@chromium.org changed reviewers:
+ boliu@chromium.org

[mnaganov (inactive), 2015-12-09 17:35:54]

Bo, please do sanity check for the CL.

[boliu, 2015-12-09 19:33:02]

Details look fine, but then let's see what owner and security people think.

Two high level comments

This patch *adds* a new way to do LoadDataWithBaseURL, rather than replacing the
existing path. The <webview> tag also has a LoadDataWithBaseURL and it still
passes data through URL in NavigationEntry:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
Should fix <webview> too, and then only allow this path

Second point is semi-related: after above is done, the base url can be
completely removed and be stored in the URL field. Then basically my fix for the
data url reload issue can be reverted.

https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:799:
entry->SetDataURLAsString(params.data_url_as_string);
probably want to assert here that GetURL is empty?

https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:426: // and persisted by
Android WebView.
<webview> tag also has a LoadDataWithBaseURL

https://codereview.chromium.org/1497743005/diff/80001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1497743005/diff/80001/content/common/navigati...
content/common/navigation_params.h:107: // to renderer, not vice versa.
(only read up to this point) how is renderer->browser avoided?

(after finish reading everything) Sounds like a soft guarantee. Is everything
renderer->browser constructed from MakeCommonNavigationParams in
render_frame_impl.cc?

[mnaganov (inactive), 2015-12-09 21:14:32]

On 2015/12/09 19:33:02, boliu wrote:
> Details look fine, but then let's see what owner and security people think.
> 
> Two high level comments
> 
> This patch *adds* a new way to do LoadDataWithBaseURL, rather than replacing
the
> existing path. The <webview> tag also has a LoadDataWithBaseURL and it still
> passes data through URL in NavigationEntry:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> Should fix <webview> too, and then only allow this path
>

I don't think that <webview> needs to be fixed. We make this change for
the purpose of legacy compatibility with Android WebView. The <webview>
didn't have this problem in the first place.

I would surround the new field with #ifdef(OS_ANDROID), but that would
make the code complicated. 
 
> Second point is semi-related: after above is done, the base url can be
> completely removed and be stored in the URL field. Then basically my fix for
the
> data url reload issue can be reverted.
>

But this is a good point actually, and this will require changing
<webview> to also pass the actual data URI in the new field. So I
would postpone the proposal to change <webview> code until the
moment we got to it.
 
>
https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/navigation_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_controller_impl.cc:799:
> entry->SetDataURLAsString(params.data_url_as_string);
> probably want to assert here that GetURL is empty?
>

This will not be true now, and we go with the proposal of removing
the base URL field, will not be needed.
 
>
https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/navigation_entry_impl.h (right):
> 
>
https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_entry_impl.h:426: // and persisted by
> Android WebView.
> <webview> tag also has a LoadDataWithBaseURL
> 
>
https://codereview.chromium.org/1497743005/diff/80001/content/common/navigati...
> File content/common/navigation_params.h (right):
> 
>
https://codereview.chromium.org/1497743005/diff/80001/content/common/navigati...
> content/common/navigation_params.h:107: // to renderer, not vice versa.
> (only read up to this point) how is renderer->browser avoided?
> 
> (after finish reading everything) Sounds like a soft guarantee. Is everything
> renderer->browser constructed from MakeCommonNavigationParams in
> render_frame_impl.cc?

Renderer can only send CommonNavigationParams via FrameHostMsg_BeginNavigation.
Also, we don't read the new field in the browser code. But I doubt this
can be enforced programmatically.

[boliu, 2015-12-09 21:17:50]

On 2015/12/09 21:14:32, mnaganov wrote:
> On 2015/12/09 19:33:02, boliu wrote:
> > Details look fine, but then let's see what owner and security people think.
> > 
> > Two high level comments
> > 
> > This patch *adds* a new way to do LoadDataWithBaseURL, rather than replacing
> the
> > existing path. The <webview> tag also has a LoadDataWithBaseURL and it still
> > passes data through URL in NavigationEntry:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > Should fix <webview> too, and then only allow this path
> >
> 
> I don't think that <webview> needs to be fixed. We make this change for
> the purpose of legacy compatibility with Android WebView. The <webview>
> didn't have this problem in the first place.
> 
> I would surround the new field with #ifdef(OS_ANDROID), but that would
> make the code complicated. 
>  
> > Second point is semi-related: after above is done, the base url can be
> > completely removed and be stored in the URL field. Then basically my fix for
> the
> > data url reload issue can be reverted.
> >
> 
> But this is a good point actually, and this will require changing
> <webview> to also pass the actual data URI in the new field. So I
> would postpone the proposal to change <webview> code until the
> moment we got to it.
>  
> >
>
https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
> > File content/browser/frame_host/navigation_controller_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
> > content/browser/frame_host/navigation_controller_impl.cc:799:
> > entry->SetDataURLAsString(params.data_url_as_string);
> > probably want to assert here that GetURL is empty?
> >
> 
> This will not be true now, and we go with the proposal of removing
> the base URL field, will not be needed.

Oops, I meant ehh, only one of GetURL and DataURLAsString is non-empty.

>  
> >
>
https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
> > File content/browser/frame_host/navigation_entry_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/1497743005/diff/80001/content/browser/frame_h...
> > content/browser/frame_host/navigation_entry_impl.h:426: // and persisted by
> > Android WebView.
> > <webview> tag also has a LoadDataWithBaseURL
> > 
> >
>
https://codereview.chromium.org/1497743005/diff/80001/content/common/navigati...
> > File content/common/navigation_params.h (right):
> > 
> >
>
https://codereview.chromium.org/1497743005/diff/80001/content/common/navigati...
> > content/common/navigation_params.h:107: // to renderer, not vice versa.
> > (only read up to this point) how is renderer->browser avoided?
> > 
> > (after finish reading everything) Sounds like a soft guarantee. Is
everything
> > renderer->browser constructed from MakeCommonNavigationParams in
> > render_frame_impl.cc?
> 
> Renderer can only send CommonNavigationParams via
FrameHostMsg_BeginNavigation.
> Also, we don't read the new field in the browser code. But I doubt this
> can be enforced programmatically.

[mnaganov (inactive), 2015-12-09 21:20:47]

mnaganov@chromium.org changed reviewers:
+ tsepez@chromium.org

[mnaganov (inactive), 2015-12-09 21:20:48]

Hi Tom,

This is a respin of https://codereview.chromium.org/84703003/ which you were
examining some time ago. Please note that in exchange of adding a new field we
are removing the previous hack of overriding the max GURL length. We may also
remove the base url field as Bo suggests.

We agree that it actually seems ineffective for the apps to pass large amounts
of data to the renderer this way, that's why the implementation is intentionally
simplistic. We only leave this way for backwards compatibility, as a lot of
well-known apps will stop working if we not allow them to load huge data: uris.

[Tom Sepez, 2015-12-09 21:30:14]

LGTM with comment.

https://codereview.chromium.org/1497743005/diff/80001/android_webview/native/...
File android_webview/native/state_serializer_unittest.cc (right):

https://codereview.chromium.org/1497743005/diff/80001/android_webview/native/...
android_webview/native/state_serializer_unittest.cc:55: const string
data_url_as_string("data:text/html;charset=utf-8;base64,");
can we get a test of a very large string here as well (one that would flunk the
old 2MB limit)?

[mnaganov (inactive), 2015-12-09 22:22:53]

https://codereview.chromium.org/1497743005/diff/80001/android_webview/native/...
File android_webview/native/state_serializer_unittest.cc (right):

https://codereview.chromium.org/1497743005/diff/80001/android_webview/native/...
android_webview/native/state_serializer_unittest.cc:55: const string
data_url_as_string("data:text/html;charset=utf-8;base64,");
On 2015/12/09 21:30:14, Tom Sepez wrote:
> can we get a test of a very large string here as well (one that would flunk
the
> old 2MB limit)?

Done.

[mnaganov (inactive), 2015-12-09 22:28:26]

mnaganov@chromium.org changed reviewers:
+ creis@chromium.org

[mnaganov (inactive), 2015-12-09 22:28:26]

Hi Charlie,

Can you please take a look at this CL?

[Charlie Reis, 2015-12-10 20:43:46]

I'd really like to consider alternatives to this approach.

I recognize that you're trying to remove the exception Webview makes today which
allows any URL to be 20MB.  I am strongly in support of that goal, and I would
have opposed the change that made the exception
(https://codereview.chromium.org/112053002/).

That said, this continues to have the problem that large amounts of data are
stored on NavigationEntry, and NavigationEntry isn't meant for that.  We clone
them in several places, pass the values between processes, etc.  It's not at all
clear to me what effect passing around arbitrarily large data here has on
overall memory use, or how many copies will exist.

It seems like the network stack is better suited towards handling large amounts
of data in general, so I wonder if there's a way to use that to our advantage. 
I don't know if there's a way to get the value from the cache (using a boolean
here rather than the actual data), or implement it via something like blob URLs
under the hood, but storing it all in a string on NavigationEntry seems like
something we wouldn't want to keep around long term.

Would something like that be a viable alternative?

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:543:
EXPECT_EQ(load_params.data_url_as_string, entry->GetDataURLAsString());
Can we also check that if data_url_as_string is specified, that GetURL is a
prefix of it?

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:591:
load_params.data_url_as_string = "data:text/html;charset=utf-8;base64,";
Why doesn't this match the specified URL (data:text/html,dataurl)?

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:215: data_url_as_string_ =
url;
Seems like we should be enforcing this starts with "data:", at least.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:85: void
SetDataURLAsString(const std::string& url) override;
boliu@ and I spent a lot of time avoiding a new member for the data URL for
https://crbug.com/522567.  I'm sad that we're having to add it (as he predicted
in https://code.google.com/p/chromium/issues/detail?id=525697#c18).

This seems like one more way for NavigationEntries to have inconsistent
information (e.g., this vs GetURL), with subtle bugs if they aren't populated
correctly or checked consistently.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:426: // and persisted by
Android WebView.
I'm concerned about the "only Android WebView" comment without having it in an
#ifdef.  The rest of the LoadDataForBaseURL feature leaked to the Chrome Apps
<webview> implementation, so these comments aren't enforceable lead to uses of
this bad API in other parts of Chrome.

If we keep this, can we put it (and the new methods) in an #ifdef?  I don't
think we want to support this hack for LoadDataForBaseURL inside Chrome Apps
<webview>.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:427: std::string
data_url_as_string_;
I'd like to consider alternatives to this if we can, as I mention in my overall
CL comment.

https://codereview.chromium.org/1497743005/diff/100001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1497743005/diff/100001/content/common/navigat...
content/common/navigation_params.h:107: // to renderer, not vice versa.
This is not the right place to put a parameter that is only passed from browser
to renderer.  CommonNavigationParams goes in both directions. 
RequestNavigationParams is from the browser to renderer (assuming it would be
used both by default and in PlzNavigate).

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5147: : GURL(params.data_url_as_string),
This is the kind of check I'm concerned about, which will likely have bugs in
the future.  LoadDataURL is only called if params.url has a data scheme, but
there's nothing guaranteeing that params.data_url_as_string matches params.url
or has a data scheme.  Any other checks we did on params.url similarly won't
have applied to data_url_as_string.

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:564: // Check post data sent to
browser matches
nit: End with period.

[mnaganov (inactive), 2015-12-11 23:14:24]

Charlie, I really appreciated your feedback here!

I decided to do the following things in order to address your major concerns:

 1) Make this code Android-specific (to rule out using for the <webview> tag);

 2) Use RefCountedString to address NavigationEntry copying issues;

 3) Add more checks to ensure that the new field is only used in the particular
scenario -- as a transport for passing a huge url to renderer.

I think that any more elaborate approach (e.g. introducing a "registry" of data:
urls, like for blob: urls; or involving the network stack) would only be needed
if we tried to solve the huge data: urls problem in the general case (i.e. to
allow using them in loadUrl('data:') or loadData() methods of WebView). I'm
hoping, that the current solution should work for the majority of our clients,
so we will not need to do that.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:543:
EXPECT_EQ(load_params.data_url_as_string, entry->GetDataURLAsString());
On 2015/12/10 20:43:46, Charlie Reis wrote:
> Can we also check that if data_url_as_string is specified, that GetURL is a
> prefix of it?

I added checks into navigation_controller_android (as not it's Android-specific
code). I think, we don't need to check for prefix, as the main point is that
GetURL needs to have empty data.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:591:
load_params.data_url_as_string = "data:text/html;charset=utf-8;base64,";
On 2015/12/10 20:43:46, Charlie Reis wrote:
> Why doesn't this match the specified URL (data:text/html,dataurl)?

Done.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:215: data_url_as_string_ =
url;
On 2015/12/10 20:43:46, Charlie Reis wrote:
> Seems like we should be enforcing this starts with "data:", at least.

Done in navigation_controller_android.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:85: void
SetDataURLAsString(const std::string& url) override;
On 2015/12/10 20:43:46, Charlie Reis wrote:
> boliu@ and I spent a lot of time avoiding a new member for the data URL for
> https://crbug.com/522567.  I'm sad that we're having to add it (as he
predicted
> in https://code.google.com/p/chromium/issues/detail?id=525697#c18).
> 
> This seems like one more way for NavigationEntries to have inconsistent
> information (e.g., this vs GetURL), with subtle bugs if they aren't populated
> correctly or checked consistently.

I hope I added enough checks to enforce consistency.

We could get rid of the base URL field (put its contents into the main URL
field). But as I'm now making 'DataURLAsString' to be specific to Android, this
will not be possible.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:426: // and persisted by
Android WebView.
On 2015/12/10 20:43:46, Charlie Reis wrote:
> I'm concerned about the "only Android WebView" comment without having it in an
> #ifdef.  The rest of the LoadDataForBaseURL feature leaked to the Chrome Apps
> <webview> implementation, so these comments aren't enforceable lead to uses of
> this bad API in other parts of Chrome.
> 
> If we keep this, can we put it (and the new methods) in an #ifdef?  I don't
> think we want to support this hack for LoadDataForBaseURL inside Chrome Apps
> <webview>.

Now it's enforced by OS_ANDROID defines.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:427: std::string
data_url_as_string_;
On 2015/12/10 20:43:46, Charlie Reis wrote:
> I'd like to consider alternatives to this if we can, as I mention in my
overall
> CL comment.

Considered.

https://codereview.chromium.org/1497743005/diff/100001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1497743005/diff/100001/content/common/navigat...
content/common/navigation_params.h:107: // to renderer, not vice versa.
On 2015/12/10 20:43:46, Charlie Reis wrote:
> This is not the right place to put a parameter that is only passed from
browser
> to renderer.  CommonNavigationParams goes in both directions. 
> RequestNavigationParams is from the browser to renderer (assuming it would be
> used both by default and in PlzNavigate).

Done.

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5147: : GURL(params.data_url_as_string),
On 2015/12/10 20:43:47, Charlie Reis wrote:
> This is the kind of check I'm concerned about, which will likely have bugs in
> the future.  LoadDataURL is only called if params.url has a data scheme, but
> there's nothing guaranteeing that params.data_url_as_string matches params.url
> or has a data scheme.  Any other checks we did on params.url similarly won't
> have applied to data_url_as_string.

Added more checks to ensure that the main url's data is empty, and that 
data_url_as_string is really a data url.

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
content/renderer/render_view_browsertest.cc:564: // Check post data sent to
browser matches
On 2015/12/10 20:43:47, Charlie Reis wrote:
> nit: End with period.

Done.

[Charlie Reis, 2015-12-14 20:35:56]

On 2015/12/11 23:14:24, mnaganov wrote:
> Charlie, I really appreciated your feedback here!
> 
> I decided to do the following things in order to address your major concerns:
> 
>  1) Make this code Android-specific (to rule out using for the <webview> tag);
> 
>  2) Use RefCountedString to address NavigationEntry copying issues;
> 
>  3) Add more checks to ensure that the new field is only used in the
particular
> scenario -- as a transport for passing a huge url to renderer.
> 
> I think that any more elaborate approach (e.g. introducing a "registry" of
data:
> urls, like for blob: urls; or involving the network stack) would only be
needed
> if we tried to solve the huge data: urls problem in the general case (i.e. to
> allow using them in loadUrl('data:') or loadData() methods of WebView). I'm
> hoping, that the current solution should work for the majority of our clients,
> so we will not need to do that.

Thanks for making these changes, and for your thoughts on other approaches. 
I'll allow this approach (without moving the data to the network stack), since
this does represent a nice improvement and should be reasonably safe with the
safeguards you've added.  It may not be ideal, but I also agree that it probably
isn't worth spending substantial time and complexity on this merely for
backwards compatibility.

A few thoughts/questions below, mostly nits.  We should be ready to go after
that.

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:215: data_url_as_string_ =
url;
On 2015/12/11 23:14:23, mnaganov wrote:
> On 2015/12/10 20:43:46, Charlie Reis wrote:
> > Seems like we should be enforcing this starts with "data:", at least.
> 
> Done in navigation_controller_android.

That's a bit indirect-- navigation_controller_android is checking it before
assigning it to LoadUrlParams, but there's several places that
SetDataURLAsString is called.  I think it might make sense to put DCHECKs here
as well (or instead, if you prefer).

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5147: : GURL(params.data_url_as_string),
On 2015/12/11 23:14:24, mnaganov wrote:
> On 2015/12/10 20:43:47, Charlie Reis wrote:
> > This is the kind of check I'm concerned about, which will likely have bugs
in
> > the future.  LoadDataURL is only called if params.url has a data scheme, but
> > there's nothing guaranteeing that params.data_url_as_string matches
params.url
> > or has a data scheme.  Any other checks we did on params.url similarly won't
> > have applied to data_url_as_string.
> 
> Added more checks to ensure that the main url's data is empty, and that 
> data_url_as_string is really a data url.

Acknowledged.

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_android.cc:238: // Treat
|data_url_as_string| as if we were intenting to put it into a GURL
nit: intending

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:217:
scoped_refptr<base::RefCountedString> url) {
nit: s/url/data_url/

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:630: GURL
data_url(GetDataURLAsString()->data());
This makes a full copy, right?  If we're just doing this to validate, is it
possible to just pass a limited-size prefix?  I would imagine the rest of the
data wouldn't affect whether it's valid or what the scheme is.

That way we only have one full copy (into request_params.data_url_as_string
below), rather than two.

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:86: void
SetDataURLAsString(scoped_refptr<base::RefCountedString>) override;
nit: Please name parameter.  (data_url)

https://codereview.chromium.org/1497743005/diff/120001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1497743005/diff/120001/content/common/navigat...
content/common/navigation_params.cc:8: #include
"content/common/content_constants_internal.h"
Not needed anymore?

https://codereview.chromium.org/1497743005/diff/120001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1497743005/diff/120001/content/common/navigat...
content/common/navigation_params.h:306: // The real content of the data: URL.
Used to circumvent the restriction
nit: Can you add something about how this is only intended to be used for
LoadDataWithBaseURL (and that it's optional, such that short data URLs can be
put directly into the URL)?

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
content/public/browser/navigation_controller.h:166: // Used in LOAD_TYPE_DATA
loads only. The real data URI represented
nit: is represented

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
content/public/browser/navigation_controller.h:167: // as a string to circumvent
the restriction on GURL size.
nit: Can you mention something about it being optional, such that short data
URLs can be specified in |url|?

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
content/public/browser/navigation_entry.h:62: virtual void
SetDataURLAsString(scoped_refptr<base::RefCountedString>) = 0;
nit: Please name parameter data_url.

[mnaganov (inactive), 2015-12-15 18:30:29]

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_android.cc (right):

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_android.cc:238: // Treat
|data_url_as_string| as if we were intenting to put it into a GURL
On 2015/12/14 20:35:57, Charlie Reis wrote:
> nit: intending

Done.

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:217:
scoped_refptr<base::RefCountedString> url) {
On 2015/12/14 20:35:57, Charlie Reis wrote:
> nit: s/url/data_url/

Done.

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:630: GURL
data_url(GetDataURLAsString()->data());
On 2015/12/14 20:35:57, Charlie Reis wrote:
> This makes a full copy, right?  If we're just doing this to validate, is it
> possible to just pass a limited-size prefix?  I would imagine the rest of the
> data wouldn't affect whether it's valid or what the scheme is.
> 
> That way we only have one full copy (into request_params.data_url_as_string
> below), rather than two.

Good idea. Yes, GURL is only interested in the scheme, it doesn't go any
further.

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1497743005/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:86: void
SetDataURLAsString(scoped_refptr<base::RefCountedString>) override;
On 2015/12/14 20:35:57, Charlie Reis wrote:
> nit: Please name parameter.  (data_url)

Done.

https://codereview.chromium.org/1497743005/diff/120001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1497743005/diff/120001/content/common/navigat...
content/common/navigation_params.cc:8: #include
"content/common/content_constants_internal.h"
On 2015/12/14 20:35:57, Charlie Reis wrote:
> Not needed anymore?

Looks so, removed.

https://codereview.chromium.org/1497743005/diff/120001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1497743005/diff/120001/content/common/navigat...
content/common/navigation_params.h:306: // The real content of the data: URL.
Used to circumvent the restriction
On 2015/12/14 20:35:57, Charlie Reis wrote:
> nit: Can you add something about how this is only intended to be used for
> LoadDataWithBaseURL (and that it's optional, such that short data URLs can be
> put directly into the URL)?

Done.

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
content/public/browser/navigation_controller.h:166: // Used in LOAD_TYPE_DATA
loads only. The real data URI represented
On 2015/12/14 20:35:57, Charlie Reis wrote:
> nit: is represented

Done.

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
content/public/browser/navigation_controller.h:167: // as a string to circumvent
the restriction on GURL size.
On 2015/12/14 20:35:57, Charlie Reis wrote:
> nit: Can you mention something about it being optional, such that short data
> URLs can be specified in |url|?

Done.

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/1497743005/diff/120001/content/public/browser...
content/public/browser/navigation_entry.h:62: virtual void
SetDataURLAsString(scoped_refptr<base::RefCountedString>) = 0;
On 2015/12/14 20:35:57, Charlie Reis wrote:
> nit: Please name parameter data_url.

Done.

[mnaganov (inactive), 2015-12-15 18:34:05]

The CQ bit was checked by mnaganov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-15 18:35:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497743005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497743005/160001

[Charlie Reis, 2015-12-15 19:22:07]

Thanks.  LGTM with one earlier request below.

https://codereview.chromium.org/1497743005/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/160001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:218: data_url_as_string_ =
data_url;
I had an earlier comment about adding a safety check here, and it looks like it
slipped through the cracks:
https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...

https://codereview.chromium.org/1497743005/diff/160001/content/public/browser...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1497743005/diff/160001/content/public/browser...
content/public/browser/navigation_controller.h:168: // needed to pass URL that
exceed the IPC limit (kMaxURLChars). Short
nit: URLs

[mnaganov (inactive), 2015-12-15 20:30:49]

Thank you, Charlie!

https://codereview.chromium.org/1497743005/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1497743005/diff/160001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.cc:218: data_url_as_string_ =
data_url;
On 2015/12/15 19:22:07, Charlie Reis wrote:
> I had an earlier comment about adding a safety check here, and it looks like
it
> slipped through the cracks:
>
https://codereview.chromium.org/1497743005/diff/100001/content/browser/frame_...

Really sorry about missing that one! Thanks for pointing that out, added a
check.

https://codereview.chromium.org/1497743005/diff/160001/content/public/browser...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1497743005/diff/160001/content/public/browser...
content/public/browser/navigation_controller.h:168: // needed to pass URL that
exceed the IPC limit (kMaxURLChars). Short
On 2015/12/15 19:22:07, Charlie Reis wrote:
> nit: URLs

Done.

[mnaganov (inactive), 2015-12-15 20:31:02]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-12-15 20:31:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1497743005/#ps180001
(title: "Final comments addressed")

[commit-bot: I haz the power, 2015-12-15 20:34:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497743005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497743005/180001

[commit-bot: I haz the power, 2015-12-15 23:27:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 23:27:30]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[mnaganov (inactive), 2015-12-16 00:43:58]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-12-16 00:43:59]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1497743005/#ps200001
(title: "Fixed the test")

[commit-bot: I haz the power, 2015-12-16 00:45:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497743005/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497743005/200001

[commit-bot: I haz the power, 2015-12-16 02:52:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-16 02:52:58]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[mnaganov (inactive), 2015-12-16 04:46:38]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2015-12-16 04:47:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497743005/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497743005/200001

[commit-bot: I haz the power, 2015-12-16 05:10:11]

Description was changed from

==========
Allow huge data: URIs only via WebView.loadDataWithBaseUrl

This CL removes WebView hack for allowing 20MB URLs to
be passed via IPC.

To maintain compatibility with legacy apps, passing long
data: URIs is still allowed via WebView.loadDataWithBaseUrl,
which passes them as a string to circumvent the URL
length limit. Data URI only goes from the browser side
to the renderer side, but not back, because "history url"
(or "virtual url" in Chromium terms) is used for representing
the URL in other IPC messages.

BUG=525697
==========

to

==========
Allow huge data: URIs only via WebView.loadDataWithBaseUrl

This CL removes WebView hack for allowing 20MB URLs to
be passed via IPC.

To maintain compatibility with legacy apps, passing long
data: URIs is still allowed via WebView.loadDataWithBaseUrl,
which passes them as a string to circumvent the URL
length limit. Data URI only goes from the browser side
to the renderer side, but not back, because "history url"
(or "virtual url" in Chromium terms) is used for representing
the URL in other IPC messages.

BUG=525697
==========

[commit-bot: I haz the power, 2015-12-16 05:10:12]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-12-16 05:11:03]

Description was changed from

==========
Allow huge data: URIs only via WebView.loadDataWithBaseUrl

This CL removes WebView hack for allowing 20MB URLs to
be passed via IPC.

To maintain compatibility with legacy apps, passing long
data: URIs is still allowed via WebView.loadDataWithBaseUrl,
which passes them as a string to circumvent the URL
length limit. Data URI only goes from the browser side
to the renderer side, but not back, because "history url"
(or "virtual url" in Chromium terms) is used for representing
the URL in other IPC messages.

BUG=525697
==========

to

==========
Allow huge data: URIs only via WebView.loadDataWithBaseUrl

This CL removes WebView hack for allowing 20MB URLs to
be passed via IPC.

To maintain compatibility with legacy apps, passing long
data: URIs is still allowed via WebView.loadDataWithBaseUrl,
which passes them as a string to circumvent the URL
length limit. Data URI only goes from the browser side
to the renderer side, but not back, because "history url"
(or "virtual url" in Chromium terms) is used for representing
the URL in other IPC messages.

BUG=525697

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

[commit-bot: I haz the power, 2015-12-16 05:11:04]

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