[webkit-reviews] review granted: [Bug 227963] definePropertyOnReceiver should check if receiver canPerformFastPutInline : [Attachment 434579] Patch

[bugzilla-daemon at webkit.org]

Alexey Shvayka <shvaikalesh at gmail.com> has granted Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 227963: definePropertyOnReceiver should check if receiver
canPerformFastPutInline
https://bugs.webkit.org/show_bug.cgi?id=227963

Attachment 434579: Patch

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




--- Comment #16 from Alexey Shvayka <shvaikalesh at gmail.com> ---
Comment on attachment 434579
  --> https://bugs.webkit.org/attachment.cgi?id=434579
Patch

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

r=me with comments. Nicely done!

> Source/JavaScriptCore/runtime/JSObject.cpp:884
> +		   return customSetter(globalObject, JSValue::encode(receiver),
JSValue::encode(value), propertyName);

1. After r280256, custom properties are called in realm of their holder, like
`customSetter(receiver->globalObject(vm), ...`.
2. `customSetter` can declare a throw scope, causing unchecked scope error in
--debug. I suggest `RELEASE_AND_RETURN(scope, customSetter...`.

> Source/JavaScriptCore/runtime/JSObject.cpp:942
> +	       ASSERT(!(entry->value->attributes() &
PropertyAttribute::CustomValue));

I've took a brief look and it seems like this ASSERT will hold for
putInlineFastReplacingStaticPropertyIfNeeded() called by
JSObject::putInlineForJSObject(). Nice!

> Source/JavaScriptCore/runtime/JSObjectInlines.h:-285
> -    // FIXME: For a failure due to non-extensible structure, the error
message is misleading.

Maybe we should keep this FIXME as it's not yet resolved?