Report Javascript Runtime Errors to the Error Console

Report Javascript Runtime Errors to the Error Console

TBR=brettw@chromium.org
(moving DEPS file from extensions/common/matcher to extensions/common).

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

[Devlin, 2013-08-23 19:47:07]

Yoyo, please take a look when you get a chance.

Also, there's a little overlap between this and
https://codereview.chromium.org/22938005/ for the ErrorConsoleBrowsertest (I'll
clean up whichever one goes in second).

https://codereview.chromium.org/23007021/diff/12001/extensions/common/constan...
File extensions/common/constants.h (right):

https://codereview.chromium.org/23007021/diff/12001/extensions/common/constan...
extensions/common/constants.h:60: bool IsSourceFromAnExtension(const
base::string16& source);
Is there a better place for this?

[Devlin, 2013-08-23 20:06:50]

https://codereview.chromium.org/23007021/diff/12001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23007021/diff/12001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:3430:
FOR_EACH_OBSERVER(WebContentsObserver,
Cris - RenderViewHostImpl receives the IPC message, and then calls
AddMessageToConsole() on its delegate (here, a WebContentsImpl).  So this is
effectively the entry-point for the IPC.

[Cris Neckar, 2013-08-23 20:08:14]

On 2013/08/23 20:06:50, D Cronin wrote:
>
https://codereview.chromium.org/23007021/diff/12001/content/browser/web_conte...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/23007021/diff/12001/content/browser/web_conte...
> content/browser/web_contents/web_contents_impl.cc:3430:
> FOR_EACH_OBSERVER(WebContentsObserver,
> Cris - RenderViewHostImpl receives the IPC message, and then calls
> AddMessageToConsole() on its delegate (here, a WebContentsImpl).  So this is
> effectively the entry-point for the IPC.

Thanks

[Yoyo Zhou, 2013-08-23 22:54:54]

mostly LGTM

https://chromiumcodereview.appspot.com/23007021/diff/12001/chrome/browser/ext...
File chrome/browser/extensions/error_console/error_console_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/23007021/diff/12001/chrome/browser/ext...
chrome/browser/extensions/error_console/error_console_browsertest.cc:43:
string16 (*const U8To16)(const base::StringPiece&) = &base::UTF8ToUTF16;
This is not great for readability.

My suggestions: you can use using base::UTF8ToUTF16; to save a few characters.
Or call UTF8ToUT16 inside the Check functions, and pass std::strings to those.

https://chromiumcodereview.appspot.com/23007021/diff/12001/chrome/browser/ext...
chrome/browser/extensions/error_console/error_console_browsertest.cc:339:
"ReferenceError: baz is not defined"),
nit: indent to (

https://chromiumcodereview.appspot.com/23007021/diff/12001/chrome/browser/ext...
File chrome/browser/extensions/extension_host.cc (right):

https://chromiumcodereview.appspot.com/23007021/diff/12001/chrome/browser/ext...
chrome/browser/extensions/extension_host.cc:434: if
(IsSourceFromAnExtension(source)) {
Do we still want this test here? I'm thinking about event_bindings errors which
would fail IsSourceFromExtension.

https://chromiumcodereview.appspot.com/23007021/diff/12001/extensions/browser...
File extensions/browser/extension_error.cc (right):

https://chromiumcodereview.appspot.com/23007021/diff/12001/extensions/browser...
extensions/browser/extension_error.cc:102: // both ways. If we reconcile this,
we can clean this up.
nit: )

https://chromiumcodereview.appspot.com/23007021/diff/12001/extensions/browser...
extensions/browser/extension_error.cc:174: // a stack trace which we included
ourselves in the error message. Since the
A pointer to the relevant part of the code would be nice here.

https://chromiumcodereview.appspot.com/23007021/diff/12001/extensions/browser...
extensions/browser/extension_error.cc:185: // other, if possible.
"the other" - can you provide some indication of where that's from?

https://chromiumcodereview.appspot.com/23007021/diff/12001/extensions/browser...
extensions/browser/extension_error.cc:207: // If the error came from a
background page, the "context" is empty because
from a generated background page?
(What happens when "background": {"page": "a.html"} ?)

https://chromiumcodereview.appspot.com/23007021/diff/12001/extensions/common/...
File extensions/common/constants.h (right):

https://chromiumcodereview.appspot.com/23007021/diff/12001/extensions/common/...
extensions/common/constants.h:60: bool IsSourceFromAnExtension(const
base::string16& source);
On 2013/08/23 19:47:07, D Cronin wrote:
> Is there a better place for this?

I'd start a new file, extension_urls.h.

[Devlin, 2013-08-23 23:44:30]

https://codereview.chromium.org/23007021/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/error_console/error_console_browsertest.cc
(right):

https://codereview.chromium.org/23007021/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/error_console/error_console_browsertest.cc:43:
string16 (*const U8To16)(const base::StringPiece&) = &base::UTF8ToUTF16;
On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> This is not great for readability.
> 
> My suggestions: you can use using base::UTF8ToUTF16; to save a few characters.
> Or call UTF8ToUT16 inside the Check functions, and pass std::strings to those.

Done.

https://codereview.chromium.org/23007021/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/error_console/error_console_browsertest.cc:339:
"ReferenceError: baz is not defined"),
On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> nit: indent to (

Done.

https://codereview.chromium.org/23007021/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/extension_host.cc (right):

https://codereview.chromium.org/23007021/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/extension_host.cc:434: if
(IsSourceFromAnExtension(source)) {
On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> Do we still want this test here? I'm thinking about event_bindings errors
which
> would fail IsSourceFromExtension.

While that's true, the bigger problem is that they'd also fail
ChromeContentRendererClient::ShouldReportDetailedMessageForSource(), since it
uses the same test.  We also don't want to report all errors (regardless of if
they're from an extension or not) to the error console.

I think it's okay for now, and I've already started work on another CL which
expands the test to allow other specific sources (like event_bindings,
schemaUtil, etc).

https://codereview.chromium.org/23007021/diff/12001/extensions/browser/extens...
File extensions/browser/extension_error.cc (right):

https://codereview.chromium.org/23007021/diff/12001/extensions/browser/extens...
extensions/browser/extension_error.cc:102: // both ways. If we reconcile this,
we can clean this up.
On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> nit: )

Done.

https://codereview.chromium.org/23007021/diff/12001/extensions/browser/extens...
extensions/browser/extension_error.cc:174: // a stack trace which we included
ourselves in the error message. Since the
On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> A pointer to the relevant part of the code would be nice here.

I think any comment-point would be flimsy and could be quickly outdated.  But I
beefed up the comment to make it more clear.  Does this work?

https://codereview.chromium.org/23007021/diff/12001/extensions/browser/extens...
extensions/browser/extension_error.cc:185: // other, if possible.
On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> "the other" - can you provide some indication of where that's from?

See above.

https://codereview.chromium.org/23007021/diff/12001/extensions/browser/extens...
extensions/browser/extension_error.cc:207: // If the error came from a
background page, the "context" is empty because
On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> from a generated background page?
> (What happens when "background": {"page": "a.html"} ?)

Ah, yes, generated background page. Whoops.

https://codereview.chromium.org/23007021/diff/12001/extensions/common/constan...
File extensions/common/constants.h (right):

https://codereview.chromium.org/23007021/diff/12001/extensions/common/constan...
extensions/common/constants.h:60: bool IsSourceFromAnExtension(const
base::string16& source);
On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> On 2013/08/23 19:47:07, D Cronin wrote:
> > Is there a better place for this?
> 
> I'd start a new file, extension_urls.h.

Done. I'll try to move some stuff that makes sense (like kExtensionScheme, and
maybe kGeneratedBackgroundPageFilename) over, too, in a smaller CL.

[Yoyo Zhou, 2013-08-23 23:55:04]

LGTM

https://codereview.chromium.org/23007021/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/extension_host.cc (right):

https://codereview.chromium.org/23007021/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/extension_host.cc:434: if
(IsSourceFromAnExtension(source)) {
On 2013/08/23 23:44:30, D Cronin wrote:
> On 2013/08/23 22:54:54, Yoyo Zhou wrote:
> > Do we still want this test here? I'm thinking about event_bindings errors
> which
> > would fail IsSourceFromExtension.
> 
> While that's true, the bigger problem is that they'd also fail
> ChromeContentRendererClient::ShouldReportDetailedMessageForSource(), since it
> uses the same test.  We also don't want to report all errors (regardless of if
> they're from an extension or not) to the error console.
> 
> I think it's okay for now, and I've already started work on another CL which
> expands the test to allow other specific sources (like event_bindings,
> schemaUtil, etc).

Ok. I was thinking we'd only get extension errors from an ExtensionHost, but
that might not necessarily be true.

https://codereview.chromium.org/23007021/diff/22001/extensions/browser/extens...
File extensions/browser/extension_error.cc (right):

https://codereview.chromium.org/23007021/diff/22001/extensions/browser/extens...
extensions/browser/extension_error.cc:181: // our own code up until the error
catch).
This is so long it's like an essay. Can you make it more concise? Here are some
suggestions:

"There are two possible scenarios:
1. The stack trace is in |stack_trace|, ...
2. The error |message| includes a stack trace string.... In this case,
|stack_trace| is less helpful because it comes from our internal bindings rather
than the extension's code.
"

When you talk about "our" code it's not immediately clear that "our" = the
extension system's, and not, say, "our" = the error console's.

[Devlin, 2013-08-24 00:00:32]

https://codereview.chromium.org/23007021/diff/22001/extensions/browser/extens...
File extensions/browser/extension_error.cc (right):

https://codereview.chromium.org/23007021/diff/22001/extensions/browser/extens...
extensions/browser/extension_error.cc:181: // our own code up until the error
catch).
On 2013/08/23 23:55:04, Yoyo Zhou wrote:
> This is so long it's like an essay. Can you make it more concise? Here are
some
> suggestions:
> 
> "There are two possible scenarios:
> 1. The stack trace is in |stack_trace|, ...
> 2. The error |message| includes a stack trace string.... In this case,
> |stack_trace| is less helpful because it comes from our internal bindings
rather
> than the extension's code.
> "
> 
> When you talk about "our" code it's not immediately clear that "our" = the
> extension system's, and not, say, "our" = the error console's.

Done.

[Devlin, 2013-08-24 00:03:01]

+sky@ - chrome/renderer/chrome_content_renderer_client.cc,
content/browser/web_contents/web_contents_impl.cc
+joi@ - content/public/browser/web_contents_observer.h

[Yoyo Zhou, 2013-08-24 00:03:17]

https://codereview.chromium.org/23007021/diff/22001/extensions/browser/extens...
File extensions/browser/extension_error.cc (right):

https://codereview.chromium.org/23007021/diff/22001/extensions/browser/extens...
extensions/browser/extension_error.cc:181: // our own code up until the error
catch).
On 2013/08/24 00:00:32, D Cronin wrote:
> On 2013/08/23 23:55:04, Yoyo Zhou wrote:
> > This is so long it's like an essay. Can you make it more concise? Here are
> some
> > suggestions:
> > 
> > "There are two possible scenarios:
> > 1. The stack trace is in |stack_trace|, ...
> > 2. The error |message| includes a stack trace string.... In this case,
> > |stack_trace| is less helpful because it comes from our internal bindings
> rather
> > than the extension's code.
> > "
> > 
> > When you talk about "our" code it's not immediately clear that "our" = the
> > extension system's, and not, say, "our" = the error console's.
> 
> Done.

Nice.

[Jói, 2013-08-25 16:36:43]

https://codereview.chromium.org/23007021/diff/41001/content/public/browser/we...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/23007021/diff/41001/content/public/browser/we...
content/public/browser/web_contents_observer.h:271: virtual void
OnMessageAddedToConsole(const base::string16& source,
Perhaps the semantics of |source| and |stack_trace| should be documented here?

[Devlin, 2013-08-26 16:32:34]

https://codereview.chromium.org/23007021/diff/41001/content/public/browser/we...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/23007021/diff/41001/content/public/browser/we...
content/public/browser/web_contents_observer.h:271: virtual void
OnMessageAddedToConsole(const base::string16& source,
On 2013/08/25 16:36:43, Jói wrote:
> Perhaps the semantics of |source| and |stack_trace| should be documented here?

Done.

[Jói, 2013-08-26 18:26:52]

LGTM, thanks.

[Cris Neckar, 2013-08-27 19:10:46]

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
File extensions/browser/extension_error.cc (right):

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
extensions/browser/extension_error.cc:112: if (!re2::RE2::FullMatch(text,
I don't much like that we are exposing the entire attack surface of re2 directly
to attack here. Why aren't you simply passing the StackFrame object across IPC?

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
extensions/browser/extension_error.cc:199: stack_trace_.push_back(new
StackFrame(line_number,
ditto for this vector.. Having this complexity on the browser side seems wrong.

[Devlin, 2013-08-27 19:52:47]

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
File extensions/browser/extension_error.cc (right):

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
extensions/browser/extension_error.cc:112: if (!re2::RE2::FullMatch(text,
On 2013/08/27 19:10:47, Cris Neckar wrote:
> I don't much like that we are exposing the entire attack surface of re2
directly
> to attack here. Why aren't you simply passing the StackFrame object across
IPC?

So, to confirm, you would prefer to do this work on the renderer side, and
introduce a content/public/common/stack_frame object? (If so, I can certainly do
that - just wanna make sure I understand fully).

[Cris Neckar, 2013-08-27 19:54:57]

On 2013/08/27 19:52:47, D Cronin wrote:
>
https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
> File extensions/browser/extension_error.cc (right):
> 
>
https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
> extensions/browser/extension_error.cc:112: if (!re2::RE2::FullMatch(text,
> On 2013/08/27 19:10:47, Cris Neckar wrote:
> > I don't much like that we are exposing the entire attack surface of re2
> directly
> > to attack here. Why aren't you simply passing the StackFrame object across
> IPC?
> 
> So, to confirm, you would prefer to do this work on the renderer side, and
> introduce a content/public/common/stack_frame object? (If so, I can certainly
do
> that - just wanna make sure I understand fully).

Yes that would be my preference. Thanks!

[Devlin, 2013-08-27 23:51:33]

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
File extensions/browser/extension_error.cc (right):

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
extensions/browser/extension_error.cc:112: if (!re2::RE2::FullMatch(text,
On 2013/08/27 19:10:47, Cris Neckar wrote:
> I don't much like that we are exposing the entire attack surface of re2
directly
> to attack here. Why aren't you simply passing the StackFrame object across
IPC?

Done.

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
extensions/browser/extension_error.cc:199: stack_trace_.push_back(new
StackFrame(line_number,
On 2013/08/27 19:10:47, Cris Neckar wrote:
> ditto for this vector.. Having this complexity on the browser side seems
wrong.

Done.

https://codereview.chromium.org/23007021/diff/58001/content/common/stack_fram...
File content/common/stack_frame_unittest.cc (right):

https://codereview.chromium.org/23007021/diff/58001/content/common/stack_fram...
content/common/stack_frame_unittest.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
I didn't see any unittests in content/public/ - where's the right place for this
file?

[Jói, 2013-08-28 07:37:48]

>
https://codereview.chromium.org/23007021/diff/58001/content/common/stack_fram...
> content/common/stack_frame_unittest.cc:1: // Copyright 2013 The Chromium
> Authors. All rights reserved.
> I didn't see any unittests in content/public/ - where's the right place for
this
> file?

content/common is fine for unittests, there are several there already.  For
tests that test code in content/public/x, they are generally in content/x (e.g.
tests for content/public/browser are generally in content/browser), alongside
the implementation of the API classes.

Cheers,
Jói

[Cris Neckar, 2013-08-28 18:02:56]

On 2013/08/28 07:37:48, Jói wrote:
> >
>
https://codereview.chromium.org/23007021/diff/58001/content/common/stack_fram...
> > content/common/stack_frame_unittest.cc:1: // Copyright 2013 The Chromium
> > Authors. All rights reserved.
> > I didn't see any unittests in content/public/ - where's the right place for
> this
> > file?
> 
> content/common is fine for unittests, there are several there already.  For
> tests that test code in content/public/x, they are generally in content/x
(e.g.
> tests for content/public/browser are generally in content/browser), alongside
> the implementation of the API classes.
> 
> Cheers,
> Jói


Thanks for reworking this

IPC changes LGTM

[Devlin, 2013-08-28 18:32:44]

+jam, since we're now adding a (small) new class to content/public/.

[jam, 2013-08-28 19:34:04]

On 2013/08/28 18:32:44, D Cronin wrote:
> +jam, since we're now adding a (small) new class to content/public/.

In general, we only add code to content for chrome's sake if there's no other
way of doing things.

In this case, it's not clear to me why this new StackFrame class is in content.
It seems that chrome code can generate this from the string itself. If content
needs to pass the result of shouldReportDetailedMessageForSource, it can just
add a boolean to the IPC and delegate method, and chrome can use that to do the
parsing.

[jam, 2013-08-28 19:43:33]

On 2013/08/28 19:34:04, jam wrote:
> On 2013/08/28 18:32:44, D Cronin wrote:
> > +jam, since we're now adding a (small) new class to content/public/.
> 
> In general, we only add code to content for chrome's sake if there's no other
> way of doing things.
> 
> In this case, it's not clear to me why this new StackFrame class is in
content.
> It seems that chrome code can generate this from the string itself. If content
> needs to pass the result of shouldReportDetailedMessageForSource, it can just
> add a boolean to the IPC and delegate method, and chrome can use that to do
the
> parsing.

btw just saw the comment about why you changed it (because you don't want to
call re2 in the browser). Even if that's the case, this doesn't mean that code
moves from chrome to content. i.e. we can add new a RenderViewObserver method to
call out to chrome/renderer

[Cris Neckar, 2013-08-28 19:53:35]

On 2013/08/28 19:43:33, jam wrote:
> On 2013/08/28 19:34:04, jam wrote:
> > On 2013/08/28 18:32:44, D Cronin wrote:
> > > +jam, since we're now adding a (small) new class to content/public/.
> > 
> > In general, we only add code to content for chrome's sake if there's no
other
> > way of doing things.
> > 
> > In this case, it's not clear to me why this new StackFrame class is in
> content.
> > It seems that chrome code can generate this from the string itself. If
content
> > needs to pass the result of shouldReportDetailedMessageForSource, it can
just
> > add a boolean to the IPC and delegate method, and chrome can use that to do
> the
> > parsing.
> 
> btw just saw the comment about why you changed it (because you don't want to
> call re2 in the browser). Even if that's the case, this doesn't mean that code
> moves from chrome to content. i.e. we can add new a RenderViewObserver method
to
> call out to chrome/renderer

jam: yeah, the goal was to keep the complexity on the renderer side but as to
the specifics of how that should be implemented I defer to you.

[Devlin, 2013-08-28 20:18:48]

On 2013/08/28 19:53:35, Cris Neckar wrote:
> On 2013/08/28 19:43:33, jam wrote:
> > On 2013/08/28 19:34:04, jam wrote:
> > > On 2013/08/28 18:32:44, D Cronin wrote:
> > > > +jam, since we're now adding a (small) new class to content/public/.
> > > 
> > > In general, we only add code to content for chrome's sake if there's no
> other
> > > way of doing things.
> > > 
> > > In this case, it's not clear to me why this new StackFrame class is in
> > content.
> > > It seems that chrome code can generate this from the string itself. If
> content
> > > needs to pass the result of shouldReportDetailedMessageForSource, it can
> just
> > > add a boolean to the IPC and delegate method, and chrome can use that to
do
> > the
> > > parsing.
> > 
> > btw just saw the comment about why you changed it (because you don't want to
> > call re2 in the browser). Even if that's the case, this doesn't mean that
code
> > moves from chrome to content. i.e. we can add new a RenderViewObserver
method
> to
> > call out to chrome/renderer
> 
> jam: yeah, the goal was to keep the complexity on the renderer side but as to
> the specifics of how that should be implemented I defer to you.

jam, cdn, yoz - Please lemme know if the following sounds good.
- Move stack_frame.h to either extensions/common/stack_frame.h or
chrome/common/[?]/stack_frame.h [1]
- in content/r/render_view_impl:
void RenderViewImpl::didAddMessageToConsole(...) {
  ...
  GetContentClient()->renderer()->OnMessageAddedToConsole(...);
  ...
}
- in chrome/r/chrome_content_renderer_client:
void ChromeContentRendererClient::OnMessageAddedToConsole(msg, line,
stack_trace...) {
  ParseStacktrace();
  render_view->Send(ChromeViewMsg_ConsoleMessageAdded(msg ,line,
stack_trace...));
}
- This message gets passed through RenderView to WebContentsImpl, which passes
it to all the observers.
- ExtensionHost and TabHelper are listening for the Message, and handle it.

This leaves complexity of parsing the stack trace on the renderer side, as well
as in Chrome code, and works with minimal additions to all components.
Does this work for everyone? =)

[1] - Right now, extensions code is the only listener for this, but it doesn't
have to be exclusive to extensions.  Would we prefer to keep it all in
extensions for the time being, or move it somewhere more accessible from the
get-go?

Cheers,
- Devlin

[jam, 2013-08-29 05:23:59]

On 2013/08/28 20:18:48, D Cronin wrote:
> On 2013/08/28 19:53:35, Cris Neckar wrote:
> > On 2013/08/28 19:43:33, jam wrote:
> > > On 2013/08/28 19:34:04, jam wrote:
> > > > On 2013/08/28 18:32:44, D Cronin wrote:
> > > > > +jam, since we're now adding a (small) new class to content/public/.
> > > > 
> > > > In general, we only add code to content for chrome's sake if there's no
> > other
> > > > way of doing things.
> > > > 
> > > > In this case, it's not clear to me why this new StackFrame class is in
> > > content.
> > > > It seems that chrome code can generate this from the string itself. If
> > content
> > > > needs to pass the result of shouldReportDetailedMessageForSource, it can
> > just
> > > > add a boolean to the IPC and delegate method, and chrome can use that to
> do
> > > the
> > > > parsing.
> > > 
> > > btw just saw the comment about why you changed it (because you don't want
to
> > > call re2 in the browser). Even if that's the case, this doesn't mean that
> code
> > > moves from chrome to content. i.e. we can add new a RenderViewObserver
> method
> > to
> > > call out to chrome/renderer
> > 
> > jam: yeah, the goal was to keep the complexity on the renderer side but as
to
> > the specifics of how that should be implemented I defer to you.
> 
> jam, cdn, yoz - Please lemme know if the following sounds good.
> - Move stack_frame.h to either extensions/common/stack_frame.h or
> chrome/common/[?]/stack_frame.h [1]
> - in content/r/render_view_impl:
> void RenderViewImpl::didAddMessageToConsole(...) {
>   ...
>   GetContentClient()->renderer()->OnMessageAddedToConsole(...);

I had suggested RenderViewObserver  because in this case we have context
(RenderView). We only fall back to ContentRendererClient when there isn't a more
specific interface to call to the embedder on.

>   ...
> }
> - in chrome/r/chrome_content_renderer_client:
> void ChromeContentRendererClient::OnMessageAddedToConsole(msg, line,
> stack_trace...) {
>   ParseStacktrace();
>   render_view->Send(ChromeViewMsg_ConsoleMessageAdded(msg ,line,
> stack_trace...));
> }
> - This message gets passed through RenderView to WebContentsImpl, which passes
> it to all the observers.
> - ExtensionHost and TabHelper are listening for the Message, and handle it.
> 
> This leaves complexity of parsing the stack trace on the renderer side, as
well
> as in Chrome code, and works with minimal additions to all components.
> Does this work for everyone? =)
> 
> [1] - Right now, extensions code is the only listener for this, but it doesn't
> have to be exclusive to extensions.  Would we prefer to keep it all in
> extensions for the time being, or move it somewhere more accessible from the
> get-go?

if only extensions is using it, put it in src/extensions.

> 
> Cheers,
> - Devlin

[Yoyo Zhou, 2013-08-29 14:08:09]

On 2013/08/29 05:23:59, jam wrote:
> On 2013/08/28 20:18:48, D Cronin wrote:

> > [1] - Right now, extensions code is the only listener for this, but it
doesn't
> > have to be exclusive to extensions.  Would we prefer to keep it all in
> > extensions for the time being, or move it somewhere more accessible from the
> > get-go?
> 
> if only extensions is using it, put it in src/extensions.

Yes. Only once someone outside of extensions wants to use it, they can move it
to a commoner location.

[jam, 2013-08-29 16:10:31]

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
File extensions/browser/extension_error.cc (right):

https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
extensions/browser/extension_error.cc:112: if (!re2::RE2::FullMatch(text,
On 2013/08/27 19:10:47, Cris Neckar wrote:
> I don't much like that we are exposing the entire attack surface of re2
directly
> to attack here. Why aren't you simply passing the StackFrame object across
IPC?

I'm not familiar with re2, but I hear that Justin is.

Justin: can we get a second opinion on how much of a threat this is? i.e. to
avoid the extra complexity of doing this in the renderer.

[jschuh, 2013-08-29 17:22:36]

On 2013/08/29 16:10:31, jam wrote:
>
https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
> File extensions/browser/extension_error.cc (right):
> 
>
https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
> extensions/browser/extension_error.cc:112: if (!re2::RE2::FullMatch(text,
> On 2013/08/27 19:10:47, Cris Neckar wrote:
> > I don't much like that we are exposing the entire attack surface of re2
> directly
> > to attack here. Why aren't you simply passing the StackFrame object across
> IPC?
> 
> I'm not familiar with re2, but I hear that Justin is.
> 
> Justin: can we get a second opinion on how much of a threat this is? i.e. to
> avoid the extra complexity of doing this in the renderer.

Took a closer look. We're willing to expose re2 on the browser side, but that's
really not the issue here from a security perspective. The bigger thing, as @cdn
noted, is that we don't want opaque blobs thrown over to the browser, except
when unavoidable. It's better to keep the complexity inside the renderer so it's
easier to audit and less browser attack surface gets exposed.

[Devlin, 2013-08-29 19:09:11]

On 2013/08/29 17:22:36, Justin Schuh wrote:
> On 2013/08/29 16:10:31, jam wrote:
> >
>
https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
> > File extensions/browser/extension_error.cc (right):
> > 
> >
>
https://codereview.chromium.org/23007021/diff/48001/extensions/browser/extens...
> > extensions/browser/extension_error.cc:112: if (!re2::RE2::FullMatch(text,
> > On 2013/08/27 19:10:47, Cris Neckar wrote:
> > > I don't much like that we are exposing the entire attack surface of re2
> > directly
> > > to attack here. Why aren't you simply passing the StackFrame object across
> > IPC?
> > 
> > I'm not familiar with re2, but I hear that Justin is.
> > 
> > Justin: can we get a second opinion on how much of a threat this is? i.e. to
> > avoid the extra complexity of doing this in the renderer.
> 
> Took a closer look. We're willing to expose re2 on the browser side, but
that's
> really not the issue here from a security perspective. The bigger thing, as
@cdn
> noted, is that we don't want opaque blobs thrown over to the browser, except
> when unavoidable. It's better to keep the complexity inside the renderer so
it's
> easier to audit and less browser attack surface gets exposed.

Please take a look.

- Method added in RenderViewObserver, overridden in ChromeRenderViewObserver to
keep all knowledge of StackFrame out of content.
- Stack trace is parsed from the string in ChromeRVO to keep complexity on
renderer side, and eliminate browser exposure of re2.
- Msg sent from ChromeRVO to listeners (here,
ExtensionHost/extensions::TabHelper).  Eliminates need for stack_trace param in
ViewHostMsg_AddMessageToConsole.

[jam, 2013-08-30 16:04:38]

https://codereview.chromium.org/23007021/diff/82001/content/public/renderer/r...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/23007021/diff/82001/content/public/renderer/r...
content/public/renderer/render_view_observer.h:93: virtual void
OnDetailedConsoleMessageAdded(const base::string16& message,
this doesn't match this block (i.e. it doesn't match a RenderView method)...

separate this out, add a comment, and take out the "On" prefix as by convention
that's usually for an ipc dispatcher. why do we need "detailed" as well, i.e.
isn't ConsoleMessageAdded sufficient?

https://codereview.chromium.org/23007021/diff/82001/extensions/common/stack_f...
File extensions/common/stack_frame.h (right):

https://codereview.chromium.org/23007021/diff/82001/extensions/common/stack_f...
extensions/common/stack_frame.h:15: /*
?

[Devlin, 2013-08-30 16:33:37]

https://codereview.chromium.org/23007021/diff/82001/content/public/renderer/r...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/23007021/diff/82001/content/public/renderer/r...
content/public/renderer/render_view_observer.h:93: virtual void
OnDetailedConsoleMessageAdded(const base::string16& message,
On 2013/08/30 16:04:39, jam wrote:
> this doesn't match this block (i.e. it doesn't match a RenderView method)...
> 
> separate this out, add a comment, and take out the "On" prefix as by
convention
> that's usually for an ipc dispatcher. why do we need "detailed" as well, i.e.
> isn't ConsoleMessageAdded sufficient?

Separated, commented (with semantics for stack trace, per Joi's previous
request), s/On//.  'Detailed' was to clarify that this isn't called in all cases
when we receive a console message - only if we receive one for which we
requested extra detail (because otherwise, we take no special action).  If you
like, though, I can simplify it to ConsoleMessageAdded(). :)

https://codereview.chromium.org/23007021/diff/82001/extensions/common/stack_f...
File extensions/common/stack_frame.h (right):

https://codereview.chromium.org/23007021/diff/82001/extensions/common/stack_f...
extensions/common/stack_frame.h:15: /*
On 2013/08/30 16:04:39, jam wrote:
> ?

Dang, sorry.

[sky, 2013-08-30 19:47:51]

LGTM

[jam, 2013-08-30 20:48:29]

lgtm

[commit-bot: I haz the power, 2013-08-30 23:15:44]

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

[commit-bot: I haz the power, 2013-09-01 23:05:14]

Change committed as 220753