Report Javascript Runtime Errors to the Error Console

extensions/browser/extension_error.cc

Index: extensions/browser/extension_error.cc
diff --git a/extensions/browser/extension_error.cc b/extensions/browser/extension_error.cc
index 8b6196c4200967605b3eb51f0653035bd4e88744..4e15ba0af2ef6fec1dc776a5e05ffb11c0a5b2b4 100644
--- a/extensions/browser/extension_error.cc
+++ b/extensions/browser/extension_error.cc
@@ -4,11 +4,13 @@
 
 #include "extensions/browser/extension_error.h"
 
-#include "base/json/json_reader.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "extensions/common/constants.h"
+#include "third_party/re2/re2/re2.h"
 #include "url/gurl.h"
 
 using base::string16;
@@ -23,6 +25,8 @@ const char kURLKey[] = "url";
 const char kFunctionNameKey[] = "functionName";
 const char kExecutionContextURLKey[] = "executionContextURL";
 const char kStackTraceKey[] = "stackTrace";
+const char kStackFrameDelimiter[] = "\n    at ";
+const char kAnonymousFunction[] = "(anonymous function)";
 
 // Try to retrieve an extension ID from a |url|. On success, returns true and
 // populates |extension_id| with the ID. On failure, returns false and leaves
@@ -77,10 +81,6 @@ std::string ManifestParsingError::PrintForTest() const {
          "\n  Type:    ManifestParsingError";
 }
 
-JavascriptRuntimeError::StackFrame::StackFrame() : line_number(-1),
-                                                   column_number(-1) {
-}
-
 JavascriptRuntimeError::StackFrame::StackFrame(size_t frame_line,
                                                size_t frame_column,
                                                const string16& frame_url,
@@ -94,19 +94,54 @@ JavascriptRuntimeError::StackFrame::StackFrame(size_t frame_line,
 JavascriptRuntimeError::StackFrame::~StackFrame() {
 }
 
+// Create a stack frame from the passed text. The text must follow one of two
+// formats:
+//   - "function_name (source:line_number:column_number)"
+//   - "source:line_number:column_number"
+// (We have to recognize two formats because V8 will report stack traces in
+// both ways. If we reconcile this, we can clean this up.

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

nit: )

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

Done.

+scoped_ptr<JavascriptRuntimeError::StackFrame>
+JavascriptRuntimeError::StackFrame::CreateFromText(const string16& utf16_text) {
+  // We need to use utf8 for re2 matching.
+  std::string text = base::UTF16ToUTF8(utf16_text);
+
+  size_t line = -1;
+  size_t column = -1;
+  std::string url;
+  std::string function;
+  if (!re2::RE2::FullMatch(text,
+                           "(.+) \\(([^\\(\\)]+):(\\d+):(\\d+)\\)",
+                           &function, &url, &line, &column) &&
+      !re2::RE2::FullMatch(text,
+                           "([^\\(\\)]+):(\\d+):(\\d+)",
+                           &url, &line, &column)) {
+    return scoped_ptr<StackFrame>();
+  }
+
+  if (function.empty())
+    function = kAnonymousFunction;
+
+  return scoped_ptr<StackFrame>(
+      new StackFrame(
+          line, column, base::UTF8ToUTF16(url), base::UTF8ToUTF16(function)));
+}
+
 JavascriptRuntimeError::JavascriptRuntimeError(bool from_incognito,
                                                const string16& source,
                                                const string16& message,
-                                               logging::LogSeverity level,
-                                               const string16& details)
+                                               const string16& stack_trace,
+                                               int32 line_number,
+                                               const GURL& context_url,
+                                               logging::LogSeverity level)
     : ExtensionError(ExtensionError::JAVASCRIPT_RUNTIME_ERROR,
-                     std::string(),  // We don't know the id yet.
+                     GURL(source).host(),
                      from_incognito,
                      source,
                      message),
+      context_url_(context_url),
       level_(level) {
-  ParseDetails(details);
-  DetermineExtensionID();
+  GetStackTrace(message, stack_trace, line_number);
+  CleanUpInit();
 }
 
 JavascriptRuntimeError::~JavascriptRuntimeError() {
@@ -115,58 +150,78 @@ JavascriptRuntimeError::~JavascriptRuntimeError() {
 std::string JavascriptRuntimeError::PrintForTest() const {
   std::string result = ExtensionError::PrintForTest() +
          "\n  Type:    JavascriptRuntimeError"
-         "\n  Context: " + base::UTF16ToUTF8(execution_context_url_) +
+         "\n  Context: " + context_url_.spec() +
          "\n  Stack Trace: ";
   for (StackTrace::const_iterator iter = stack_trace_.begin();
        iter != stack_trace_.end(); ++iter) {
     result += "\n    {"
-              "\n      Line:     " + base::IntToString(iter->line_number) +
-              "\n      Column:   " + base::IntToString(iter->column_number) +
-              "\n      URL:      " + base::UTF16ToUTF8(iter->url) +
-              "\n      Function: " + base::UTF16ToUTF8(iter->function) +
+              "\n      Line:     " + base::IntToString((*iter)->line_number) +
+              "\n      Column:   " + base::IntToString((*iter)->column_number) +
+              "\n      URL:      " + base::UTF16ToUTF8((*iter)->url) +
+              "\n      Function: " + base::UTF16ToUTF8((*iter)->function) +
               "\n    }";
   }
   return result;
 }
 
-void JavascriptRuntimeError::ParseDetails(const string16& details) {
-  scoped_ptr<base::Value> value(
-      base::JSONReader::Read(base::UTF16ToUTF8(details)));
-  const base::DictionaryValue* details_value;
-  const base::ListValue* trace_value = NULL;
-
-  // The |details| value should contain an execution context url and a stack
-  // trace.
-  if (!value.get() ||
-      !value->GetAsDictionary(&details_value) ||
-      !details_value->GetString(kExecutionContextURLKey,
-                                &execution_context_url_) ||
-      !details_value->GetList(kStackTraceKey, &trace_value)) {
-    NOTREACHED();
-    return;
+void JavascriptRuntimeError::GetStackTrace(const string16& message,
+                                           const string16& stack_trace,
+                                           int32 line_number) {
+  std::vector<string16> pieces;
+  size_t index = 0;
+
+  // If the message includes a stack trace, it's indicative (heuristically) of
+  // a stack trace which we included ourselves in the error message. Since the

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

A pointer to the relevant part of the code would be nice here.

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

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?

+  // thrown error is actually from our own code, the supplied stack trace
+  // is more detailed, and should be used.
+  if (message.find(base::UTF8ToUTF16(kStackFrameDelimiter)) != string16::npos) {
+    base::SplitStringUsingSubstr(message,
+                                 base::UTF8ToUTF16(kStackFrameDelimiter),
+                                 &pieces);
+    message_ = pieces[0];
+    index = 1;
+  } else if (!stack_trace.empty()) {
+    // If we didn't include a stack trace in the message, we should use the
+    // other, if possible.

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

"the other" - can you provide some indication of where that's from?

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

See above.

+    base::SplitStringUsingSubstr(stack_trace,
+                                 base::UTF8ToUTF16(kStackFrameDelimiter),
+                                 &pieces);
   }
 
-  int line = 0;
-  int column = 0;
-  string16 url;
-
-  for (size_t i = 0; i < trace_value->GetSize(); ++i) {
-    const base::DictionaryValue* frame_value = NULL;
-    CHECK(trace_value->GetDictionary(i, &frame_value));
-
-    frame_value->GetInteger(kLineNumberKey, &line);
-    frame_value->GetInteger(kColumnNumberKey, &column);
-    frame_value->GetString(kURLKey, &url);
-
-    string16 function;
-    frame_value->GetString(kFunctionNameKey, &function);  // This can be empty.
-    stack_trace_.push_back(StackFrame(line, column, url, function));
+  // If we got a stack trace, parse each frame from the text.
+  if (index < pieces.size()) {
+    for (; index < pieces.size(); ++index) {
+      scoped_ptr<StackFrame> frame = StackFrame::CreateFromText(pieces[index]);
+      if (frame.get())
+        stack_trace_.push_back(frame.release());
+    }
+  } else {  // Otherwise, mock one up from the given line/url.
+    stack_trace_.push_back(new StackFrame(line_number,
+                                          1,  // column number
+                                          source_,
+                                          UTF8ToUTF16(kAnonymousFunction)));
   }
 }
 
-void JavascriptRuntimeError::DetermineExtensionID() {
-  if (!GetExtensionIDFromGURL(GURL(source_), &extension_id_))
-    GetExtensionIDFromGURL(GURL(execution_context_url_), &extension_id_);
+void JavascriptRuntimeError::CleanUpInit() {
+  // If the error came from a background page, the "context" is empty because

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

from a generated background page?
(What happens when "background": {"page": "a.html"} ?)

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

Ah, yes, generated background page. Whoops.

+  // there's no visible URL. We should set context to be the background page in
+  // this case.
+  GURL source_url = GURL(source_);
+  if (context_url_.is_empty() &&
+      source_url.path() ==
+          std::string("/") + kGeneratedBackgroundPageFilename) {
+    context_url_ = source_url;
+  }
+
+  // In some instances (due to the fact that we're reusing error reporting from
+  // other systems), the source won't match up with the final entry in the stack
+  // trace. (For instance, in a browser action error, the source is the page -
+  // sometimes the background page - but the error is thrown from the script.)
+  // Make the source match the stack trace, since that is more likely the cause
+  // of the error.
+  if (!stack_trace_.empty() && source_ != stack_trace_[0]->url)
+    source_ = stack_trace_[0]->url;
 }
 
 }  // namespace extensions