Route JS Error Info From Blink to Chrome

This extends the amount of information passed from Blink to Chrome in the case of a runtime JS error.  The extra information is attached to the existing message, and is in the form of a JSON-stringified representation of a ConsoleMessage object, as specified in Source/devtools/protocol.json.

Design Doc: https://docs.google.com/a/chromium.org/document/d/1moGXfxo6BbulkoCWISlKq2aQR_buVYGeGhDj6_bqMXk/edit#heading=h.rh6wl5ot0k8n

[Devlin, 2013-07-24 23:48:22]

Adam, could you take a look at this?

I think this addresses the concerns you raised earlier, in particular this now
uses the same structure as the inspector (allowing for future extension), and
uses the inspector's code in order to generate the JSON, avoiding any
duplication of effort.  Additionally, it consolidates some versions of
addMessageToConsole() (though much of this is only reflected in generated code).

[abarth-chromium, 2013-07-24 23:59:09]

This looks to be on a much better track.  Thank you for reconsidering the
design.  One small note below.

I've also added pfeldman to the review because you're changing the inspector
protocol.

https://codereview.chromium.org/20191003/diff/2001/Source/core/inspector/Cons...
File Source/core/inspector/ConsoleMessage.cpp (right):

https://codereview.chromium.org/20191003/diff/2001/Source/core/inspector/Cons...
Source/core/inspector/ConsoleMessage.cpp:54: , m_executionContextURL()
There's no need to call the default constructor explicitly.  The compiler will
do that for you.

https://codereview.chromium.org/20191003/diff/2001/Source/core/inspector/Insp...
File Source/core/inspector/InspectorConsoleAgent.h (right):

https://codereview.chromium.org/20191003/diff/2001/Source/core/inspector/Insp...
Source/core/inspector/InspectorConsoleAgent.h:78: void
addMessageToConsole(PassOwnPtr<ConsoleMessage>, bool checkFrontend = false);
Rather than adding a raw Boolean parameter, consider adding an enum parameter so
we can more easily tell what callers are doing.

https://codereview.chromium.org/20191003/diff/2001/Source/core/page/PageConso...
File Source/core/page/PageConsole.cpp (right):

https://codereview.chromium.org/20191003/diff/2001/Source/core/page/PageConso...
Source/core/page/PageConsole.cpp:93: : adoptPtr(new
ConsoleMessage(reportDetailedMessage, source, LogMessageType, level, message,
executionContextURL, url, lineNumber, columnNumber, state, requestIdentifier));
Can you extract some of this logic into a helper function?  This is a lot of
detailed work for this function.

https://codereview.chromium.org/20191003/diff/2001/Source/web/ChromeClientImpl.h
File Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/20191003/diff/2001/Source/web/ChromeClientImp...
Source/web/ChromeClientImpl.h:98: virtual bool
shouldReportDetailedMessageForURL(const WTF::String&);
Maybe this should be a WebSetting?  It seems strange that we would need to call
out to the embedder for each message.  Is this going to change on a
message-by-message basis?

[Devlin, 2013-07-25 00:10:45]

https://codereview.chromium.org/20191003/diff/2001/Source/core/inspector/Cons...
File Source/core/inspector/ConsoleMessage.cpp (right):

https://codereview.chromium.org/20191003/diff/2001/Source/core/inspector/Cons...
Source/core/inspector/ConsoleMessage.cpp:54: , m_executionContextURL()
On 2013/07/24 23:59:09, abarth wrote:
> There's no need to call the default constructor explicitly.  The compiler will
> do that for you.

Yeah, I was just wondering if it was Blink style (m_url does this whenever it
isn't set in the constructor). Speaking of, is there any style guide other than
http://www.chromium.org/blink/coding-style#TOC-Classes ? (And also, should I
then remove the m_url() explicit construction, since I'm here?)

https://codereview.chromium.org/20191003/diff/2001/Source/web/ChromeClientImpl.h
File Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/20191003/diff/2001/Source/web/ChromeClientImp...
Source/web/ChromeClientImpl.h:98: virtual bool
shouldReportDetailedMessageForURL(const WTF::String&);
On 2013/07/24 23:59:09, abarth wrote:
> Maybe this should be a WebSetting?  It seems strange that we would need to
call
> out to the embedder for each message.  Is this going to change on a
> message-by-message basis?

This will change on a message-by-message basis, since we don't (currently) have
any reason to keep/report detailed messages from all sources - for example, we
don't care if a website's js throws an error.  Right now, we only want extra
info if an extension caused the error.

That being said, given that errors are (hopefully) infrequent, and the overhead
shouldn't be too significant for generating/passing the details, it may be worth
it to just always pass them (with a WebSetting), and then determine whether or
not to keep them once we have them.  Preferences?

[Devlin, 2013-07-25 01:46:00]

https://codereview.chromium.org/20191003/diff/2001/Source/core/inspector/Insp...
File Source/core/inspector/InspectorConsoleAgent.h (right):

https://codereview.chromium.org/20191003/diff/2001/Source/core/inspector/Insp...
Source/core/inspector/InspectorConsoleAgent.h:78: void
addMessageToConsole(PassOwnPtr<ConsoleMessage>, bool checkFrontend = false);
On 2013/07/24 23:59:09, abarth wrote:
> Rather than adding a raw Boolean parameter, consider adding an enum parameter
so
> we can more easily tell what callers are doing.

Done, but not sure I like the location (InspectorAgent).  Do you think it'd be
better somewhere else?

https://codereview.chromium.org/20191003/diff/2001/Source/core/page/PageConso...
File Source/core/page/PageConsole.cpp (right):

https://codereview.chromium.org/20191003/diff/2001/Source/core/page/PageConso...
Source/core/page/PageConsole.cpp:93: : adoptPtr(new
ConsoleMessage(reportDetailedMessage, source, LogMessageType, level, message,
executionContextURL, url, lineNumber, columnNumber, state, requestIdentifier));
On 2013/07/24 23:59:09, abarth wrote:
> Can you extract some of this logic into a helper function?  This is a lot of
> detailed work for this function.

Done.

[yurys, 2013-07-26 09:17:21]

https://codereview.chromium.org/20191003/diff/10001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorAgent.h (right):

https://codereview.chromium.org/20191003/diff/10001/Source/core/inspector/Ins...
Source/core/inspector/InspectorAgent.h:64: enum CheckFrontendBehavior {
InspectorBaseAgent.h would be a better place for this enum.

https://codereview.chromium.org/20191003/diff/10001/Source/core/page/ChromeCl...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/20191003/diff/10001/Source/core/page/ChromeCl...
Source/core/page/ChromeClient.h:128: virtual bool
shouldReportDetailedMessageForContext(v8::Handle<v8::Context>) = 0;
Are we fine with using v8 types in this API? Or should it be ScriptState*
instead?

https://codereview.chromium.org/20191003/diff/10001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/20191003/diff/10001/Source/devtools/protocol....
Source/devtools/protocol.json:737: { "name": "executionContextURL", "type":
"string", "optional": true, "description": "The context url in which error was
caused." },
We use "Runtime.ExecutionContextId" to reference a context, can we do the same
here?

[pfeldman, 2013-07-26 11:46:26]

not lgtm while we clearing the product side of things in a bug. What are you
doing with that information? Extending the amount of information passed from
Blink to Chrome in the case of a runtime JS error is potentially going to result
in the unbounded memory growth. Extensions are running at all times and the
amount of data you collect is going to be huge. Once again, could we consider
counting errors or collecting their text and linking to the inspector instead.

[Devlin, 2013-07-26 18:32:24]

On 2013/07/26 11:46:26, pfeldman wrote:
> What are you doing with that information?
Our goal is to keep errors which are caused by extensions in order to alert 
developers (right now, it's very unclear if an extension threw an error, because
you have to inspect each page the extension is running on, and sometimes even 
then it's a little ambiguous).  We'll eventually display this information in a 
user-facing context, such as the Extensions page.

> Extending the amount of information passed from Blink to Chrome in the case of
> a runtime JS error is potentially going to result in the unbounded memory 
> growth. Extensions are running at all times and the amount of data you collect
> is going to be huge.

Couple of points here.
- I don't think we're extending it by a huge amount - we're not including any 
  extra details about the state, arguments, etc. All we're adding in is a basic
  stack trace (i.e. function, line, column, source for each frame). So, it is
  more than what we have now, but not ridiculously so.
- Again, we are only collecting this information for extensions, not for every
  JS error that's thrown.
- We plan on only keeping this data if we think the user will be interested in
  the errors (e.g., if the user has Developer Mode enabled in
  chrome:extensions); the default behavior will be to not store any errors.
- We'll probably put in some kind of trimming for errors to ensure they don't
  get too large - grouping similar messages, and keeping only the last n errors,
  for instance.

> Once again, could we consider counting errors or collecting their text and 
> linking to the inspector instead.

Linking to the inspector:
Unfortunately, I don't think this will work well. Right now, the inspector will
only queue up error messages for errors which were caused in a particular render
view.  Once the user navigates away, closes the tab, etc, these errors will be
lost from the inspector.  One of the biggest advantages I see to this system is
that a developer can do an hour of normal browsing, and then check to see if
the extension threw any errors (and, if so, get information about where in the
extension, the page it was thrown on, etc), without needing to have an inspector
(or the extensions page) open during that time.

Additionally, if we have to query the inspector, we lose the advantage of having
"real-time" error reporting, which would allow us to do something like, e.g.,
notify a developer when an extension throws an error, even if he doesn't have
devtools or the extensions page open.

Counting Errors:
As mentioned above, we'll probably have a trim if we get too many, in which case
we could just resort to "This extension has thrown <n> errors." This should be a
fallback, though, because it's not as useful, particularly if the error was
thrown because of a particular site's content.  If an extension only throws
an error on cnn.com, and the developer has visited 100 sites and sees that the
extension threw 2 errors, it's not very helpful.

---------------------

Sorry for the long message, and thanks very much for your time. =)

[Matt Perry, 2013-07-26 18:42:33]

On 2013/07/26 18:32:24, D Cronin wrote:
> On 2013/07/26 11:46:26, pfeldman wrote:
> > What are you doing with that information?
> Our goal is to keep errors which are caused by extensions in order to alert 
> developers (right now, it's very unclear if an extension threw an error,
because
> you have to inspect each page the extension is running on, and sometimes even 
> then it's a little ambiguous).  We'll eventually display this information in a

> user-facing context, such as the Extensions page.
> 
> > Extending the amount of information passed from Blink to Chrome in the case
of
> > a runtime JS error is potentially going to result in the unbounded memory 
> > growth. Extensions are running at all times and the amount of data you
collect
> > is going to be huge.
> 
> Couple of points here.
> - I don't think we're extending it by a huge amount - we're not including any 
>   extra details about the state, arguments, etc. All we're adding in is a
basic
>   stack trace (i.e. function, line, column, source for each frame). So, it is
>   more than what we have now, but not ridiculously so.
> - Again, we are only collecting this information for extensions, not for every
>   JS error that's thrown.
> - We plan on only keeping this data if we think the user will be interested in
>   the errors (e.g., if the user has Developer Mode enabled in
>   chrome:extensions); the default behavior will be to not store any errors.
> - We'll probably put in some kind of trimming for errors to ensure they don't
>   get too large - grouping similar messages, and keeping only the last n
errors,
>   for instance.
> 
> > Once again, could we consider counting errors or collecting their text and 
> > linking to the inspector instead.
> 
> Linking to the inspector:
> Unfortunately, I don't think this will work well. Right now, the inspector
will
> only queue up error messages for errors which were caused in a particular
render
> view.  Once the user navigates away, closes the tab, etc, these errors will be
> lost from the inspector.  One of the biggest advantages I see to this system
is
> that a developer can do an hour of normal browsing, and then check to see if
> the extension threw any errors (and, if so, get information about where in the
> extension, the page it was thrown on, etc), without needing to have an
inspector
> (or the extensions page) open during that time.
> 
> Additionally, if we have to query the inspector, we lose the advantage of
having
> "real-time" error reporting, which would allow us to do something like, e.g.,
> notify a developer when an extension throws an error, even if he doesn't have
> devtools or the extensions page open.
> 
> Counting Errors:
> As mentioned above, we'll probably have a trim if we get too many, in which
case
> we could just resort to "This extension has thrown <n> errors." This should be
a
> fallback, though, because it's not as useful, particularly if the error was
> thrown because of a particular site's content.  If an extension only throws
> an error on http://cnn.com, and the developer has visited 100 sites and sees
that the
> extension threw 2 errors, it's not very helpful.
> 
> ---------------------
> 
> Sorry for the long message, and thanks very much for your time. =)

BTW Pavel, in case you haven't seen it, here's the design doc for this feature:
https://docs.google.com/a/chromium.org/document/d/1moGXfxo6BbulkoCWISlKq2aQR_...

I don't expect a huge amount of errors from extension pages. Content scripts
might be a different story, in which case we can certainly limit the number of
those we keep. But the main goal of this is to do passive collection - so a
developer doesn't have to enable the inspector first in order to see past
errors.

[pfeldman, 2013-07-29 15:24:35]

Looking and commenting on the docs, thanks.

> I don't expect a huge amount of errors from extension pages. Content scripts
> might be a different story, in which case we can certainly limit the number of
> those we keep. But the main goal of this is to do passive collection - so a
> developer doesn't have to enable the inspector first in order to see past
> errors.

You don't need to enable the inspector first to see past errors. You don't get
their stack traces though. That's what we do for regular pages. It is just that
I don't think that we need to provide more information on the extensions than we
do on regular pages in this area.

[Devlin, 2013-07-29 18:08:55]

On 2013/07/29 15:24:35, pfeldman wrote:
> Looking and commenting on the docs, thanks.

Replied in-doc (and added a link in CL description, so we don't have to hunt
through the comments for it).

> You don't need to enable the inspector first to see past errors.

I thought that only errors from the page you are currently on were persisted
in the inspector?  That is, this works:
- I cause an error on google.com
- I open the inspector on google.com
- I see the error message
But this doesn't:
- I cause an error on google.com
- I navigate to myfavoritewebsite.com
- I open the inspector on myfavoritewebsite.com
- I don't see the error

This is the behavior I notice in practice on the latest version of chromium,
and also what I understand from the code - the InspectorConsoleAgent stores the
previous messages, and is created by the InspectorController, which is owned by
the Page, which, if I understand correctly, does not persist across navigations.
Please let me know if this is incorrect (I'm still trying to get up to speed on
WebKit/Blink).

> You don't get their stack traces though. That's what we do for regular pages.
> It is just that I don't think that we need to provide more information on the
> extensions than we do on regular pages in this area.

I've said a bit more on this in a reply in the doc, but to summarize:
- We sometimes use the stack trace to determine the extension which threw the
  error.
- We want developers to know where the extension failed, not just that it did.

Also, bear in mind that we're designing this with some kind of enabling option
in mind (tentatively, we're considering using the "Developer Mode" flag as an
indication).  If users don't have this enabled, we won't bother storing any
errors.  If they do, though, it'd be great to give them as much information as
we can (within reason), so that we can help them improve the extensions, rather
than just letting them know that something, somewhere, sometime went wrong.

Thanks for your help on this :)
- Devlin

[Devlin, 2013-08-01 17:00:20]

On 2013/07/29 18:08:55, D Cronin wrote:
> On 2013/07/29 15:24:35, pfeldman wrote:
> > Looking and commenting on the docs, thanks.
> 
> Replied in-doc (and added a link in CL description, so we don't have to hunt
> through the comments for it).
> 
> > You don't need to enable the inspector first to see past errors.
> 
> I thought that only errors from the page you are currently on were persisted
> in the inspector?  That is, this works:
> - I cause an error on http://google.com
> - I open the inspector on http://google.com
> - I see the error message
> But this doesn't:
> - I cause an error on http://google.com
> - I navigate to http://myfavoritewebsite.com
> - I open the inspector on http://myfavoritewebsite.com
> - I don't see the error
> 
> This is the behavior I notice in practice on the latest version of chromium,
> and also what I understand from the code - the InspectorConsoleAgent stores
the
> previous messages, and is created by the InspectorController, which is owned
by
> the Page, which, if I understand correctly, does not persist across
navigations.
> Please let me know if this is incorrect (I'm still trying to get up to speed
on
> WebKit/Blink).
> 
> > You don't get their stack traces though. That's what we do for regular
pages.
> > It is just that I don't think that we need to provide more information on
the
> > extensions than we do on regular pages in this area.
> 
> I've said a bit more on this in a reply in the doc, but to summarize:
> - We sometimes use the stack trace to determine the extension which threw the
>   error.
> - We want developers to know where the extension failed, not just that it did.
> 
> Also, bear in mind that we're designing this with some kind of enabling option
> in mind (tentatively, we're considering using the "Developer Mode" flag as an
> indication).  If users don't have this enabled, we won't bother storing any
> errors.  If they do, though, it'd be great to give them as much information as
> we can (within reason), so that we can help them improve the extensions,
rather
> than just letting them know that something, somewhere, sometime went wrong.
> 
> Thanks for your help on this :)
> - Devlin

Pavel, still waiting on a reply in the doc, or here.  If possible, I'd like to
try to push this (or something that achieves the same purpose) through soon, or
decide that we definitely need to find a new way to do it.  It's kind of a
roadblock for the ErrorConsole project, so the sooner the better.

Thanks in advance.

Cheers,
- Devlin

[Devlin, 2013-08-06 23:01:25]

On 2013/07/29 15:24:35, pfeldman wrote:
> Looking and commenting on the docs, thanks.
> 
> > I don't expect a huge amount of errors from extension pages. Content scripts
> > might be a different story, in which case we can certainly limit the number
of
> > those we keep. But the main goal of this is to do passive collection - so a
> > developer doesn't have to enable the inspector first in order to see past
> > errors.
> 
> You don't need to enable the inspector first to see past errors. You don't get
> their stack traces though. That's what we do for regular pages. It is just
that
> I don't think that we need to provide more information on the extensions than
we
> do on regular pages in this area.

Ping?  Still need some input on this (and/or the design doc).