Bug 164081

Summary: JSFunction::put() should not allow caching of lazily reified properties.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, jfbastien, keith_miller, msaboff, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
ggaren: review+
Patch for landing. none

Description Mark Lam 2016-10-27 14:34:56 PDT
Patch coming.
Comment 1 Mark Lam 2016-10-27 14:35:57 PDT
<rdar://problem/28958291>
Comment 2 Mark Lam 2016-10-27 15:02:25 PDT
Created attachment 293066 [details]
proposed patch.
Comment 3 Geoffrey Garen 2016-10-27 15:30:04 PDT
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.
Comment 4 Mark Lam 2016-10-27 16:45:45 PDT
Created attachment 293078 [details]
Patch for landing.
Comment 5 Mark Lam 2016-10-27 16:47:24 PDT
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.
Comment 6 Mark Lam 2016-10-27 16:50:38 PDT
Landed in r208018: <http://trac.webkit.org/r208018>.