Don't leak contexts in Object.observe

src/runtime.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stdlib.h>
 #include <limits>
 
 #include "v8.h"
 
 #include "accessors.h"
@@ skipping 14978 matching lines @@
   HandleScope scope(isolate);
   ASSERT(args.length() == 1);
   CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 0);
 
   Handle<Context> creation_context(object->GetCreationContext(), isolate);
   return isolate->heap()->ToBoolean(
       ContextsHaveSameOrigin(creation_context, isolate->native_context()));
 }
 
 
+RUNTIME_FUNCTION(Runtime_NativeObjectObserve) {
+  HandleScope scope(isolate);
+  ASSERT(args.length() == 3);
+  CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 0);
+  CONVERT_ARG_HANDLE_CHECKED(Object, callback, 1);

[Jakob Kummerow, 2014/05/02 19:31:37]

Please be as specific as you can regarding the object type here. I have a
feeling |callback| must be a JSFunction, so use
CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 1);

[adamk, 2014/05/02 20:09:13]

Done.

[Toon Verwaest, 2014/05/03 05:28:26]

I think this should become JSReceiver to handle function proxy callbacks.

On 2014/05/02 19:31:37, Jakob wrote:

[adamk, 2014/05/05 19:55:05]

I've filed a but to track the fact that proxies don't work with Object.observe
at the moment: https://code.google.com/p/v8/issues/detail?id=3310

[rossberg, 2014/05/06 10:03:50]

Thanks! While you're at it, can you please also create bugs to track the issues
that

1. observing the global object does not currently allowed, due to various issues
(esp with navigation, e.g. notifier context)
2. observing/unobserving in the middle of a splice event does not work correctly
3. change records in cross context scenarios live in different contexts
depending on how they are created
4. the implementation of observe does not handle acceptList accurately (e.g.
observable side effects in accessing .length multiple times)

Some of these might require modifications/extensions to the spec as well.

+  CONVERT_ARG_HANDLE_CHECKED(Object, accept, 2);
+
+  Handle<Context> context(object->GetCreationContext(), isolate);
+  Handle<JSFunction> function(context->native_object_observe(), isolate);
+  Handle<Object> call_args[] = { object, callback, accept };
+  Handle<Object> result;
+
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, result,
+      Execution::Call(isolate, function,
+          handle(context->object_function(), isolate), 3, call_args, true));
+  return *result;
+}
+
+
+RUNTIME_FUNCTION(Runtime_NativeObjectGetNotifier) {
+  HandleScope scope(isolate);
+  ASSERT(args.length() == 1);
+  CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 0);
+
+  Handle<Context> context(object->GetCreationContext(), isolate);
+  Handle<JSFunction> function(context->native_object_get_notifier(), isolate);
+  Handle<Object> call_args[] = { object };
+  Handle<Object> result;
+
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, result,
+      Execution::Call(isolate, function,
+          handle(context->object_function(), isolate), 1, call_args, true));
+  return *result;
+}
+
+
+RUNTIME_FUNCTION(Runtime_NativeObjectNotifierPerformChange) {

[Jakob Kummerow, 2014/05/02 19:31:37]

I don't see this function declared in runtime.h, is that intentional?

[adamk, 2014/05/02 20:09:13]

That is not intentional, and combined with the same-naming, it appears that this
caused us to not be doing the right thing with performChange. I've updated the
performChange leak test to actually test that performChange changes contexts.

+  HandleScope scope(isolate);
+  ASSERT(args.length() == 3);
+  CONVERT_ARG_HANDLE_CHECKED(JSObject, object_info, 0);
+  CONVERT_ARG_HANDLE_CHECKED(Object, change_type, 1);
+  CONVERT_ARG_HANDLE_CHECKED(Object, change_fn, 2);
+
+  Handle<Context> context(object_info->GetCreationContext(), isolate);
+  Handle<JSFunction> function(context->native_object_notifier_perform_change(),
+      isolate);
+  Handle<Object> call_args[] = { change_type, change_fn };
+  Handle<Object> result;
+
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, result,
+      Execution::Call(isolate, function, isolate->factory()->undefined_value(),
+                      2, call_args, true));
+  return *result;
+}
+
+
 static Object* ArrayConstructorCommon(Isolate* isolate,
                                            Handle<JSFunction> constructor,
                                            Handle<AllocationSite> site,
                                            Arguments* caller_args) {
   Factory* factory = isolate->factory();
 
   bool holey = false;
   bool can_use_type_feedback = true;
   if (caller_args->length() == 1) {
     Handle<Object> argument_one = caller_args->at<Object>(0);
@@ skipping 195 matching lines @@
   }
   return NULL;
 }
 
 
 const Runtime::Function* Runtime::FunctionForId(Runtime::FunctionId id) {
   return &(kIntrinsicFunctions[static_cast<int>(id)]);
 }
 
 } }  // namespace v8::internal