JSFunction::put() should not allow caching of lazily reified properties.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Oct 2016 23:46:13 +0000 (23:46 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Oct 2016 23:46:13 +0000 (23:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164081

Reviewed by Geoffrey Garen.

It is incorrect for JSFunction::put() to return PutPropertySlots that indicates
that its lazily reified properties (e.g. .caller, and .arguments) are cacheable.
The reason for this is:

1. Currently, a cacheable put may only consist of the following types of put
   operations:
   a. putting a new property at an offset in the object storage.
   b. changing the value of an existing property at an offset in the object storage.
   c. invoking the setter for a property at an offset in the object storage.

   Returning a PutPropertySlot that indicates the property is cacheable means that
   the property put must be one of the above operations.

   For lazily reified properties, JSFunction::put() implements complex conditional
   behavior that is different than the set of cacheable put operations above.
   Hence, it should not claim that the property put is cacheable.

2. Cacheable puts are cached on the original structure of the object before the
   put operation.

   Reifying a lazy property will trigger a structure transition.  Even though
   subsequent puts to such a property may be cacheable after the structure
   transition, it is incorrect to indicate that the property put is cacheable
   because the caching is on the original structure, not the new transitioned
   structure.

Also fixed some missing exception checks.

* jit/JITOperations.cpp:
* runtime/JSFunction.cpp:
(JSC::JSFunction::put):
(JSC::JSFunction::reifyLazyPropertyIfNeeded):
(JSC::JSFunction::reifyBoundNameIfNeeded):
* runtime/JSFunction.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@208018 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/runtime/JSFunction.cpp
Source/JavaScriptCore/runtime/JSFunction.h

index 69f565b..78e8c3a 100644 (file)
@@ -1,3 +1,45 @@
+2016-10-27  Mark Lam  <mark.lam@apple.com>
+
+        JSFunction::put() should not allow caching of lazily reified properties.
+        https://bugs.webkit.org/show_bug.cgi?id=164081
+
+        Reviewed by Geoffrey Garen.
+
+        It is incorrect for JSFunction::put() to return PutPropertySlots that indicates
+        that its lazily reified properties (e.g. .caller, and .arguments) are cacheable.
+        The reason for this is:
+
+        1. Currently, a cacheable put may only consist of the following types of put
+           operations:
+           a. putting a new property at an offset in the object storage.
+           b. changing the value of an existing property at an offset in the object storage.
+           c. invoking the setter for a property at an offset in the object storage.
+
+           Returning a PutPropertySlot that indicates the property is cacheable means that
+           the property put must be one of the above operations.
+
+           For lazily reified properties, JSFunction::put() implements complex conditional
+           behavior that is different than the set of cacheable put operations above.
+           Hence, it should not claim that the property put is cacheable.
+    
+        2. Cacheable puts are cached on the original structure of the object before the
+           put operation.
+
+           Reifying a lazy property will trigger a structure transition.  Even though
+           subsequent puts to such a property may be cacheable after the structure
+           transition, it is incorrect to indicate that the property put is cacheable
+           because the caching is on the original structure, not the new transitioned
+           structure.
+
+        Also fixed some missing exception checks.
+
+        * jit/JITOperations.cpp:
+        * runtime/JSFunction.cpp:
+        (JSC::JSFunction::put):
+        (JSC::JSFunction::reifyLazyPropertyIfNeeded):
+        (JSC::JSFunction::reifyBoundNameIfNeeded):
+        * runtime/JSFunction.h:
+
 2016-10-27  Caitlin Potter  <caitp@igalia.com>
 
         [JSC] forbid lexical redeclaration of generator formal parameters
index 8f8d4ad..2c045f8 100644 (file)
@@ -196,12 +196,15 @@ EncodedJSValue JIT_OPERATION operationTryGetByIdOptimize(ExecState* exec, Struct
 {
     VM* vm = &exec->vm();
     NativeCallFrameTracer tracer(vm, exec);
+    auto scope = DECLARE_THROW_SCOPE(*vm);
     Identifier ident = Identifier::fromUid(vm, uid);
 
     JSValue baseValue = JSValue::decode(base);
     PropertySlot slot(baseValue, PropertySlot::InternalMethodType::VMInquiry);
 
     baseValue.getPropertySlot(exec, ident, slot);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+
     if (stubInfo->considerCaching(baseValue.structureOrNull()) && !slot.isTaintedByOpaqueObject() && (slot.isCacheableValue() || slot.isCacheableGetter() || slot.isUnset()))
         repatchGetByID(exec, baseValue, ident, slot, *stubInfo, GetByIDKind::Pure);
 
@@ -387,7 +390,8 @@ void JIT_OPERATION operationPutByIdStrictOptimize(ExecState* exec, StructureStub
     
     VM* vm = &exec->vm();
     NativeCallFrameTracer tracer(vm, exec);
-    
+    auto scope = DECLARE_THROW_SCOPE(*vm);
+
     Identifier ident = Identifier::fromUid(vm, uid);
     AccessType accessType = static_cast<AccessType>(stubInfo->accessType);
 
@@ -398,7 +402,8 @@ void JIT_OPERATION operationPutByIdStrictOptimize(ExecState* exec, StructureStub
 
     Structure* structure = baseValue.isCell() ? baseValue.asCell()->structure(*vm) : nullptr;
     baseValue.putInline(exec, ident, value, slot);
-    
+    RETURN_IF_EXCEPTION(scope, void());
+
     if (accessType != static_cast<AccessType>(stubInfo->accessType))
         return;
     
@@ -412,7 +417,8 @@ void JIT_OPERATION operationPutByIdNonStrictOptimize(ExecState* exec, StructureS
     
     VM* vm = &exec->vm();
     NativeCallFrameTracer tracer(vm, exec);
-    
+    auto scope = DECLARE_THROW_SCOPE(*vm);
+
     Identifier ident = Identifier::fromUid(vm, uid);
     AccessType accessType = static_cast<AccessType>(stubInfo->accessType);
 
@@ -423,7 +429,8 @@ void JIT_OPERATION operationPutByIdNonStrictOptimize(ExecState* exec, StructureS
 
     Structure* structure = baseValue.isCell() ? baseValue.asCell()->structure(*vm) : nullptr;    
     baseValue.putInline(exec, ident, value, slot);
-    
+    RETURN_IF_EXCEPTION(scope, void());
+
     if (accessType != static_cast<AccessType>(stubInfo->accessType))
         return;
     
@@ -516,6 +523,7 @@ static void putByVal(CallFrame* callFrame, JSValue baseValue, JSValue subscript,
     if (byValInfo->stubInfo && (!isStringOrSymbol(subscript) || byValInfo->cachedId != property))
         byValInfo->tookSlowPath = true;
 
+    scope.release();
     PutPropertySlot slot(baseValue, callFrame->codeBlock()->isStrictMode());
     baseValue.putInline(callFrame, property, value, slot);
 }
index adabfca..8584bc2 100644 (file)
@@ -429,24 +429,26 @@ bool JSFunction::put(JSCell* cell, ExecState* exec, PropertyName propertyName, J
         return ordinarySetSlow(exec, thisObject, propertyName, value, slot.thisValue(), slot.isStrictMode());
 
     if (thisObject->isHostOrBuiltinFunction()) {
-        thisObject->reifyBoundNameIfNeeded(vm, exec, propertyName);
+        LazyPropertyType propType = thisObject->reifyBoundNameIfNeeded(vm, exec, propertyName);
+        if (propType == LazyPropertyType::IsLazyProperty)
+            slot.disableCaching();
         return Base::put(thisObject, exec, propertyName, value, slot);
     }
 
     if (propertyName == vm.propertyNames->prototype) {
+        slot.disableCaching();
         // Make sure prototype has been reified, such that it can only be overwritten
         // following the rules set out in ECMA-262 8.12.9.
-        PropertySlot slot(thisObject, PropertySlot::InternalMethodType::VMInquiry);
-        thisObject->methodTable(vm)->getOwnPropertySlot(thisObject, exec, propertyName, slot);
+        PropertySlot getSlot(thisObject, PropertySlot::InternalMethodType::VMInquiry);
+        thisObject->methodTable(vm)->getOwnPropertySlot(thisObject, exec, propertyName, getSlot);
         if (thisObject->m_rareData)
             thisObject->m_rareData->clear("Store to prototype property of a function");
-        // Don't allow this to be cached, since a [[Put]] must clear m_rareData.
-        PutPropertySlot dontCache(thisObject);
         scope.release();
-        return Base::put(thisObject, exec, propertyName, value, dontCache);
+        return Base::put(thisObject, exec, propertyName, value, slot);
     }
 
-    if (propertyName == exec->propertyNames().arguments || propertyName == exec->propertyNames().caller) {
+    if (propertyName == vm.propertyNames->arguments || propertyName == vm.propertyNames->caller) {
+        slot.disableCaching();
         if (!thisObject->jsExecutable()->hasCallerAndArgumentsProperties()) {
             // This will trigger the property to be reified, if this is not already the case!
             // FIXME: Investigate if the `hasProperty()` call is even needed, as in the `!hasCallerAndArgumentsProperties()` case,
@@ -458,7 +460,9 @@ bool JSFunction::put(JSCell* cell, ExecState* exec, PropertyName propertyName, J
         }
         return typeError(exec, scope, slot.isStrictMode(), ASCIILiteral(ReadonlyPropertyWriteError));
     }
-    thisObject->reifyLazyPropertyIfNeeded(vm, exec, propertyName);
+    LazyPropertyType propType = thisObject->reifyLazyPropertyIfNeeded(vm, exec, propertyName);
+    if (propType == LazyPropertyType::IsLazyProperty)
+        slot.disableCaching();
     scope.release();
     return Base::put(thisObject, exec, propertyName, value, slot);
 }
@@ -661,25 +665,28 @@ void JSFunction::reifyName(VM& vm, ExecState* exec, String name)
     rareData->setHasReifiedName();
 }
 
-void JSFunction::reifyLazyPropertyIfNeeded(VM& vm, ExecState* exec, PropertyName propertyName)
+JSFunction::LazyPropertyType JSFunction::reifyLazyPropertyIfNeeded(VM& vm, ExecState* exec, PropertyName propertyName)
 {
     if (propertyName == vm.propertyNames->length) {
         if (!hasReifiedLength())
             reifyLength(vm);
+        return LazyPropertyType::IsLazyProperty;
     } else if (propertyName == vm.propertyNames->name) {
         if (!hasReifiedName())
             reifyName(vm, exec);
+        return LazyPropertyType::IsLazyProperty;
     }
+    return LazyPropertyType::NotLazyProperty;
 }
 
-void JSFunction::reifyBoundNameIfNeeded(VM& vm, ExecState* exec, PropertyName propertyName)
+JSFunction::LazyPropertyType JSFunction::reifyBoundNameIfNeeded(VM& vm, ExecState* exec, PropertyName propertyName)
 {
     const Identifier& nameIdent = vm.propertyNames->name;
     if (propertyName != nameIdent)
-        return;
+        return LazyPropertyType::NotLazyProperty;
 
     if (hasReifiedName())
-        return;
+        return LazyPropertyType::IsLazyProperty;
 
     if (this->inherits(JSBoundFunction::info())) {
         FunctionRareData* rareData = this->rareData(vm);
@@ -688,6 +695,7 @@ void JSFunction::reifyBoundNameIfNeeded(VM& vm, ExecState* exec, PropertyName pr
         putDirect(vm, nameIdent, jsString(exec, name), initialAttributes);
         rareData->setHasReifiedName();
     }
+    return LazyPropertyType::IsLazyProperty;
 }
 
 } // namespace JSC
index fdead0b..8df03a1 100644 (file)
@@ -190,9 +190,11 @@ private:
     bool hasReifiedName() const;
     void reifyLength(VM&);
     void reifyName(VM&, ExecState*);
-    void reifyBoundNameIfNeeded(VM&, ExecState*, PropertyName);
     void reifyName(VM&, ExecState*, String name);
-    void reifyLazyPropertyIfNeeded(VM&, ExecState*, PropertyName propertyName);
+
+    enum class LazyPropertyType { NotLazyProperty, IsLazyProperty };
+    LazyPropertyType reifyLazyPropertyIfNeeded(VM&, ExecState*, PropertyName);
+    LazyPropertyType reifyBoundNameIfNeeded(VM&, ExecState*, PropertyName);
 
     friend class LLIntOffsetsExtractor;