Strict Mode assignment to read only property.

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index 4f75ade0e8edd96f0b8fad7cbae1125543e171bf..4772a7947d5f042d6123beb182e1eb725c28f61e 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -343,7 +343,7 @@ void StoreIC::Clear(Address address, Code* target) {
   if (target->ic_state() == UNINITIALIZED) return;
   ClearInlinedVersion(address);
   SetTargetAtAddress(address,
-      target->extra_ic_state() == kStoreICStrict
+      target->extra_ic_state() == kStrictMode
         ? initialize_stub_strict()
         : initialize_stub());
 }
@@ -366,7 +366,10 @@ void KeyedStoreIC::RestoreInlinedVersion(Address address) {
 
 void KeyedStoreIC::Clear(Address address, Code* target) {
   if (target->ic_state() == UNINITIALIZED) return;
-  SetTargetAtAddress(address, initialize_stub());
+  SetTargetAtAddress(address,
+      target->extra_ic_state() == kStrictMode
+        ? initialize_stub_strict()
+        : initialize_stub());
 }
 
 
@@ -1227,7 +1230,8 @@ MaybeObject* KeyedLoadIC::Load(State state,
         if (receiver->HasExternalArrayElements()) {
           MaybeObject* probe =
               StubCache::ComputeKeyedLoadOrStoreExternalArray(*receiver,
-                                                              false);
+                                                              false,
+                                                              kNonStrictMode);
           stub = probe->IsFailure() ?
               NULL : Code::cast(probe->ToObjectUnchecked());
         } else if (receiver->HasIndexedInterceptor()) {
@@ -1383,7 +1387,7 @@ static bool LookupForWrite(JSObject* object,
 
 
 MaybeObject* StoreIC::Store(State state,
-                            Code::ExtraICState extra_ic_state,
+                            StrictModeFlag strict_mode,
                             Handle<Object> object,
                             Handle<String> name,
                             Handle<Object> value) {
@@ -1413,11 +1417,11 @@ MaybeObject* StoreIC::Store(State state,
 #ifdef DEBUG
     if (FLAG_trace_ic) PrintF("[StoreIC : +#length /array]\n");
 #endif
-    Builtins::Name target = (extra_ic_state == kStoreICStrict)
+    Builtins::Name target = (strict_mode == kStrictMode)
         ? Builtins::StoreIC_ArrayLength_Strict
         : Builtins::StoreIC_ArrayLength;
     set_target(Builtins::builtin(target));
-    return receiver->SetProperty(*name, *value, NONE);
+    return receiver->SetProperty(*name, *value, NONE, strict_mode);
   }
 
   // Lookup the property locally in the receiver.
@@ -1441,13 +1445,15 @@ MaybeObject* StoreIC::Store(State state,
           // Index is an offset from the end of the object.
           int offset = map->instance_size() + (index * kPointerSize);
           if (PatchInlinedStore(address(), map, offset)) {
-            set_target(megamorphic_stub());
+            set_target(strict_mode == kStrictMode
+                         ? megamorphic_stub_strict()
+                         : megamorphic_stub());
 #ifdef DEBUG
             if (FLAG_trace_ic) {
               PrintF("[StoreIC : inline patch %s]\n", *name->ToCString());
             }
 #endif
-            return receiver->SetProperty(*name, *value, NONE);
+            return receiver->SetProperty(*name, *value, NONE, strict_mode);
 #ifdef DEBUG
 
           } else {
@@ -1474,11 +1480,15 @@ MaybeObject* StoreIC::Store(State state,
 
       // If no inlined store ic was patched, generate a stub for this
       // store.
-      UpdateCaches(&lookup, state, extra_ic_state, receiver, name, value);
+      UpdateCaches(&lookup, state, strict_mode, receiver, name, value);
     } else {
       // Strict mode doesn't allow setting non-existent global property.

[Mads Ager (chromium), 2011/02/28 12:23:06]

Update comment to reflect the read-only part of this code.

[Martin Maly, 2011/03/01 01:40:29]

Done.

-      if (extra_ic_state == kStoreICStrict && IsContextual(object)) {
-        return ReferenceError("not_defined", name);
+      if (strict_mode == kStrictMode) {
+        if (lookup.IsFound() && lookup.IsReadOnly()) {
+          return TypeError("strict_rdonly_property", object, name);

[Mads Ager (chromium), 2011/02/28 12:23:06]

Please don't abbreviate read to rd.

[Martin Maly, 2011/03/01 01:40:29]

Done.

+        } else if (IsContextual(object)) {
+          return ReferenceError("not_defined", name);
+        }
       }
     }
   }
@@ -1486,7 +1496,7 @@ MaybeObject* StoreIC::Store(State state,
   if (receiver->IsJSGlobalProxy()) {
     // Generate a generic stub that goes to the runtime when we see a global
     // proxy as receiver.
-    Code* stub = (extra_ic_state == kStoreICStrict)
+    Code* stub = (strict_mode == kStrictMode)
         ? global_proxy_stub_strict()
         : global_proxy_stub();
     if (target() != stub) {
@@ -1498,13 +1508,13 @@ MaybeObject* StoreIC::Store(State state,
   }
 
   // Set the property.
-  return receiver->SetProperty(*name, *value, NONE);
+  return receiver->SetProperty(*name, *value, NONE, strict_mode);
 }
 
 
 void StoreIC::UpdateCaches(LookupResult* lookup,
                            State state,
-                           Code::ExtraICState extra_ic_state,
+                           StrictModeFlag strict_mode,
                            Handle<JSObject> receiver,
                            Handle<String> name,
                            Handle<Object> value) {
@@ -1526,7 +1536,7 @@ void StoreIC::UpdateCaches(LookupResult* lookup,
   switch (type) {
     case FIELD: {
       maybe_code = StubCache::ComputeStoreField(
-          *name, *receiver, lookup->GetFieldIndex(), NULL, extra_ic_state);
+          *name, *receiver, lookup->GetFieldIndex(), NULL, strict_mode);
       break;
     }
     case MAP_TRANSITION: {
@@ -1536,7 +1546,7 @@ void StoreIC::UpdateCaches(LookupResult* lookup,
       Handle<Map> transition(lookup->GetTransitionMap());
       int index = transition->PropertyIndexFor(*name);
       maybe_code = StubCache::ComputeStoreField(
-          *name, *receiver, index, *transition, extra_ic_state);
+          *name, *receiver, index, *transition, strict_mode);
       break;
     }
     case NORMAL: {
@@ -1548,10 +1558,10 @@ void StoreIC::UpdateCaches(LookupResult* lookup,
         JSGlobalPropertyCell* cell =
             JSGlobalPropertyCell::cast(global->GetPropertyCell(lookup));
         maybe_code = StubCache::ComputeStoreGlobal(
-            *name, *global, cell, extra_ic_state);
+            *name, *global, cell, strict_mode);
       } else {
         if (lookup->holder() != *receiver) return;
-        maybe_code = StubCache::ComputeStoreNormal(extra_ic_state);
+        maybe_code = StubCache::ComputeStoreNormal(strict_mode);
       }
       break;
     }
@@ -1560,13 +1570,13 @@ void StoreIC::UpdateCaches(LookupResult* lookup,
       AccessorInfo* callback = AccessorInfo::cast(lookup->GetCallbackObject());
       if (v8::ToCData<Address>(callback->setter()) == 0) return;
       maybe_code = StubCache::ComputeStoreCallback(
-          *name, *receiver, callback, extra_ic_state);
+          *name, *receiver, callback, strict_mode);
       break;
     }
     case INTERCEPTOR: {
       ASSERT(!receiver->GetNamedInterceptor()->setter()->IsUndefined());
       maybe_code = StubCache::ComputeStoreInterceptor(
-          *name, *receiver, extra_ic_state);
+          *name, *receiver, strict_mode);
       break;
     }
     default:
@@ -1583,7 +1593,7 @@ void StoreIC::UpdateCaches(LookupResult* lookup,
   } else if (state == MONOMORPHIC) {
     // Only move to megamorphic if the target changes.
     if (target() != Code::cast(code)) {
-      set_target(extra_ic_state == kStoreICStrict
+      set_target(strict_mode == kStrictMode
                    ? megamorphic_stub_strict()
                    : megamorphic_stub());
     }
@@ -1599,6 +1609,7 @@ void StoreIC::UpdateCaches(LookupResult* lookup,
 
 
 MaybeObject* KeyedStoreIC::Store(State state,
+                                 StrictModeFlag strict_mode,
                                  Handle<Object> object,
                                  Handle<Object> key,
                                  Handle<Object> value) {
@@ -1630,11 +1641,11 @@ MaybeObject* KeyedStoreIC::Store(State state,
 
     // Update inline cache and stub cache.
     if (FLAG_use_ic) {
-      UpdateCaches(&lookup, state, receiver, name, value);
+      UpdateCaches(&lookup, state, strict_mode, receiver, name, value);
     }
 
     // Set the property.
-    return receiver->SetProperty(*name, *value, NONE);
+    return receiver->SetProperty(*name, *value, NONE, strict_mode);
   }
 
   // Do not use ICs for objects that require access checks (including
@@ -1643,23 +1654,25 @@ MaybeObject* KeyedStoreIC::Store(State state,
   ASSERT(!(use_ic && object->IsJSGlobalProxy()));
 
   if (use_ic) {
-    Code* stub = generic_stub();
+    Code* stub =
+        strict_mode == kStrictMode ? generic_stub_strict() : generic_stub();
     if (state == UNINITIALIZED) {
       if (object->IsJSObject()) {
         Handle<JSObject> receiver = Handle<JSObject>::cast(object);
         if (receiver->HasExternalArrayElements()) {
           MaybeObject* probe =
-              StubCache::ComputeKeyedLoadOrStoreExternalArray(*receiver, true);
+              StubCache::ComputeKeyedLoadOrStoreExternalArray(
+                  *receiver, true, strict_mode);
           stub = probe->IsFailure() ?
               NULL : Code::cast(probe->ToObjectUnchecked());
         } else if (receiver->HasPixelElements()) {
           MaybeObject* probe =
-              StubCache::ComputeKeyedStorePixelArray(*receiver);
+              StubCache::ComputeKeyedStorePixelArray(*receiver, strict_mode);
           stub = probe->IsFailure() ?
               NULL : Code::cast(probe->ToObjectUnchecked());
         } else if (key->IsSmi() && receiver->map()->has_fast_elements()) {
           MaybeObject* probe =
-              StubCache::ComputeKeyedStoreSpecialized(*receiver);
+              StubCache::ComputeKeyedStoreSpecialized(*receiver, strict_mode);
           stub = probe->IsFailure() ?
               NULL : Code::cast(probe->ToObjectUnchecked());
         }
@@ -1669,12 +1682,13 @@ MaybeObject* KeyedStoreIC::Store(State state,
   }
 
   // Set the property.
-  return Runtime::SetObjectProperty(object, key, value, NONE);
+  return Runtime::SetObjectProperty(object, key, value, NONE, strict_mode);
 }
 
 
 void KeyedStoreIC::UpdateCaches(LookupResult* lookup,
                                 State state,
+                                StrictModeFlag strict_mode,
                                 Handle<JSObject> receiver,
                                 Handle<String> name,
                                 Handle<Object> value) {
@@ -1701,8 +1715,8 @@ void KeyedStoreIC::UpdateCaches(LookupResult* lookup,
 
   switch (type) {
     case FIELD: {
-      maybe_code = StubCache::ComputeKeyedStoreField(*name, *receiver,
-                                                     lookup->GetFieldIndex());
+      maybe_code = StubCache::ComputeKeyedStoreField(
+          *name, *receiver, lookup->GetFieldIndex(), NULL, strict_mode);
       break;
     }
     case MAP_TRANSITION: {
@@ -1711,8 +1725,8 @@ void KeyedStoreIC::UpdateCaches(LookupResult* lookup,
         ASSERT(type == MAP_TRANSITION);
         Handle<Map> transition(lookup->GetTransitionMap());
         int index = transition->PropertyIndexFor(*name);
-        maybe_code = StubCache::ComputeKeyedStoreField(*name, *receiver,
-                                                       index, *transition);
+        maybe_code = StubCache::ComputeKeyedStoreField(
+            *name, *receiver, index, *transition, strict_mode);
         break;
       }
       // fall through.
@@ -1720,7 +1734,9 @@ void KeyedStoreIC::UpdateCaches(LookupResult* lookup,
     default: {
       // Always rewrite to the generic case so that we do not
       // repeatedly try to rewrite.
-      maybe_code = generic_stub();
+      maybe_code = strict_mode == kStrictMode
+          ? generic_stub_strict()
+          : generic_stub();
       break;
     }
   }
@@ -1735,7 +1751,9 @@ void KeyedStoreIC::UpdateCaches(LookupResult* lookup,
   if (state == UNINITIALIZED || state == PREMONOMORPHIC) {
     set_target(Code::cast(code));
   } else if (state == MONOMORPHIC) {
-    set_target(megamorphic_stub());
+    set_target(strict_mode == kStrictMode
+                 ? megamorphic_stub_strict()
+                 : megamorphic_stub());
   }
 
 #ifdef DEBUG
@@ -1836,8 +1854,20 @@ MUST_USE_RESULT MaybeObject* StoreIC_Miss(Arguments args) {
   StoreIC ic;
   IC::State state = IC::StateFrom(ic.target(), args[0], args[1]);
   Code::ExtraICState extra_ic_state = ic.target()->extra_ic_state();
-  return ic.Store(state, extra_ic_state, args.at<Object>(0),
-                  args.at<String>(1), args.at<Object>(2));
+  ASSERT(extra_ic_state == kStrictMode || extra_ic_state == kNonStrictMode);
+  MaybeObject* result = ic.Store(state,
+                                 static_cast<StrictModeFlag>(extra_ic_state),
+                                 args.at<Object>(0),
+                                 args.at<String>(1),
+                                 args.at<Object>(2));
+  // If this assert triggers the IC strict mode was changed.
+  // The strict mode flag must be preserved on the IC.
+  //
+  // ic.target() doesn't appear to be valid in some cases when exiting from
+  // ic.Store. Could it be crankshaft OSR?
+  //

[Lasse Reichstein, 2011/02/28 11:18:30]

I don't know. Do you have a way to reproduce the problem?

[Mads Ager (chromium), 2011/02/28 12:23:06]

I would be interested in knowing what fails here as well. 

My guess here would be lazy deoptimization. If SetProperty performs a call to
something that either force deletes a property or enters the debugger, then we
will destroy the code object (overwriting return points with a call to
deoptimization). Therefore the target will be invalid at this point.

So, overall, the assert at this point will not work. We should never, ever use
target() or set_target() after we have invoked something that can call arbitrary
code (such as GetProperty, SetProperty). I don't think we do, but if we do that
will be bad in the presence of lazy deoptimization.

+  // ASSERT(extra_ic_state == ic.target()->extra_ic_state());
+  return result;

[Martin Maly, 2011/02/27 23:04:04]

This will be cleaned up before commit, the value is only the inline question in
the comment. It was my attempt at 3rd order paranoid check that ICs don't switch
strictness.

 }
 
 
@@ -1901,8 +1931,21 @@ MUST_USE_RESULT MaybeObject* KeyedStoreIC_Miss(Arguments args) {
   ASSERT(args.length() == 3);
   KeyedStoreIC ic;
   IC::State state = IC::StateFrom(ic.target(), args[0], args[1]);
-  return ic.Store(state, args.at<Object>(0), args.at<Object>(1),
-                  args.at<Object>(2));
+  Code::ExtraICState extra_ic_state = ic.target()->extra_ic_state();
+  ASSERT(extra_ic_state == kStrictMode || extra_ic_state == kNonStrictMode);
+  MaybeObject* result = ic.Store(state,
+                                 static_cast<StrictModeFlag>(extra_ic_state),
+                                 args.at<Object>(0),
+                                 args.at<Object>(1),
+                                 args.at<Object>(2));
+  // If this assert triggers the IC strict mode was changed.
+  // The strict mode flag must be preserved on the IC.
+  //
+  // ic.target() doesn't appear to be valid in some cases when exiting from
+  // ic.Store. Could it be crankshaft OSR?
+  //

[Lasse Reichstein, 2011/02/28 11:18:30]

Again, can you reproduce it? That would make it much easier to figure out who to
ask about it :)

+  // ASSERT(extra_ic_state == ic.target()->extra_ic_state());

[Lasse Reichstein, 2011/02/28 11:18:30]

Should be removed before commit.

[Martin Maly, 2011/03/01 01:40:29]

Done.

+  return result;
 }