[webkit-reviews] review granted: [Bug 217916] [JSC] OrdinarySet should invoke custom [[Set]] methods : [Attachment 426047] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 217916: [JSC] OrdinarySet should invoke custom [[Set]] methods
https://bugs.webkit.org/show_bug.cgi?id=217916

Attachment 426047: Patch

https://bugs.webkit.org/attachment.cgi?id=426047&action=review




--- Comment #13 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 426047
  --> https://bugs.webkit.org/attachment.cgi?id=426047
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426047&action=review

This is awesome. r=me

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:229
> +    RELEASE_AND_RETURN(scope, obj->methodTable(vm)->put(obj, globalObject,
propertyName, value, slot));

This is a bit incorrect, but the incorrectness is unobservable.

"Hello".length = 42;

This should be an error when we are assigning this value to StringObject
materialized from "Hello". But we avoid materializing it (that's important
optimization), and we are querying to StringPrototype first.
And since StringPrototype has the same "length" property, we are anyway
throwing an error.

Can we have an explicit test for this? ("Hello".length = 42 in sloppy and
strict modes) And can we have FIXME comment about it here?

> Source/JavaScriptCore/runtime/JSObject.cpp:823
> +		   customSetter(globalObject,
JSValue::encode(slot.thisValue()), JSValue::encode(value), propertyName);
>		   RETURN_IF_EXCEPTION(scope, false);
> -		   if (result != TriState::Indeterminate)
> -		       return result == TriState::True;
> +		   return true;

Let's make it

scope.release();
customSetter(globalObject, JSValue::encode(slot.thisValue()),
JSValue::encode(value), propertyName);
return true;

> Source/JavaScriptCore/runtime/JSObject.cpp:831
> +		       RELEASE_AND_RETURN(scope, customSetter(globalObject,
JSValue::encode(obj), JSValue::encode(value), propertyName));

Should we return true too?

> Source/JavaScriptCore/runtime/JSObject.h:-1441
> -
> -ALWAYS_INLINE void JSObject::doPutPropertySecurityCheck(JSObject*,
JSGlobalObject*, PropertyName, PutPropertySlot&)
> -{
> -}

This is awesome.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:70
> +	   if (structure->hasReadOnlyOrGetterSetterPropertiesExcludingProto()
|| structure->typeInfo().overridesPut() ||
structure->typeInfo().overridesGetPrototype())

One optimization idea I think. Should we check
`structure->typeInfo().overridesPut()` even for the originating object?
Let's consider a case,

[A: object, which overrides put] -[[Prototype]]-> [B: object, which does not
override put]

In that case, when calling A::put, we are already in A::put. And then we can
consider whether we can use fast-path put or not. At that point, I think we do
not need to check OverridesPut for the initial object since we already called
A::put, is my understanding correct?
RegExpObject, ErrorInstance, JSFunction, TypedArrays, and Arguments objects are
the cases.

(If we do this optimization, we also need to take care for Object.assign case
since that function directly calls it.)

> Source/JavaScriptCore/runtime/PutPropertySlot.h:119
> +    bool isTaintedByOpaqueObject() const { return m_isTaintedByOpaqueObject;
}

Can we remove `Context context() const { return
static_cast<Context>(m_context); }`'s cast?

> Source/JavaScriptCore/runtime/PutPropertySlot.h:140
> +    bool m_isTaintedByOpaqueObject { false };

Can we make it a bit? (`bool m_isTaintedByOpaqueObject : 1`). And we need to
put `false` initialization in the constructor.

> Source/JavaScriptCore/runtime/PutPropertySlot.h:142
>      uint8_t m_context;

Can we use Context instead of uint8_t?

> Source/JavaScriptCore/runtime/StringPrototype.cpp:-168
> -    putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(0),
PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly |
PropertyAttribute::DontEnum);

Nice