[webkit-reviews] review granted: [Bug 206846] [JSC] Give up IC when unknown structure transition happens : [Attachment 389099] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 206846: [JSC] Give up IC when unknown structure transition happens
https://bugs.webkit.org/show_bug.cgi?id=206846

Attachment 389099: Patch

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




--- Comment #6 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 389099
  --> https://bugs.webkit.org/attachment.cgi?id=389099
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:12
> +	   When we are creating Put IC for new property, we grab old Structure
before performing put.
> +	   Our custom ::put has convention that implemented ::put mark
PutPropertySlot non-cachable
> +	   appropriately so that IC can work correctly. If we miss this,
semantic failure can happen.
> +	   This patch hardens this semantic failure case by giving up IC when
newStructure calculated
> +	   from oldStructure does not match against the actual structure.

I suggest a few edits for clarity:

When we are creating Put IC for a new property, we grab the old Structure
before performing the put.
For a custom ::put, our convention is that the implemented ::put should mark
the PutPropertySlot 
as non-cachable.  The IC code relies on this in order to work correctly.  If we
didn't mark it as non-cacheable,
a semantic failure can happen.	This patch hardens the code against this
semantic failure case by giving up
trying to cache the IC when the newStructure calculated from oldStructure does
not match against the actual 
structure after the put operation.

> Source/WebCore/ChangeLog:10
> +	   when it implements custom ::put operation which has side-effect
regardless of Structure. IC could be setup
> +	   and IC can do fast path without consulting to custom ::put
operation.

"Structure. IC could be setup" => "Structure. Otherwise, IC can be setup"
"without consulting to custom" => ""without consulting the custom"

> Source/JavaScriptCore/jit/Repatch.cpp:615
> +		   // If JSObject::put is overridden by UserObject,
UserObject::put performs side-effect on JSObject::put, and it forget making
PutPropertySlot non-cachaeble,
> +		   // then arbitrary structure transitions can happen during
the put operation, and this generates wrong transition information here,
oldStructure -> newStructure,
> +		   // while the reality is oldStructure -> something unknown
structures -> baseValue's structure. We should check, and give up to guard
against the incorrect embedder's
> +		   // UserObject::put implementation.

Some suggestions:

"it forget making PutPropertySlot" => "it neglects to mark the PutPropertySlot
as"
"wrong transition information here, oldStructure -> newStructure" => "wrong
transition information here as if oldStructure -> newStructure."
"while the reality is" => "In reality, the transition is"
"We should check, and give up to guard against the incorrect embedder's
UserObject::put implementation." => "To guard against the embedder's
potentially incorrect UserObject::put implementation, we should check for this
condition and if found, give up on caching the put."

> Source/JavaScriptCore/tools/JSDollarVM.cpp:2304
> +    auto* dollarVM = jsDynamicCast<JSDollarVM*>(vm, callFrame->thisValue());
> +    if (!dollarVM)
> +	   return JSValue::encode(jsNull());

Do you expect the function to be called with a this value that is not $vm?  I'm
guessing you're trying to prevent unauthorized use of this function.  I think
you don't need this.  The DollarVMAssertScope above already ensures that
JSC_useDollarVM is true, i.e. the client has authorization to call this
function.  Since tests that call this function are always well-formed, we don't
need this check.  Though if you want to keep it, that's fine too.

> JSTests/stress/ensure-crash.js:1
> +//@ crash!

I suggest adding a //@ runDefault line before this.  There's no point in
running this test on all flavors.  We just need to check it once, right?