Avoid GURL allocations/copies/frees in RenderFrameImpl::willSendRequest

Avoid GURL allocations/copies/frees in RenderFrameImpl::willSendRequest

ContentRendererClient::WillSendRequest has an API which takes the request
URL as a GURL. This entails an allocation, copy and free when the method
returns. Even for moderate size URLs this work adds up and is visible
in profiles.

This patch threads a blink::WebURL instead of the GURL, as consumers really
just want to know the scheme. This copy-free approach is thread to callers
when possible. Because the method is only used for SearchBox and Extension
URLs, most subresources will take the no-copy path.

Additionally, this patch early-exits before doing some non-trivial work.

BUG=348655
Committed: https://crrev.com/29d7eaf19413110f27cc101c7e2d7088102184a5
Cr-Commit-Position: refs/heads/master@{#428754}

[Charlie Harrison, 2016-10-28 21:39:19]

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

[commit-bot: I haz the power, 2016-10-28 21:40:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 21:53:45]

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

[commit-bot: I haz the power, 2016-10-28 21:53:46]

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

[Charlie Harrison, 2016-10-28 21:54:02]

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

[commit-bot: I haz the power, 2016-10-28 21:54:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 22:06:45]

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

[commit-bot: I haz the power, 2016-10-28 22:06:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Charlie Harrison, 2016-10-28 23:23:56]

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

[commit-bot: I haz the power, 2016-10-28 23:24:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 00:20:57]

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

[commit-bot: I haz the power, 2016-10-29 00:20:58]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-10-29 15:48:15]

csharrison@chromium.org changed reviewers:
+ jochen@chromium.org, rdevlin.cronin@chromium.org

[Charlie Harrison, 2016-10-29 15:48:15]

jochen, ptal at //chrome/renderer and //content.

rdevlin, ptal at extensions.

WillSendRequest is a strange method that I'd like to try to get rid of if
possible. Extensions only really needs it for blocking the request (not changing
the URL), so the only real consumer of the API's full power is search box. It
would be nice if Blink could be safe from arbitrary URL re-writing from an
embedder.

[jochen (gone - plz use gerrit), 2016-10-31 10:38:37]

lgtm

[Devlin, 2016-10-31 15:17:40]

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1136: // Early exit before
fetching the RenderView or SearchBox.
optional nit: This comment doesn't seem to add much value (as it just describes
the code).  I'd suggest removing it, but up to you.

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_extensions_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_extensions_renderer_client.cc:215: if
(url.protocolIs(extensions::kExtensionScheme) &&
My url spec knowledge is limited, so I might be completely off base, but I
thought there were actually some cases when scheme and protocol were *not* the
same thing (even though they almost always are).  e.g. with the news scheme, I
thought the protocol was nntp.

Looking into the implementation for protocolIs(), it looks like we even just
compare schemes there, so maybe this is all well and good.  But I wonder if it's
worth a comment explaining why this is safe explicitly for the extension scheme?

https://codereview.chromium.org/2459143002/diff/40001/extensions/shell/render...
File extensions/shell/renderer/shell_content_renderer_client.h (right):

https://codereview.chromium.org/2459143002/diff/40001/extensions/shell/render...
extensions/shell/renderer/shell_content_renderer_client.h:16: }  // namespace
blink
\n after this

[Charlie Harrison, 2016-10-31 15:46:13]

Thanks!

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1136: // Early exit before
fetching the RenderView or SearchBox.
On 2016/10/31 15:17:39, Devlin wrote:
> optional nit: This comment doesn't seem to add much value (as it just
describes
> the code).  I'd suggest removing it, but up to you.

Done.

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_extensions_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_extensions_renderer_client.cc:215: if
(url.protocolIs(extensions::kExtensionScheme) &&
On 2016/10/31 15:17:39, Devlin wrote:
> My url spec knowledge is limited, so I might be completely off base, but I
> thought there were actually some cases when scheme and protocol were *not* the
> same thing (even though they almost always are).  e.g. with the news scheme, I
> thought the protocol was nntp.
> 
> Looking into the implementation for protocolIs(), it looks like we even just
> compare schemes there, so maybe this is all well and good.  But I wonder if
it's
> worth a comment explaining why this is safe explicitly for the extension
scheme?

Hm yeah I think "protocol" here is a misnomer, as we're really accessing the
scheme part of the URL. In that case I would not want to add a comment saying
that the protocol and scheme for extensions are the same, because it could
possibly spread misinformation that they would ever be different in the code.

I can get in touch with KURL owners to see about this discrepancy.

https://codereview.chromium.org/2459143002/diff/40001/extensions/shell/render...
File extensions/shell/renderer/shell_content_renderer_client.h (right):

https://codereview.chromium.org/2459143002/diff/40001/extensions/shell/render...
extensions/shell/renderer/shell_content_renderer_client.h:16: }  // namespace
blink
On 2016/10/31 15:17:39, Devlin wrote:
> \n after this

Done.

[Devlin, 2016-10-31 16:00:58]

lgtm

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_extensions_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_extensions_renderer_client.cc:215: if
(url.protocolIs(extensions::kExtensionScheme) &&
On 2016/10/31 15:46:13, Charlie Harrison wrote:
> On 2016/10/31 15:17:39, Devlin wrote:
> > My url spec knowledge is limited, so I might be completely off base, but I
> > thought there were actually some cases when scheme and protocol were *not*
the
> > same thing (even though they almost always are).  e.g. with the news scheme,
I
> > thought the protocol was nntp.
> > 
> > Looking into the implementation for protocolIs(), it looks like we even just
> > compare schemes there, so maybe this is all well and good.  But I wonder if
> it's
> > worth a comment explaining why this is safe explicitly for the extension
> scheme?
> 
> Hm yeah I think "protocol" here is a misnomer, as we're really accessing the
> scheme part of the URL. In that case I would not want to add a comment saying
> that the protocol and scheme for extensions are the same, because it could
> possibly spread misinformation that they would ever be different in the code.
> 
> I can get in touch with KURL owners to see about this discrepancy.

sgtm; I'm fine with this for now since it *is* safe for extension schemes. 
Lemme know what they say. :)

https://codereview.chromium.org/2459143002/diff/60001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1145: if (gurl.host() ==
chrome::kChromeUIFaviconHost)
Because this is a cl targeting cleaning up unneeded copies...

GURL::host() actually makes a new std::string with each invocation (separately -
I wonder if we can fix that, at least making a base::StringPiece() variant -
there's no reason it *needs* to do the allocation).  Maybe cache std::string
host at line 1144 so we can just use that?

[Charlie Harrison, 2016-10-31 16:36:33]

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

[commit-bot: I haz the power, 2016-10-31 16:36:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-10-31 16:37:34]

Thanks Devlin, landing now.

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_extensions_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_extensions_renderer_client.cc:215: if
(url.protocolIs(extensions::kExtensionScheme) &&
On 2016/10/31 16:00:57, Devlin wrote:
> On 2016/10/31 15:46:13, Charlie Harrison wrote:
> > On 2016/10/31 15:17:39, Devlin wrote:
> > > My url spec knowledge is limited, so I might be completely off base, but I
> > > thought there were actually some cases when scheme and protocol were *not*
> the
> > > same thing (even though they almost always are).  e.g. with the news
scheme,
> I
> > > thought the protocol was nntp.
> > > 
> > > Looking into the implementation for protocolIs(), it looks like we even
just
> > > compare schemes there, so maybe this is all well and good.  But I wonder
if
> > it's
> > > worth a comment explaining why this is safe explicitly for the extension
> > scheme?
> > 
> > Hm yeah I think "protocol" here is a misnomer, as we're really accessing the
> > scheme part of the URL. In that case I would not want to add a comment
saying
> > that the protocol and scheme for extensions are the same, because it could
> > possibly spread misinformation that they would ever be different in the
code.
> > 
> > I can get in touch with KURL owners to see about this discrepancy.
> 
> sgtm; I'm fine with this for now since it *is* safe for extension schemes. 
> Lemme know what they say. :)

I filed crbug.com/660875 asking for owners advice here, and cc'd you.

https://codereview.chromium.org/2459143002/diff/60001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1145: if (gurl.host() ==
chrome::kChromeUIFaviconHost)
On 2016/10/31 16:00:58, Devlin wrote:
> Because this is a cl targeting cleaning up unneeded copies...
> 
> GURL::host() actually makes a new std::string with each invocation (separately
-
> I wonder if we can fix that, at least making a base::StringPiece() variant -
> there's no reason it *needs* to do the allocation).  Maybe cache std::string
> host at line 1144 so we can just use that?

Good catch! We actually have GURL::host_piece which does just what you want.
Possibly we should switch the default to use StringPiece.

[Charlie Harrison, 2016-10-31 16:37:51]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2016-10-31 16:37:52]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-10-31 16:37:52]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2459143002/#ps80001
(title: "s/host/host_piece")

[commit-bot: I haz the power, 2016-10-31 16:38:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-10-31 16:57:42]

https://codereview.chromium.org/2459143002/diff/60001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2459143002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1145: if (gurl.host() ==
chrome::kChromeUIFaviconHost)
On 2016/10/31 16:37:34, Charlie Harrison wrote:
> On 2016/10/31 16:00:58, Devlin wrote:
> > Because this is a cl targeting cleaning up unneeded copies...
> > 
> > GURL::host() actually makes a new std::string with each invocation
(separately
> -
> > I wonder if we can fix that, at least making a base::StringPiece() variant -
> > there's no reason it *needs* to do the allocation).  Maybe cache std::string
> > host at line 1144 so we can just use that?
> 
> Good catch! We actually have GURL::host_piece which does just what you want.
> Possibly we should switch the default to use StringPiece.

...I swear I looked for *just that*.  I was on the verge of filing a bug for
creating GURL::host_piece().  Not sure how I missed it.  Thanks! :)

[commit-bot: I haz the power, 2016-10-31 17:35:04]

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

[commit-bot: I haz the power, 2016-10-31 17:35:05]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2016-10-31 17:39:01]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-10-31 17:39:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 18:07:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-31 18:17:53]

Description was changed from

==========
Avoid GURL allocations/copies/frees in RenderFrameImpl::willSendRequest

ContentRendererClient::WillSendRequest has an API which takes the request
URL as a GURL. This entails an allocation, copy and free when the method
returns. Even for moderate size URLs this work adds up and is visible
in profiles.

This patch threads a blink::WebURL instead of the GURL, as consumers really
just want to know the scheme. This copy-free approach is thread to callers
when possible. Because the method is only used for SearchBox and Extension
URLs, most subresources will take the no-copy path.

Additionally, this patch early-exits before doing some non-trivial work.

BUG=348655
==========

to

==========
Avoid GURL allocations/copies/frees in RenderFrameImpl::willSendRequest

ContentRendererClient::WillSendRequest has an API which takes the request
URL as a GURL. This entails an allocation, copy and free when the method
returns. Even for moderate size URLs this work adds up and is visible
in profiles.

This patch threads a blink::WebURL instead of the GURL, as consumers really
just want to know the scheme. This copy-free approach is thread to callers
when possible. Because the method is only used for SearchBox and Extension
URLs, most subresources will take the no-copy path.

Additionally, this patch early-exits before doing some non-trivial work.

BUG=348655

Committed: https://crrev.com/29d7eaf19413110f27cc101c7e2d7088102184a5
Cr-Commit-Position: refs/heads/master@{#428754}
==========

[commit-bot: I haz the power, 2016-10-31 18:17:54]

Patchset 5 (id:??) landed as
https://crrev.com/29d7eaf19413110f27cc101c7e2d7088102184a5
Cr-Commit-Position: refs/heads/master@{#428754}