CallIC and KeyedCallIC not wrapping this.

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index 968b45d0b61f4ac27c102cdc8dae927cc8c51ff2..81b659efa9f89ee01a38e9680b383f11f4944598 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -435,16 +435,25 @@ Object* CallICBase::TryCallAsFunction(Object* object) {
 }
 
 
-void CallICBase::ReceiverToObject(Handle<Object> object) {
-  HandleScope scope;
-  Handle<Object> receiver(object);
+void CallICBase::ReceiverToObjectIfRequired(Handle<Object> callee,
+                                            Handle<Object> object) {
+  if (callee->IsJSFunction()) {
+    JSFunction* function = JSFunction::cast(*callee);
+    if (function->shared()->strict_mode() || function->IsBuiltin()) {
+      // Do not wrap receiver for strict mode functions or for builtins.
+      return;
+    }
+  }
 
-  // Change the receiver to the result of calling ToObject on it.
-  const int argc = this->target()->arguments_count();
-  StackFrameLocator locator;
-  JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
-  int index = frame->ComputeExpressionsCount() - (argc + 1);
-  frame->SetExpression(index, *Factory::ToObject(object));
+  // And only wrap string, number or boolean.
+  if (object->IsString() || object->IsNumber() || object->IsBoolean()) {

[Martin Maly, 2011/02/18 00:55:38]

I don't understand why the object was double-wrapped in a handle. Based on my
understanding it is not necessary but if it was necessary I wonder why the
Handle<Object> object which arrives straight from the IC. I am assuming GC will
discover that object by walking the stack.

[Mads Ager (chromium), 2011/02/18 07:22:48]

Yes, there is no reason to rewrap the incoming objects.

+    // Change the receiver to the result of calling ToObject on it.
+    const int argc = this->target()->arguments_count();
+    StackFrameLocator locator;
+    JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
+    int index = frame->ComputeExpressionsCount() - (argc + 1);
+    frame->SetExpression(index, *Factory::ToObject(object));
+  }
 }
 
 
@@ -458,10 +467,6 @@ MaybeObject* CallICBase::LoadFunction(State state,
     return TypeError("non_object_property_call", object, name);
   }
 
-  if (object->IsString() || object->IsNumber() || object->IsBoolean()) {
-    ReceiverToObject(object);
-  }
-
   // Check if the name is trivially convertible to an index and get
   // the element if so.
   uint32_t index;
@@ -500,11 +505,20 @@ MaybeObject* CallICBase::LoadFunction(State state,
 
   // Get the property.
   PropertyAttributes attr;
-  Object* result;
+  HandleScope scope;
+  Handle<Object> result;

[Mads Ager (chromium), 2011/02/18 07:22:48]

I would move the object_result out here (maybe call it result and call the
handle result_handle.) Then only create the HandleScope after the GetProperty
and ToObject part.

Then you can add a comment that you wrap the result in a handle because
ReceiverToObjectIfRequired can cause a GC.

It would be nice to just use a handlified version of GetProperty but that would
redo work that you already did, so I would probably leave the raw pointers stuff
in here.

[Martin Maly, 2011/02/19 01:26:13]

Done.

   { MaybeObject* maybe_result =
         object->GetProperty(*object, &lookup, *name, &attr);
-    if (!maybe_result->ToObject(&result)) return maybe_result;
+    Object* object_result;
+    if (!maybe_result->ToObject(&object_result)) return maybe_result;
+    result = Handle<Object>(object_result);
   }
+
+  // Make receiver an object if the callee requires it. Strict mode or builtin
+  // functions do not wrap the receiver, non-strict functions and objects
+  // called as functions do.
+  ReceiverToObjectIfRequired(result, object);
+
   if (lookup.type() == INTERCEPTOR) {

[Mads Ager (chromium), 2011/02/18 07:22:48]

It looks like you can move the wrapping below this exception throwing step as
well. If you throw an exception from here, there is no reason to wrap the
receiver. However, the more important part is that LookupResults are not GC
safe. I believe it is safe for the things you are using here (not accessing any
heap object pointers stored on the LookupResult). In general you have to be
careful with LookupResults. To avoid having to think about it I would move the
wrapping to after the final checks on lookup.

[Martin Maly, 2011/02/19 01:26:13]

Done.

     // If the object does not have the requested property, check which
     // exception we need to throw.
@@ -516,7 +530,7 @@ MaybeObject* CallICBase::LoadFunction(State state,
     }
   }
 
-  ASSERT(result != Heap::the_hole_value());
+  ASSERT(*result != Heap::the_hole_value());

[Mads Ager (chromium), 2011/02/18 07:22:48]

ASSERT(!result->IsTheHole());

[Martin Maly, 2011/02/19 01:26:13]

Done.

 
   if (result->IsJSFunction()) {
 #ifdef ENABLE_DEBUGGER_SUPPORT
@@ -524,23 +538,20 @@ MaybeObject* CallICBase::LoadFunction(State state,
     if (Debug::StepInActive()) {
       // Protect the result in a handle as the debugger can allocate and might
       // cause GC.
-      HandleScope scope;
-      Handle<JSFunction> function(JSFunction::cast(result));
+      Handle<JSFunction> function(JSFunction::cast(*result));

[Martin Maly, 2011/02/18 00:55:38]

No HandleScope needed here, I believe ... the Handle will simply use the one
above (line 508).

[Mads Ager (chromium), 2011/02/18 07:22:48]

Yes, that's fine.

       Debug::HandleStepIn(function, object, fp(), false);
       return *function;
     }
 #endif
 
-    return result;
+    return *result;
   }
 
   // Try to find a suitable function delegate for the object at hand.
-  result = TryCallAsFunction(result);
-  MaybeObject* answer = result;
-  if (!result->IsJSFunction()) {
-    answer = TypeError("property_not_function", object, name);
-  }
-  return answer;
+  result = Handle<Object>(TryCallAsFunction(*result));
+  if (result->IsJSFunction()) return *result;

[Martin Maly, 2011/02/18 00:55:38]

I am a bit hazy on using
"return handle.EscapeFrom(&scope)"
vs.
"return *handle;"
I saw both in the codebase...

[Mads Ager (chromium), 2011/02/18 07:22:48]

This method returns a MaybeObject which is a raw object pointer. It can either
be a Smi, an Object pointer or a failure. The caller will deal with failures
(either throwing exceptions or garbage collecting or just returning the result).
So here, you just have to extract the raw Object* from the handle and return
that. This is done with "return *handle".

handle.EscapeFrom(&scope) is used rarely. It is used in situations where you
have a local handle scope and you want to return one of those handles to the
caller (who has his own handle scope). If you just return the handle you will
get into trouble because that handle belongs to your local handle scope. Once
the local handle scope gets destructed as part of returning, the GC no longer
knows about the value in your handle and you are no better off than with a raw
pointer. handle.EscapeFrom(&scope) moves the handle from the current scope to
the outer scope which makes it safe to return it. The outer scope is still
active after returning and the GC will therefore update the pointer in the
handle.

+
+  return TypeError("property_not_function", object, name);
 }
 
 
@@ -565,8 +576,8 @@ bool CallICBase::TryUpdateExtraICState(LookupResult* lookup,
     case kStringCharAt:
       if (object->IsString()) {
         String* string = String::cast(*object);
-        // Check that there's the right wrapper in the receiver slot.
-        ASSERT(string == JSValue::cast(args[0])->value());
+        // Check there's the right string value or wrapper in the receiver slot.
+        ASSERT(string == args[0] || string == JSValue::cast(args[0])->value());
         // If we're in the default (fastest) state and the index is
         // out of bounds, update the state to record this fact.
         if (*extra_ic_state == DEFAULT_STRING_STUB &&
@@ -775,10 +786,6 @@ MaybeObject* KeyedCallIC::LoadFunction(State state,
     return TypeError("non_object_property_call", object, key);
   }
 
-  if (object->IsString() || object->IsNumber() || object->IsBoolean()) {
-    ReceiverToObject(object);
-  }
-
   if (FLAG_use_ic && state != MEGAMORPHIC && !object->IsAccessCheckNeeded()) {
     int argc = target()->arguments_count();
     InLoopFlag in_loop = target()->ic_in_loop();
@@ -793,17 +800,25 @@ MaybeObject* KeyedCallIC::LoadFunction(State state,
 #endif
     }
   }
-  Object* result;
+
+  HandleScope scope;
+  Handle<Object> result;

[Mads Ager (chromium), 2011/02/18 07:22:48]

This case can be rewritten to just use handles. You don't need any of the
maybe_object stuff below and you can just call the handlified version of
GetObjectProperty (defined in handles.[h,cc]:

Handle<Object> result = GetObjectProperty(object, key);

[Martin Maly, 2011/02/19 01:26:13]

Done.

   { MaybeObject* maybe_result = Runtime::GetObjectProperty(object, key);
-    if (!maybe_result->ToObject(&result)) return maybe_result;
+    Object* object_result;
+    if (!maybe_result->ToObject(&object_result)) return maybe_result;
+    result = Handle<Object>(object_result);

[Martin Maly, 2011/02/18 00:55:38]

Is there an established V8 pattern to export objects out of the nested scope and
Handle-ify it?

   }
-  if (result->IsJSFunction()) return result;
-  result = TryCallAsFunction(result);
-  MaybeObject* answer = result;
-  if (!result->IsJSFunction()) {
-    answer = TypeError("property_not_function", object, key);
-  }
-  return answer;
+
+  // Make receiver an object if the callee requires it. Strict mode or builtin
+  // functions do not wrap the receiver, non-strict functions and objects
+  // called as functions do.
+  ReceiverToObjectIfRequired(result, object);
+
+  if (result->IsJSFunction()) return *result;
+  result = Handle<Object>(TryCallAsFunction(*result));
+  if (result->IsJSFunction()) return *result;
+
+  return TypeError("property_not_function", object, key);

[Martin Maly, 2011/02/18 00:55:38]

This sequence seemed cleaner, get rid of "answer" altogether.

[Mads Ager (chromium), 2011/02/18 07:22:48]

Yes, thanks for cleaning that up! The original code was very strange. :)

 }