Unify logic of stack trace generation for extension errors

extensions/renderer/module_system.cc

Index: extensions/renderer/module_system.cc
diff --git a/extensions/renderer/module_system.cc b/extensions/renderer/module_system.cc
index 1f337643476d06ad230eab1fb4949e61ec616a8f..f12ad590802f07958f72fc7c350b80f7befa15a7 100644
--- a/extensions/renderer/module_system.cc
+++ b/extensions/renderer/module_system.cc
@@ -552,7 +552,8 @@ v8::Handle<v8::String> ModuleSystem::WrapSource(v8::Handle<v8::String> source) {
       "(function(define, require, requireNative, requireAsync, exports, "
       "console, privates,"
       "$Array, $Function, $JSON, $Object, $RegExp, $String) {"
-      "'use strict';");
+      "'use strict';"
+      "var Error = window.Error;");

[not at google - send to devlin, 2014/08/18 16:09:44]

This isn't any more desirable than using Error in the code. Is the purpose to
have code safe against the extension tampering with the environment? Probably
wise, but that would be better as $Error and using SafeBuiltins.

[robwu, 2014/08/18 20:59:46]

This protects against redefining the global Error constructor, because
extensions cannot modify the Error variable in the local scope. It is sufficient
because most of the uses of Error are of the form "throw new Error". The only
other use of the error constructor is Error.captureStackTrace, but that method
is also stored in a local variable.

I'm using Error instead of $Error to allow extension code to use "instanceof
Error" and "Error.stackTraceLimit". I've added a comment.

[not at google - send to devlin, 2014/08/18 23:13:10]

Maybe I'm misunderstanding - that doesn't prevent extensions from overriding the
error constructor. They can still do

window.Error = function MySpecialError(message) {
  console.log('oops');
};

and what is stopping extension code from doing instanceof $Error?

[robwu, 2014/08/18 23:27:18]

I assumed that the module system is run before the extension is initialized.
When window.Error is stored in a local variable, the extension cannot mess with
it any more.
An extension cannot easily get a reference to $Error, since it a variable that
is only exposed via the module system. Besides, $Error is non-standard.

[not at google - send to devlin, 2014/08/18 23:39:16]

Right. Unfortunately that's not true, these modules are loaded only when they're
used not ahead of time, so the extension can mess with whatever it wants before
we get a chance. That's why $Function, $Array etc exist - they *are* saved
before the extension has a chance to run.
Yes the idea of $Function and $Array - and $Error - is that it's intentionally
hidden from the extension.

The only functionality you really need is that captureStackTrace (and
stackTraceLimit) right, not the Error constructor? Those are the sorts of things
you'd expose. See safe_builtins.

[robwu, 2014/08/19 14:21:54]

Done.

   v8::Handle<v8::String> right = v8::String::NewFromUtf8(GetIsolate(), "\n})");
   return handle_scope.Escape(v8::Local<v8::String>(
       v8::String::Concat(left, v8::String::Concat(source, right))));