Change LookupForWrite to always do a full lookup and check the result.

src/ic.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 1400 matching lines @@
           name, receiver, holder);
     default:
       // Always rewrite to the generic case so that we do not
       // repeatedly try to rewrite.
       return generic_stub();
   }
   return Handle<Code>::null();
 }
 
 
-static bool StoreICableLookup(LookupResult* lookup) {
-  // Bail out if we didn't find a result.
-  if (!lookup->IsFound()) return false;
-
-  // Bail out if inline caching is not allowed.
-  if (!lookup->IsCacheable()) return false;
-
-  // If the property is read-only, we leave the IC in its current state.
-  if (lookup->IsTransition()) {
-    return !lookup->GetTransitionDetails().IsReadOnly();
-  }
-  return !lookup->IsReadOnly();
-}
-
-
 static bool LookupForWrite(Handle<JSObject> receiver,
                            Handle<String> name,
                            LookupResult* lookup) {
-  receiver->LocalLookup(*name, lookup);
-  if (!lookup->IsFound()) {
-    receiver->map()->LookupTransition(*receiver, *name, lookup);
-  }
-  if (!StoreICableLookup(lookup)) {
-    // 2nd chance: There can be accessors somewhere in the prototype chain.
-    receiver->Lookup(*name, lookup);
-    return lookup->IsPropertyCallbacks() && StoreICableLookup(lookup);
+  Handle<JSObject> holder = receiver;
+  receiver->Lookup(*name, lookup);
+  if (lookup->IsFound()) {
+    if (lookup->IsReadOnly() || !lookup->IsCacheable()) return false;
+
+    if (lookup->holder() == *receiver) {
+      if (lookup->IsInterceptor() &&
+          receiver->GetNamedInterceptor()->setter()->IsUndefined()) {
+        receiver->LocalLookupRealNamedProperty(*name, lookup);
+        return lookup->IsFound() &&
+            !lookup->IsReadOnly() &&
+            lookup->IsCacheable();
+      }
+      return true;
+    }
+
+    if (lookup->IsPropertyCallbacks()) return true;

[rossberg, 2013/03/22 14:07:44]

Test case... :)

[Toon Verwaest, 2013/03/25 11:44:44]

Done.

+
+    // Currently normal holders in the prototype chain are not supported. They
+    // would require a runtime positive lookup and verification that the details
+    // have not changed.
+    if (lookup->IsInterceptor() || lookup->IsNormal()) return false;
+    holder = Handle<JSObject>(lookup->holder(), lookup->isolate());
   }
 
-  if (lookup->IsInterceptor() &&
-      receiver->GetNamedInterceptor()->setter()->IsUndefined()) {
-    receiver->LocalLookupRealNamedProperty(*name, lookup);
-    return StoreICableLookup(lookup);
-  }
-
-  return true;
+  receiver->map()->LookupTransition(*holder, *name, lookup);

[rossberg, 2013/03/22 14:07:44]

Maybe add a comment explaining why you use the holder here.

[Toon Verwaest, 2013/03/25 11:44:44]

Done.

+  return lookup->IsTransition() &&
+      !lookup->GetTransitionDetails(receiver->map()).IsReadOnly();
 }
 
 
 MaybeObject* StoreIC::Store(State state,
                             StrictModeFlag strict_mode,
                             Handle<Object> object,
                             Handle<String> name,
                             Handle<Object> value,
                             JSReceiver::StoreFromKeyed store_mode) {
   // Handle proxies.
@@ skipping 79 matching lines @@
 }
 
 
 void StoreIC::UpdateCaches(LookupResult* lookup,
                            State state,
                            StrictModeFlag strict_mode,
                            Handle<JSObject> receiver,
                            Handle<String> name,
                            Handle<Object> value) {
   ASSERT(!receiver->IsJSGlobalProxy());
-  ASSERT(StoreICableLookup(lookup));
   ASSERT(lookup->IsFound());
 
   // These are not cacheable, so we never see such LookupResults here.
   ASSERT(!lookup->IsHandler());
 
   Handle<Code> code =
       ComputeStoreMonomorphic(lookup, strict_mode, receiver, name);
   if (code.is_null()) return;
 
   PatchCache(state, strict_mode, receiver, name, code);
   TRACE_IC("StoreIC", name, state, target());
 }
 
 
 Handle<Code> StoreIC::ComputeStoreMonomorphic(LookupResult* lookup,
                                               StrictModeFlag strict_mode,
                                               Handle<JSObject> receiver,
                                               Handle<String> name) {
   Handle<JSObject> holder(lookup->holder());
   switch (lookup->type()) {
     case FIELD:
       return isolate()->stub_cache()->ComputeStoreField(
-          name, receiver, lookup->GetFieldIndex().field_index(),
-          Handle<Map>::null(), strict_mode);
+          name, receiver, lookup, Handle<Map>::null(), strict_mode);
     case NORMAL:
       if (receiver->IsGlobalObject()) {
         // The stub generated for the global object picks the value directly
         // from the property cell. So the property must be directly on the
         // global object.
         Handle<GlobalObject> global = Handle<GlobalObject>::cast(receiver);
         Handle<JSGlobalPropertyCell> cell(
             global->GetPropertyCell(lookup), isolate());
         return isolate()->stub_cache()->ComputeStoreGlobal(
             name, global, cell, strict_mode);
       }
-      if (!holder.is_identical_to(receiver)) break;

[rossberg, 2013/03/22 14:07:44]

If that's an impossible case, maybe turn it into an assertion.

[Toon Verwaest, 2013/03/25 11:44:44]

Done.

       return isolate()->stub_cache()->ComputeStoreNormal(strict_mode);
     case CALLBACKS: {
       Handle<Object> callback(lookup->GetCallbackObject(), isolate());
       if (callback->IsExecutableAccessorInfo()) {
         Handle<ExecutableAccessorInfo> info =
             Handle<ExecutableAccessorInfo>::cast(callback);
         if (v8::ToCData<Address>(info->setter()) == 0) break;
         if (!holder->HasFastProperties()) break;
         if (!info->IsCompatibleReceiver(*receiver)) break;
         return isolate()->stub_cache()->ComputeStoreCallback(
@@ skipping 14 matching lines @@
       // No IC support for old-style native accessors.
       break;
     }
     case INTERCEPTOR:
       ASSERT(!receiver->GetNamedInterceptor()->setter()->IsUndefined());
       return isolate()->stub_cache()->ComputeStoreInterceptor(
           name, receiver, strict_mode);
     case CONSTANT_FUNCTION:
       break;
     case TRANSITION: {
-      Handle<Map> transition(lookup->GetTransitionTarget(), isolate());
+      Handle<Map> transition(
+          lookup->GetTransitionTarget(receiver->map()), isolate());
       int descriptor = transition->LastAdded();
 
       DescriptorArray* target_descriptors = transition->instance_descriptors();
       PropertyDetails details = target_descriptors->GetDetails(descriptor);
 
       if (details.type() != FIELD || details.attributes() != NONE) break;
 
-      int field_index = target_descriptors->GetFieldIndex(descriptor);
       return isolate()->stub_cache()->ComputeStoreField(
-          name, receiver, field_index, transition, strict_mode);
+          name, receiver, lookup, transition, strict_mode);
     }
     case NONEXISTENT:
     case HANDLER:
       UNREACHABLE();
       break;
   }
   return Handle<Code>::null();
 }
 
 
@@ skipping 310 matching lines @@
 Handle<Code> KeyedStoreIC::ComputeStoreMonomorphic(LookupResult* lookup,
                                                    StrictModeFlag strict_mode,
                                                    Handle<JSObject> receiver,
                                                    Handle<String> name) {
   // If the property has a non-field type allowing map transitions
   // where there is extra room in the object, we leave the IC in its
   // current state.
   switch (lookup->type()) {
     case FIELD:
       return isolate()->stub_cache()->ComputeKeyedStoreField(
-          name, receiver, lookup->GetFieldIndex().field_index(),
-          Handle<Map>::null(), strict_mode);
+          name, receiver, lookup, Handle<Map>::null(), strict_mode);
     case TRANSITION: {
-      Handle<Map> transition(lookup->GetTransitionTarget(), isolate());
+      Handle<Map> transition(
+          lookup->GetTransitionTarget(receiver->map()), isolate());
       int descriptor = transition->LastAdded();
 
       DescriptorArray* target_descriptors = transition->instance_descriptors();
       PropertyDetails details = target_descriptors->GetDetails(descriptor);
 
       if (details.type() == FIELD && details.attributes() == NONE) {
-        int field_index = target_descriptors->GetFieldIndex(descriptor);
         return isolate()->stub_cache()->ComputeKeyedStoreField(
-            name, receiver, field_index, transition, strict_mode);
+            name, receiver, lookup, transition, strict_mode);
       }
       // fall through.
     }
     case NORMAL:
     case CONSTANT_FUNCTION:
     case CALLBACKS:
     case INTERCEPTOR:
       // Always rewrite to the generic case so that we do not
       // repeatedly try to rewrite.
       return (strict_mode == kStrictMode)
@@ skipping 786 matching lines @@
 #undef ADDR
 };
 
 
 Address IC::AddressFromUtilityId(IC::UtilityId id) {
   return IC_utilities[id];
 }
 
 
 } }  // namespace v8::internal