Fixed global object leak caused by overwriting the global receiver (the global proxy...
authorverwaest@chromium.org <verwaest@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Wed, 11 Dec 2013 13:51:48 +0000 (13:51 +0000)
committerverwaest@chromium.org <verwaest@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Wed, 11 Dec 2013 13:51:48 +0000 (13:51 +0000)
This CL additionally removes the API function to reattach a global proxy to a
global object.

BUG=324812
LOG=y
R=dcarney@chromium.org, titzer@chromium.org

Review URL: https://chromiumcodereview.appspot.com/101733002

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@18299 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

12 files changed:
include/v8.h
src/api.cc
src/bootstrapper.cc
src/bootstrapper.h
src/objects-inl.h
src/objects.h
src/runtime.cc
src/runtime.h
src/v8natives.js
test/cctest/test-api.cc
test/cctest/test-decls.cc
test/cctest/test-object-observe.cc

index 815f43d..9bf9608 100644 (file)
@@ -5150,20 +5150,16 @@ class V8_EXPORT ExtensionConfiguration {
 class V8_EXPORT Context {
  public:
   /**
-   * Returns the global proxy object or global object itself for
-   * detached contexts.
+   * Returns the global proxy object.
    *
-   * Global proxy object is a thin wrapper whose prototype points to
-   * actual context's global object with the properties like Object, etc.
-   * This is done that way for security reasons (for more details see
+   * Global proxy object is a thin wrapper whose prototype points to actual
+   * context's global object with the properties like Object, etc. This is done
+   * that way for security reasons (for more details see
    * https://wiki.mozilla.org/Gecko:SplitWindow).
    *
    * Please note that changes to global proxy object prototype most probably
-   * would break VM---v8 expects only global object as a prototype of
-   * global proxy object.
-   *
-   * If DetachGlobal() has been invoked, Global() would return actual global
-   * object until global is reattached with ReattachGlobal().
+   * would break VM---v8 expects only global object as a prototype of global
+   * proxy object.
    */
   Local<Object> Global();
 
@@ -5174,18 +5170,6 @@ class V8_EXPORT Context {
   void DetachGlobal();
 
   /**
-   * Reattaches a global object to a context.  This can be used to
-   * restore the connection between a global object and a context
-   * after DetachGlobal has been called.
-   *
-   * \param global_object The global object to reattach to the
-   *   context.  For this to work, the global object must be the global
-   *   object that was associated with this context before a call to
-   *   DetachGlobal.
-   */
-  void ReattachGlobal(Handle<Object> global_object);
-
-  /**
    * Creates a new context and returns a handle to the newly allocated
    * context.
    *
index 290fcba..f82ebe8 100644 (file)
@@ -5409,6 +5409,12 @@ v8::Local<v8::Object> Context::Global() {
   i::Handle<i::Context> context = Utils::OpenHandle(this);
   i::Isolate* isolate = context->GetIsolate();
   i::Handle<i::Object> global(context->global_proxy(), isolate);
+  // TODO(dcarney): This should always return the global proxy
+  // but can't presently as calls to GetProtoype will return the wrong result.
+  if (i::Handle<i::JSGlobalProxy>::cast(
+          global)->IsDetachedFrom(context->global_object())) {
+     global = i::Handle<i::Object>(context->global_object(), isolate);
+  }
   return Utils::ToLocal(i::Handle<i::JSObject>::cast(global));
 }
 
@@ -5421,16 +5427,6 @@ void Context::DetachGlobal() {
 }
 
 
-void Context::ReattachGlobal(Handle<Object> global_object) {
-  i::Handle<i::Context> context = Utils::OpenHandle(this);
-  i::Isolate* isolate = context->GetIsolate();
-  ENTER_V8(isolate);
-  i::Handle<i::JSGlobalProxy> global_proxy =
-      i::Handle<i::JSGlobalProxy>::cast(Utils::OpenHandle(*global_object));
-  isolate->bootstrapper()->ReattachGlobal(context, global_proxy);
-}
-
-
 void Context::AllowCodeGenerationFromStrings(bool allow) {
   i::Handle<i::Context> context = Utils::OpenHandle(this);
   i::Isolate* isolate = context->GetIsolate();
index 1d2fb86..ac3fbb0 100644 (file)
@@ -341,17 +341,6 @@ void Bootstrapper::DetachGlobal(Handle<Context> env) {
   Handle<JSGlobalProxy> global_proxy(JSGlobalProxy::cast(env->global_proxy()));
   global_proxy->set_native_context(*factory->null_value());
   SetObjectPrototype(global_proxy, factory->null_value());
-  env->set_global_proxy(env->global_object());
-  env->global_object()->set_global_receiver(env->global_object());
-}
-
-
-void Bootstrapper::ReattachGlobal(Handle<Context> env,
-                                  Handle<JSGlobalProxy> global_proxy) {
-  env->global_object()->set_global_receiver(*global_proxy);
-  env->set_global_proxy(*global_proxy);
-  SetObjectPrototype(global_proxy, Handle<JSObject>(env->global_object()));
-  global_proxy->set_native_context(*env);
 }
 
 
index bac9f40..4f63c87 100644 (file)
@@ -105,9 +105,6 @@ class Bootstrapper {
   // Detach the environment from its outer global object.
   void DetachGlobal(Handle<Context> env);
 
-  // Reattach an outer global object to an environment.
-  void ReattachGlobal(Handle<Context> env, Handle<JSGlobalProxy> global_proxy);
-
   // Traverses the pointers for memory management.
   void Iterate(ObjectVisitor* v);
 
index 1717a5f..b41a8d3 100644 (file)
@@ -5891,16 +5891,13 @@ PropertyAttributes JSReceiver::GetElementAttribute(uint32_t index) {
 }
 
 
-// TODO(504): this may be useful in other places too where JSGlobalProxy
-// is used.
-Object* JSObject::BypassGlobalProxy() {
-  if (IsJSGlobalProxy()) {
-    Object* proto = GetPrototype();
-    if (proto->IsNull()) return GetHeap()->undefined_value();
-    ASSERT(proto->IsJSGlobalObject());
-    return proto;
-  }
-  return this;
+bool JSGlobalObject::IsDetached() {
+  return JSGlobalProxy::cast(global_receiver())->IsDetachedFrom(this);
+}
+
+
+bool JSGlobalProxy::IsDetachedFrom(GlobalObject* global) {
+  return GetPrototype() != global;
 }
 
 
index cc77e19..7b14574 100644 (file)
@@ -878,6 +878,7 @@ class DictionaryElementsAccessor;
 class ElementsAccessor;
 class Failure;
 class FixedArrayBase;
+class GlobalObject;
 class ObjectVisitor;
 class StringStream;
 class Type;
@@ -7412,6 +7413,8 @@ class JSGlobalProxy : public JSObject {
   // Casting.
   static inline JSGlobalProxy* cast(Object* obj);
 
+  inline bool IsDetachedFrom(GlobalObject* global);
+
   // Dispatched behavior.
   DECLARE_PRINTER(JSGlobalProxy)
   DECLARE_VERIFIER(JSGlobalProxy)
@@ -7481,6 +7484,8 @@ class JSGlobalObject: public GlobalObject {
   static Handle<PropertyCell> EnsurePropertyCell(Handle<JSGlobalObject> global,
                                                  Handle<Name> name);
 
+  inline bool IsDetached();
+
   // Dispatched behavior.
   DECLARE_PRINTER(JSGlobalObject)
   DECLARE_VERIFIER(JSGlobalObject)
index e763d43..8d84fda 100644 (file)
@@ -9624,6 +9624,16 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GlobalReceiver) {
 }
 
 
+RUNTIME_FUNCTION(MaybeObject*, Runtime_IsAttachedGlobal) {
+  SealHandleScope shs(isolate);
+  ASSERT(args.length() == 1);
+  Object* global = args[0];
+  if (!global->IsJSGlobalObject()) return isolate->heap()->false_value();
+  return isolate->heap()->ToBoolean(
+      !JSGlobalObject::cast(global)->IsDetached());
+}
+
+
 RUNTIME_FUNCTION(MaybeObject*, Runtime_ParseJson) {
   HandleScope scope(isolate);
   ASSERT_EQ(1, args.length());
index 95fb419..fe8a26b 100644 (file)
@@ -275,6 +275,7 @@ namespace internal {
   \
   /* Eval */ \
   F(GlobalReceiver, 1, 1) \
+  F(IsAttachedGlobal, 1, 1) \
   F(ResolvePossiblyDirectEval, 5, 2) \
   \
   F(SetProperty, -1 /* 4 or 5 */, 1) \
index b715e89..96b88c5 100644 (file)
@@ -170,19 +170,18 @@ function GlobalParseFloat(string) {
 function GlobalEval(x) {
   if (!IS_STRING(x)) return x;
 
-  var global_receiver = %GlobalReceiver(global);
-  var global_is_detached = (global === global_receiver);
-
   // For consistency with JSC we require the global object passed to
   // eval to be the global object from which 'eval' originated. This
   // is not mandated by the spec.
   // We only throw if the global has been detached, since we need the
   // receiver as this-value for the call.
-  if (global_is_detached) {
+  if (!%IsAttachedGlobal(global)) {
     throw new $EvalError('The "this" value passed to eval must ' +
                          'be the global object from which eval originated');
   }
 
+  var global_receiver = %GlobalReceiver(global);
+
   var f = %CompileString(x, false);
   if (!IS_FUNCTION(f)) return f;
 
index fdd79f1..91f3736 100644 (file)
@@ -8527,8 +8527,6 @@ TEST(ContextDetachGlobal) {
   // Detach env2's global, and reuse the global object of env2
   env2->Exit();
   env2->DetachGlobal();
-  // env2 has a new global object.
-  CHECK(!env2->Global()->Equals(global2));
 
   v8::Handle<Context> env3 = Context::New(env1->GetIsolate(),
                                           0,
@@ -8563,7 +8561,7 @@ TEST(ContextDetachGlobal) {
 }
 
 
-TEST(DetachAndReattachGlobal) {
+TEST(DetachGlobal) {
   LocalContext env1;
   v8::HandleScope scope(env1->GetIsolate());
 
@@ -8628,16 +8626,58 @@ TEST(DetachAndReattachGlobal) {
   // so access should be blocked.
   result = CompileRun("other.p");
   CHECK(result->IsUndefined());
+}
 
-  // Detach the global for env3 and reattach it to env2.
-  env3->DetachGlobal();
-  env2->ReattachGlobal(global2);
 
-  // Check that we have access to other.p again in env1.  |other| is now
-  // the global object for env2 which has the same security token as env1.
-  result = CompileRun("other.p");
-  CHECK(result->IsInt32());
-  CHECK_EQ(42, result->Int32Value());
+TEST(DetachedAccesses) {
+  LocalContext env1;
+  v8::HandleScope scope(env1->GetIsolate());
+
+  // Create second environment.
+  v8::Handle<Context> env2 = Context::New(env1->GetIsolate());
+
+  Local<Value> foo = v8_str("foo");
+
+  // Set same security token for env1 and env2.
+  env1->SetSecurityToken(foo);
+  env2->SetSecurityToken(foo);
+
+  {
+    v8::Context::Scope scope(env2);
+    CompileRun(
+        "var x = 'x';"
+        "function get_x() { return this.x; }"
+        "function get_x_w() { return get_x(); }"
+        "");
+    env1->Global()->Set(v8_str("get_x"), CompileRun("get_x"));
+    env1->Global()->Set(v8_str("get_x_w"), CompileRun("get_x_w"));
+  }
+
+  Local<Object> env2_global = env2->Global();
+  env2_global->TurnOnAccessCheck();
+  env2->DetachGlobal();
+
+  Local<Value> result;
+  result = CompileRun("get_x()");
+  CHECK(result->IsUndefined());
+  result = CompileRun("get_x_w()");
+  CHECK(result->IsUndefined());
+
+  // Reattach env2's proxy
+  env2 = Context::New(env1->GetIsolate(),
+                      0,
+                      v8::Handle<v8::ObjectTemplate>(),
+                      env2_global);
+  env2->SetSecurityToken(foo);
+  {
+    v8::Context::Scope scope(env2);
+    CompileRun("var x = 'x2';");
+  }
+
+  result = CompileRun("get_x()");
+  CHECK(result->IsUndefined());
+  result = CompileRun("get_x_w()");
+  CHECK_EQ(v8_str("x2"), result);
 }
 
 
@@ -14251,8 +14291,10 @@ THREADED_TEST(TurnOnAccessCheck) {
   }
 
   // Detach the global and turn on access check.
+  Local<Object> hidden_global = Local<Object>::Cast(
+      context->Global()->GetPrototype());
   context->DetachGlobal();
-  context->Global()->TurnOnAccessCheck();
+  hidden_global->TurnOnAccessCheck();
 
   // Failing access check to property get results in undefined.
   CHECK(f1->Call(global, 0, NULL)->IsUndefined());
@@ -14336,8 +14378,10 @@ THREADED_TEST(TurnOnAccessCheckAndRecompile) {
 
   // Detach the global and turn on access check now blocking access to property
   // a and function h.
+  Local<Object> hidden_global = Local<Object>::Cast(
+      context->Global()->GetPrototype());
   context->DetachGlobal();
-  context->Global()->TurnOnAccessCheck();
+  hidden_global->TurnOnAccessCheck();
 
   // Failing access check to property get results in undefined.
   CHECK(f1->Call(global, 0, NULL)->IsUndefined());
@@ -14353,11 +14397,11 @@ THREADED_TEST(TurnOnAccessCheckAndRecompile) {
   // Now compile the source again. And get the newly compiled functions, except
   // for h for which access is blocked.
   CompileRun(source);
-  f1 = Local<Function>::Cast(context->Global()->Get(v8_str("f1")));
-  f2 = Local<Function>::Cast(context->Global()->Get(v8_str("f2")));
-  g1 = Local<Function>::Cast(context->Global()->Get(v8_str("g1")));
-  g2 = Local<Function>::Cast(context->Global()->Get(v8_str("g2")));
-  CHECK(context->Global()->Get(v8_str("h"))->IsUndefined());
+  f1 = Local<Function>::Cast(hidden_global->Get(v8_str("f1")));
+  f2 = Local<Function>::Cast(hidden_global->Get(v8_str("f2")));
+  g1 = Local<Function>::Cast(hidden_global->Get(v8_str("g1")));
+  g2 = Local<Function>::Cast(hidden_global->Get(v8_str("g2")));
+  CHECK(hidden_global->Get(v8_str("h"))->IsUndefined());
 
   // Failing access check to property get results in undefined.
   CHECK(f1->Call(global, 0, NULL)->IsUndefined());
@@ -15224,23 +15268,6 @@ THREADED_TEST(ReplaceConstantFunction) {
 }
 
 
-// Regression test for http://crbug.com/16276.
-THREADED_TEST(Regress16276) {
-  LocalContext context;
-  v8::HandleScope scope(context->GetIsolate());
-  // Force the IC in f to be a dictionary load IC.
-  CompileRun("function f(obj) { return obj.x; }\n"
-             "var obj = { x: { foo: 42 }, y: 87 };\n"
-             "var x = obj.x;\n"
-             "delete obj.y;\n"
-             "for (var i = 0; i < 5; i++) f(obj);");
-  // Detach the global object to make 'this' refer directly to the
-  // global object (not the proxy), and make sure that the dictionary
-  // load IC doesn't mess up loading directly from the global object.
-  context->DetachGlobal();
-  CHECK_EQ(42, CompileRun("f(this).foo")->Int32Value());
-}
-
 static void CheckElementValue(i::Isolate* isolate,
                               int expected,
                               i::Handle<i::Object> obj,
index c3bd5c4..de0d745 100644 (file)
@@ -648,9 +648,9 @@ class ExistsInHiddenPrototypeContext: public DeclarationContext {
   virtual void PostInitializeContext(Handle<Context> context) {
     Local<Object> global_object = context->Global();
     Local<Object> hidden_proto = hidden_proto_->GetFunction()->NewInstance();
-    context->DetachGlobal();
-    context->Global()->SetPrototype(hidden_proto);
-    context->ReattachGlobal(global_object);
+    Local<Object> inner_global =
+        Local<Object>::Cast(global_object->GetPrototype());
+    inner_global->SetPrototype(hidden_proto);
   }
 
   // Use the hidden prototype as the holder for the interceptors.
index 4c32d44..b4e7c69 100644 (file)
@@ -242,7 +242,6 @@ TEST(GlobalObjectObservation) {
   LocalContext context(isolate.GetIsolate());
   HandleScope scope(isolate.GetIsolate());
   Handle<Object> global_proxy = context->Global();
-  Handle<Object> inner_global = global_proxy->GetPrototype().As<Object>();
   CompileRun(
       "var records = [];"
       "var global = this;"
@@ -255,33 +254,26 @@ TEST(GlobalObjectObservation) {
   context->DetachGlobal();
   CompileRun("global.bar = 'goodbye';");
   CHECK_EQ(1, CompileRun("records.length")->Int32Value());
-
-  // Mutating the global object directly still has an effect...
-  CompileRun("this.bar = 'goodbye';");
-  CHECK_EQ(2, CompileRun("records.length")->Int32Value());
-  CHECK(inner_global->StrictEquals(CompileRun("records[1].object")));
-
-  // Reattached, back to global proxy.
-  context->ReattachGlobal(global_proxy);
-  CompileRun("global.baz = 'again';");
-  CHECK_EQ(3, CompileRun("records.length")->Int32Value());
-  CHECK(global_proxy->StrictEquals(CompileRun("records[2].object")));
+  CompileRun("this.baz = 'goodbye';");
+  CHECK_EQ(1, CompileRun("records.length")->Int32Value());
 
   // Attached to a different context, should not leak mutations
   // to the old context.
   context->DetachGlobal();
   {
     LocalContext context2(isolate.GetIsolate());
-    context2->DetachGlobal();
-    context2->ReattachGlobal(global_proxy);
     CompileRun(
         "var records2 = [];"
+        "var global = this;"
         "Object.observe(this, function(r) { [].push.apply(records2, r) });"
-        "this.bat = 'context2';");
+        "this.v1 = 'context2';");
+    context2->DetachGlobal();
+    CompileRun(
+        "global.v2 = 'context2';"
+        "this.v3 = 'context2';");
     CHECK_EQ(1, CompileRun("records2.length")->Int32Value());
-    CHECK(global_proxy->StrictEquals(CompileRun("records2[0].object")));
   }
-  CHECK_EQ(3, CompileRun("records.length")->Int32Value());
+  CHECK_EQ(1, CompileRun("records.length")->Int32Value());
 
   // Attaching by passing to Context::New
   {
@@ -295,7 +287,7 @@ TEST(GlobalObjectObservation) {
     CHECK_EQ(1, CompileRun("records3.length")->Int32Value());
     CHECK(global_proxy->StrictEquals(CompileRun("records3[0].object")));
   }
-  CHECK_EQ(3, CompileRun("records.length")->Int32Value());
+  CHECK_EQ(1, CompileRun("records.length")->Int32Value());
 }