Extension Error Piping - Blink: Chrome Side

Pipe detailed information (including stack trace) about runtime JS errors caused by extensions from the WebKit side over to the Chrome side.

This is needed for the Extension Error Console project described here: https://docs.google.com/a/chromium.org/document/d/1moGXfxo6BbulkoCWISlKq2aQR_buVYGeGhDj6_bqMXk/edit?pli=1#heading=h.rh6wl5ot0k8n

Goes with https://codereview.chromium.org/18822004/

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

[Devlin, 2013-07-10 22:24:25]

(2/2)

[Yoyo Zhou, 2013-07-10 23:39:10]

I'm worried about references to extensions in content being a possible layering
violation, even if we don't actually include anything from extensions. It looks
fine otherwise.

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:772: static const char schema_delimeter[] =
"://";
kStandardSchemeSeparator

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:775: (source.find(schema_delimeter) ==
std::string::npos ||
Can you document why these are the cases we care about?

[Matt Perry, 2013-07-10 23:43:13]

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:773: static const char
extension_url_prefix[] = "chrome-extension://";
This is defined in
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

I think Yoyo's right - referencing extensions in content is a layering
violation. You'll need another layer of indirection to call out to the Chrome
layer. chrome_content_renderer_client.h looks like the right place.

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2278: bool
RenderViewImpl::shouldReportDetailedMessage(const WebString& source) {
Why is source a string and not a URL? It would be easier to check the scheme as
a URL.

[Devlin, 2013-07-11 00:55:35]

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:772: static const char schema_delimeter[] =
"://";
On 2013/07/10 23:39:10, Yoyo Zhou wrote:
> kStandardSchemeSeparator

Much less cringe-worthy, but not needed if we go with a GURL. =)

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:773: static const char
extension_url_prefix[] = "chrome-extension://";
On 2013/07/10 23:43:13, Matt Perry wrote:
> This is defined in
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> 
> I think Yoyo's right - referencing extensions in content is a layering
> violation. You'll need another layer of indirection to call out to the Chrome
> layer. chrome_content_renderer_client.h looks like the right place.

I was wondering about that, but saw other references to extensions lying around,
so wasn't quite sure. Done.

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:775: (source.find(schema_delimeter) ==
std::string::npos ||
On 2013/07/10 23:39:10, Yoyo Zhou wrote:
> Can you document why these are the cases we care about?

Done.

https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2278: bool
RenderViewImpl::shouldReportDetailedMessage(const WebString& source) {
On 2013/07/10 23:43:13, Matt Perry wrote:
> Why is source a string and not a URL? It would be easier to check the scheme
as
> a URL.

Well, it comes from WebKit, and is a String in most places there (plus, their
equivalent, a KURL, isn't public).  I kinda thought that initializing a GURL (a
surprisingly expensive process) might be overkill for the checks we need, but it
does make the code more readable and extendable. Changed it to a GURL once it
gets to our side.

https://codereview.chromium.org/18799006/diff/5001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/18799006/diff/5001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:1291: // url at all (as in the
source being "event_bindings").
Are there any other cases in which we know something came from an extension?  Or
in which the "!url.is_valid()" case will include something we don't want?

[Devlin, 2013-07-11 20:28:53]

On 2013/07/11 00:55:35, D Cronin wrote:
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:772: static const char schema_delimeter[]
=
> "://";
> On 2013/07/10 23:39:10, Yoyo Zhou wrote:
> > kStandardSchemeSeparator
> 
> Much less cringe-worthy, but not needed if we go with a GURL. =)
> 
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:773: static const char
> extension_url_prefix[] = "chrome-extension://";
> On 2013/07/10 23:43:13, Matt Perry wrote:
> > This is defined in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> > 
> > I think Yoyo's right - referencing extensions in content is a layering
> > violation. You'll need another layer of indirection to call out to the
Chrome
> > layer. chrome_content_renderer_client.h looks like the right place.
> 
> I was wondering about that, but saw other references to extensions lying
around,
> so wasn't quite sure. Done.
> 
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:775: (source.find(schema_delimeter) ==
> std::string::npos ||
> On 2013/07/10 23:39:10, Yoyo Zhou wrote:
> > Can you document why these are the cases we care about?
> 
> Done.
> 
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:2278: bool
> RenderViewImpl::shouldReportDetailedMessage(const WebString& source) {
> On 2013/07/10 23:43:13, Matt Perry wrote:
> > Why is source a string and not a URL? It would be easier to check the scheme
> as
> > a URL.
> 
> Well, it comes from WebKit, and is a String in most places there (plus, their
> equivalent, a KURL, isn't public).  I kinda thought that initializing a GURL
(a
> surprisingly expensive process) might be overkill for the checks we need, but
it
> does make the code more readable and extendable. Changed it to a GURL once it
> gets to our side.
> 
>
https://codereview.chromium.org/18799006/diff/5001/chrome/renderer/chrome_con...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/18799006/diff/5001/chrome/renderer/chrome_con...
> chrome/renderer/chrome_content_renderer_client.cc:1291: // url at all (as in
the
> source being "event_bindings").
> Are there any other cases in which we know something came from an extension? 
Or
> in which the "!url.is_valid()" case will include something we don't want?

Updated to extend didAddMessageToConsole, rather than create a new method.

[Devlin, 2013-07-11 20:28:54]

On 2013/07/11 00:55:35, D Cronin wrote:
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:772: static const char schema_delimeter[]
=
> "://";
> On 2013/07/10 23:39:10, Yoyo Zhou wrote:
> > kStandardSchemeSeparator
> 
> Much less cringe-worthy, but not needed if we go with a GURL. =)
> 
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:773: static const char
> extension_url_prefix[] = "chrome-extension://";
> On 2013/07/10 23:43:13, Matt Perry wrote:
> > This is defined in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> > 
> > I think Yoyo's right - referencing extensions in content is a layering
> > violation. You'll need another layer of indirection to call out to the
Chrome
> > layer. chrome_content_renderer_client.h looks like the right place.
> 
> I was wondering about that, but saw other references to extensions lying
around,
> so wasn't quite sure. Done.
> 
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:775: (source.find(schema_delimeter) ==
> std::string::npos ||
> On 2013/07/10 23:39:10, Yoyo Zhou wrote:
> > Can you document why these are the cases we care about?
> 
> Done.
> 
>
https://codereview.chromium.org/18799006/diff/1/content/renderer/render_view_...
> content/renderer/render_view_impl.cc:2278: bool
> RenderViewImpl::shouldReportDetailedMessage(const WebString& source) {
> On 2013/07/10 23:43:13, Matt Perry wrote:
> > Why is source a string and not a URL? It would be easier to check the scheme
> as
> > a URL.
> 
> Well, it comes from WebKit, and is a String in most places there (plus, their
> equivalent, a KURL, isn't public).  I kinda thought that initializing a GURL
(a
> surprisingly expensive process) might be overkill for the checks we need, but
it
> does make the code more readable and extendable. Changed it to a GURL once it
> gets to our side.
> 
>
https://codereview.chromium.org/18799006/diff/5001/chrome/renderer/chrome_con...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/18799006/diff/5001/chrome/renderer/chrome_con...
> chrome/renderer/chrome_content_renderer_client.cc:1291: // url at all (as in
the
> source being "event_bindings").
> Are there any other cases in which we know something came from an extension? 
Or
> in which the "!url.is_valid()" case will include something we don't want?

Updated to extend didAddMessageToConsole, rather than create a new method.

[Matt Perry, 2013-07-15 21:35:31]

lgtm

https://codereview.chromium.org/18799006/diff/30001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/18799006/diff/30001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1291: // url at all (as in the
source being "event_bindings").
I'm not sure the latter assertion is true (e.g. maybe there are non-extension
sources that are not URLs). But, this is just a heuristic, so it's OK if we're
overly permissive. Please update the comment to that effect.

[Devlin, 2013-07-16 01:41:28]

https://codereview.chromium.org/18799006/diff/30001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/18799006/diff/30001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1291: // url at all (as in the
source being "event_bindings").
On 2013/07/15 21:35:31, Matt Perry wrote:
> I'm not sure the latter assertion is true (e.g. maybe there are non-extension
> sources that are not URLs). But, this is just a heuristic, so it's OK if we're
> overly permissive. Please update the comment to that effect.

Done.

[pfeldman, 2013-07-17 09:08:37]

https://codereview.chromium.org/18799006/diff/35001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/18799006/diff/35001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1293: return !url.is_empty()
&&
This does not look good. It will make all the //@ sourceURL users end up here.
You probably want only valid extension URLs here. Why filtering by URL at all? I
thought that extensions are only running in their own processes / isolates. You
should instead make a decision based on the v8 context.

[Devlin, 2013-07-17 22:13:54]

https://codereview.chromium.org/18799006/diff/35001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/18799006/diff/35001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1293: return !url.is_empty()
&&
On 2013/07/17 09:08:38, pfeldman wrote:
> This does not look good. It will make all the //@ sourceURL users end up here.
> You probably want only valid extension URLs here. Why filtering by URL at all?
I
> thought that extensions are only running in their own processes / isolates.
You
> should instead make a decision based on the v8 context.

Commented in other CL (https://codereview.chromium.org/18822004/).

[pfeldman, 2013-08-16 08:00:48]

lgtm

https://chromiumcodereview.appspot.com/18799006/diff/63001/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/18799006/diff/63001/content/renderer/r...
content/renderer/render_view_impl.h:21: #include "base/strings/string16.h"
You don't need it anymore?

[Devlin, 2013-08-16 16:06:27]

https://chromiumcodereview.appspot.com/18799006/diff/63001/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/18799006/diff/63001/content/renderer/r...
content/renderer/render_view_impl.h:21: #include "base/strings/string16.h"
On 2013/08/16 08:00:48, pfeldman wrote:
> You don't need it anymore?

I added it originally as part of a separate patch, but left it in because Chrome
has an "include what you use" rule in order to avoid transitive include issues. 
This file uses string16 in many functions already, so it should include the
string16 header, too.

[Devlin, 2013-08-19 20:33:56]

Now that the WebKit side is straightened out...

+jochen@ - chrome/renderer/*, content/browser/renderer_host/*,
content/browser/web_contents/*, content/renderer/*
+joi@ - content/public/renderer/*
+cdn@ - content/common/view_messages.h

Thanks in advance

[Devlin, 2013-08-19 20:34:43]

On 2013/08/19 20:33:56, D Cronin wrote:
> Now that the WebKit side is straightened out...
> 
> +jochen@ - chrome/renderer/*, content/browser/renderer_host/*,
> content/browser/web_contents/*, content/renderer/*
> +joi@ - content/public/renderer/*
> +cdn@ - content/common/view_messages.h
> 
> Thanks in advance

[Jói, 2013-08-20 12:16:48]

https://codereview.chromium.org/18799006/diff/63001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/18799006/diff/63001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:250: const base::string16&
source) const;
Please document the semantics of the |source| parameter.

[Devlin, 2013-08-20 16:09:36]

https://codereview.chromium.org/18799006/diff/63001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/18799006/diff/63001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:250: const base::string16&
source) const;
On 2013/08/20 12:16:49, Jói wrote:
> Please document the semantics of the |source| parameter.

Done.

[Jói, 2013-08-20 17:22:47]

//content/public LGTM.

[Cris Neckar, 2013-08-20 21:23:58]

Have you uploaded a CL which implements
RenderViewHostDelegate::AddMessageToConsole() yet? please add me for a review
when you do.

pending the review of that though IPC changes here LGTM

[Devlin, 2013-08-21 00:19:07]

On 2013/08/20 21:23:58, Cris Neckar wrote:
> Have you uploaded a CL which implements
> RenderViewHostDelegate::AddMessageToConsole() yet? please add me for a review
> when you do.
> 
> pending the review of that though IPC changes here LGTM

Cris - an early version of that CL is here:
https://codereview.chromium.org/23007021/.
This will allow the ErrorConsole to record runtime errors.

Succinctly, this uses RenderViewHostDelegate::AddMessageToConsole() to allow for
a WebContentsObserver::OnConsoleMessageAdded(), which is then used by
extensions::TabHelper and extensions::ExtensionHost to report extension-related
javascript errors to the ErrorConsole.

[jochen (gone - plz use gerrit), 2013-08-21 08:16:15]

lgtm

[commit-bot: I haz the power, 2013-08-21 20:23:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/18799006/8...

[commit-bot: I haz the power, 2013-08-22 00:42:03]

Change committed as 218881

[jam, 2013-08-25 07:12:57]

https://chromiumcodereview.appspot.com/18799006/diff/85014/content/public/ren...
File content/public/renderer/content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/18799006/diff/85014/content/public/ren...
content/public/renderer/content_renderer_client.h:251: // (e.g.,
"chrome-extension://<extension_id>/background.js"), or internal
nit: it's a layering violation to mention concepts from embedders, like
extensions, in content. please remove this mention

[Devlin, 2013-08-26 20:32:40]

https://chromiumcodereview.appspot.com/18799006/diff/85014/content/public/ren...
File content/public/renderer/content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/18799006/diff/85014/content/public/ren...
content/public/renderer/content_renderer_client.h:251: // (e.g.,
"chrome-extension://<extension_id>/background.js"), or internal
On 2013/08/25 07:12:58, jam wrote:
> nit: it's a layering violation to mention concepts from embedders, like
> extensions, in content. please remove this mention

Done in https://codereview.chromium.org/23252006/