Don't call DidLoadResourceFromMemoryCache() on data: URLs.

Don't call DidLoadResourceFromMemoryCache() on data: URLs.

These can cause crashes in IPC on large data URLs. Chrome has no use for the long data: URLs, so there's no reason to send them.

BUG=95747
TEST=See bug.

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

[James Simonsen, 2011-10-27 19:36:00]

Adam, I saw your name on SSLPolicy::OnRequestStarted(). These large data: URLs
ultimately end up there. Do you think it's okay just to ignore them for the
purposes of SSLPolicy::OnRequestStarted()? Or should I try a different
technique?

If you're okay with it, I'll find an OWNER to approve it.

[abarth-chromium, 2011-10-27 20:36:35]

http://codereview.chromium.org/8341076/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8341076/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:2770: }
I guess this is ok.  It's kind of a hack though.  (Also, no braces on
single-line if statements.)  Is there something concrete we gain by doing this?

If you're going to do this, please add a big comment explaining why we're doing
this.

[James Simonsen, 2011-10-27 20:42:07]

On 2011/10/27 20:36:35, abarth wrote:
>
http://codereview.chromium.org/8341076/diff/1/content/renderer/render_view_im...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/8341076/diff/1/content/renderer/render_view_im...
> content/renderer/render_view_impl.cc:2770: }
> I guess this is ok.  It's kind of a hack though.  (Also, no braces on
> single-line if statements.)  Is there something concrete we gain by doing
this?

It doesn't crash the IPC code. :) It's also silly to send the entire contents of
an image across IPC in a data: URL since that URL will never be used. The only
purpose for the URL is to display it as insecure content. I'm open to
alternatives:

I wouldn't mind not skipping didLoadResourceFromMemoryCache() for data: URLs in
WebCore. Do you think any other ports would mind?

Another idea is just to strip out the content of the data: URL and continue to
send the IPC.

> If you're going to do this, please add a big comment explaining why we're
doing
> this.

Sure.

[James Simonsen, 2011-10-27 20:43:11]

On 2011/10/27 20:42:07, James Simonsen wrote:
> I wouldn't mind not skipping didLoadResourceFromMemoryCache() for data: URLs
in

Oops, there's an extra "not" there. Should be, "I wouldn't mind skipping..."

[abarth-chromium, 2011-10-27 20:44:50]

Not-crashing sounds like a solid reason to have this sort of hack.  I missed
that in your patch description.  I'm happy if you add a comment explaining that
next to the branch.

Thanks!

[James Simonsen, 2011-10-27 21:07:10]

Antoine or James: Can you please give an OWNERs lgtm for this? Adam has already
signed off on it as the other consumer (besides me) of this message.

[jamesr, 2011-10-27 21:09:31]

owners LGTM

Could we use a whitelist of schemes here instead of a blacklist?

[piman, 2011-10-27 21:11:44]

lgtm

http://codereview.chromium.org/8341076/diff/3001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8341076/diff/3001/content/renderer/render_view...
content/renderer/render_view_impl.cc:2772: }
We usually skip braces if we only have one statement in the if block.

[James Simonsen, 2011-10-27 21:15:16]

On 2011/10/27 21:11:44, piman wrote:
> lgtm
> 
>
http://codereview.chromium.org/8341076/diff/3001/content/renderer/render_view...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/8341076/diff/3001/content/renderer/render_view...
> content/renderer/render_view_impl.cc:2772: }
> We usually skip braces if we only have one statement in the if block.

Done.

[commit-bot: I haz the power, 2011-10-27 23:06:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/8341076/1005

[commit-bot: I haz the power, 2011-10-28 00:18:54]

Try job failure for 8341076-1005 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...