Make the externally connectable browser test clobber all of the builtins,

Make the externally connectable browser test clobber all of the builtins,
except for a very small subset of them, and use the safe builtins to make the
test pass.

This will give us a decent sense of confidence that web pages can't
accidentally override builtins in a way that breaks us, though there are still
infinite ways for an extension to deliberately break itself.

BUG=55316
R=jyasskin@chromium.org

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

[not at google - send to devlin, 2013-06-20 00:57:22]

Jeffrey - heads up on this, heading out for the day and there are a few
extension tests this made fail, but yeah, tomorrow...

[not at google - send to devlin, 2013-06-20 20:48:02]

Ok done. PTAL.

[Jeffrey Yasskin, 2013-06-20 23:33:04]

I didn't look at all of the .js changes in detail, and I'd be happier if each
one were driven by a test failure, but this lgtm as a first step.

https://codereview.chromium.org/17451011/diff/10001/chrome/renderer/extension...
File chrome/renderer/extensions/safe_builtins.cc (right):

https://codereview.chromium.org/17451011/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/safe_builtins.cc:104: // info[1]->Object()      //
recv (will throw error not check)
Object() isn't correct anymore, so update this comment.

https://codereview.chromium.org/17451011/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/safe_builtins.cc:113: recv =
v8::StringObject::New(info[1]->ToString())->ToObject();
Why are you creating a new string here instead of just upcasting the argument?

https://codereview.chromium.org/17451011/diff/10001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js
(right):

https://codereview.chromium.org/17451011/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js:23:
function clobber(obj, name, description) {
s/description/qualifiedName/?

https://codereview.chromium.org/17451011/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js:55:
clobber(builtin, name, name);
'name' doesn't seem right for the qualified name here. Can you use
<typename>.name instead? Clearly you're not using this now, but it may save some
confusion in the future.

[not at google - send to devlin, 2013-06-21 00:01:43]

https://codereview.chromium.org/17451011/diff/10001/chrome/renderer/extension...
File chrome/renderer/extensions/safe_builtins.cc (right):

https://codereview.chromium.org/17451011/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/safe_builtins.cc:104: // info[1]->Object()      //
recv (will throw error not check)
On 2013/06/20 23:33:04, Jeffrey Yasskin wrote:
> Object() isn't correct anymore, so update this comment.

Done.

https://codereview.chromium.org/17451011/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/safe_builtins.cc:113: recv =
v8::StringObject::New(info[1]->ToString())->ToObject();
On 2013/06/20 23:33:04, Jeffrey Yasskin wrote:
> Why are you creating a new string here instead of just upcasting the argument?

No new string is being created, ToString() is just a cast, and I don't think
that StringObject and String can be cast (at least, I don't see documented). The
trailing ToObject is necessary because StringObject::New inexplicably returns a
Value not an Object.

https://codereview.chromium.org/17451011/diff/10001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js
(right):

https://codereview.chromium.org/17451011/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js:23:
function clobber(obj, name, description) {
On 2013/06/20 23:33:04, Jeffrey Yasskin wrote:
> s/description/qualifiedName/?

Done.

https://codereview.chromium.org/17451011/diff/10001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js:55:
clobber(builtin, name, name);
On 2013/06/20 23:33:04, Jeffrey Yasskin wrote:
> 'name' doesn't seem right for the qualified name here. Can you use
> <typename>.name instead? Clearly you're not using this now, but it may save
some
> confusion in the future.

Done

[not at google - send to devlin, 2013-06-21 22:26:30]

rebased, ptal

[commit-bot: I haz the power, 2013-06-22 00:34:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/17451011/40001

[not at google - send to devlin, 2013-06-22 02:36:27]

Committed patchset #4 manually as r207995 (presubmit successful).

[commit-bot: I haz the power, 2013-06-22 02:43:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/17451011/40001

[commit-bot: I haz the power, 2013-06-22 02:43:56]

Failed to apply patch for chrome/renderer/extensions/module_system.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/extensions/module_system.cc
  Hunk #1 FAILED at 189.
  Hunk #2 FAILED at 200.
  Hunk #3 FAILED at 483.
  3 out of 3 hunks FAILED -- saving rejects to file
chrome/renderer/extensions/module_system.cc.rej

Patch:       chrome/renderer/extensions/module_system.cc
Index: chrome/renderer/extensions/module_system.cc
diff --git a/chrome/renderer/extensions/module_system.cc
b/chrome/renderer/extensions/module_system.cc
index
29575b2ec1ff20de6ad9245e001477ce528c4937..822c15e54853cdbf2a0034dfb6921a2779dfa969
100644
--- a/chrome/renderer/extensions/module_system.cc
+++ b/chrome/renderer/extensions/module_system.cc
@@ -189,6 +189,8 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
 
   exports = v8::Object::New();
   v8::Handle<v8::Object> natives(NewInstance());
+  CHECK(!natives.IsEmpty());  // this can happen if v8 has issues
+
   // These must match the argument order in WrapSource.
   v8::Handle<v8::Value> args[] = {
     // CommonJS.
@@ -200,6 +202,8 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
     context_->safe_builtins()->GetFunction(),
     context_->safe_builtins()->GetJSON(),
     context_->safe_builtins()->GetObjekt(),
+    context_->safe_builtins()->GetRegExp(),
+    context_->safe_builtins()->GetString(),
   };
   {
     v8::TryCatch try_catch;
@@ -483,7 +487,7 @@ v8::Handle<v8::String>
ModuleSystem::WrapSource(v8::Handle<v8::String> source) {
   // Keep in order with the arguments in RequireForJsInner.
   v8::Handle<v8::String> left = v8::String::New(
       "(function(require, requireNative, exports,"
-                "$Array, $Function, $JSON, $Object) {"
+                "$Array, $Function, $JSON, $Object, $RegExp, $String) {"
        "'use strict';");
   v8::Handle<v8::String> right = v8::String::New("\n})");
   return handle_scope.Close(