Bug 164081 – JSFunction::put() should not allow caching of lazily reified properties.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Mark Lam]

<rdar://problem/28958291>

[Mark Lam]

Created attachment 293066 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 293066 [details]
proposed patch.

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

r=me with some mixups

> Source/JavaScriptCore/ChangeLog:10
> +        It is incorrect to cache the putting of lazily reified properties because the
> +        inline cache code that is generated needs to be able to operate on other
> +        instances of JSFunctions that may not have these properties reified yet.

This explanation isn't complete.

There are two things going on here:

(1) The setters for these properties implement complex conditional behavior. It's just not true that they are PropertyOffset puts, so it's an error to say that they are.

(2) Even when we instantiate a new property that is a PropertyOffset put (as .caller and .arguments do), it's still wrong to return a cacheable PropertyOffset put. 

The reason for (2) is subtle: Returning a cacheable PropertyOffset put promises that the put is cacheable for any object with our *starting* Structure. But the process of instantiating .caller and .arguments transitions us to a *new* Structure, and the put is only cacheable for our *new* Structure.

It would be correct for some future version of this code to allow caching of lazily reified properties, if it took these problems into account.

> Source/JavaScriptCore/runtime/JSFunction.cpp:446
> +        slot.disableCaching();

I think it's better to put the disableCaching() call right at the top, when we learn that the property is dynamic.

> Source/JavaScriptCore/runtime/JSFunction.cpp:458
> +            slot.disableCaching();

I think it's better to put the disableCaching() call right at the top, when we learn that the property is dynamic.

[Mark Lam]

Created attachment 293078 [details]
Patch for landing.

[Mark Lam]

Thanks for the review.  I've moved the calls to slot.disableCaching() earlier as suggested and written a more detailed ChangeLog based on the 2 points you brought up.

[Mark Lam]

Landed in r208018: <http://trac.webkit.org/r208018>.