Bug 197311 – tryCachePutByID should not crash if target offset changes

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

<rdar://problem/48033612>

[Tadeu Zagallo]

Created attachment 368318 [details]
Patch

[Filip Pizlo]

Comment on attachment 368318 [details]
Patch

Did you test what happens if you change the property to no longer be a setter?  I'm worried that an attribute change can happen.  I think you need to check the attributes of the slotBaseCondition() to ensure that it's still a setter.

Generally, I'm worried about the fact that generateConditionsForBlahBlah has a weird contract with the JSObject virtual methods like getOwnProperty and etc: generateConditionsForBlahBlah is only correct if it is called right after a JSObject get or put that returns a cacheable PropertySlot or PutPropertySlot. In other words, if some JSObject subclass wants to prevent this logic from working, they only need to ensure that their overrides return an uncacheable PropertySlot.

The trouble with this is that it assumes that the only effects that can happen are those that will not lead some JSObject subclass to change its mind about whether or not this property should get intercepted.  I think that this is mostly OK because JSObject::putInlineSlow is responsible for doing the prototype walk itself - it's not like each object in the prototype chain can override behavior.  For the purpose of the prototype walk that put() does, the only way to affect behavior is to change what putInlineSlow does.

Looking at that function, I see two possible problems:

1) The !obj->staticPropertiesReified(vm) check.  Say that the setter changes the proto chain so that it still bottoms out on the same object (like, previously you had O->Q->P and now you have O->R->P) but has some extra object in the middle (like R) and that object has static properties.  generateConditionsForBlahBlah won't catch that.  Not sure if that's a problem or not.

2) The obj->type() == ProxyObjectType check.  I think that this is OK because generateCondition rejects proxies.

So, I think you might still have a subtle bug in the static properties case.

[Tadeu Zagallo]

Created attachment 368401 [details]
Patch

[Tadeu Zagallo]

(In reply to Filip Pizlo from comment #2)
> Did you test what happens if you change the property to no longer be a
> setter?  I'm worried that an attribute change can happen.  I think you need
> to check the attributes of the slotBaseCondition() to ensure that it's still
> a setter.

You were correct. I had tested, but I had to increase the number of iterations in the loop to actually generate the code, which lead to the crash. I added the check and an extra test for that.
 
> So, I think you might still have a subtle bug in the static properties case.

I'm still trying to find a repro case for this one...

[Tadeu Zagallo]

FWIW, here's what I tried, but got the expected behavior. Does this cover the static properties case you mentioned?

```
let counter = 0;
class A {
    static set y(x) {
        if (counter++ === 9) {
            print('A');
            this.__proto__ = B2;
        }
    }
}

class B1 extends A {
}

class B2 extends A {
    static set y(x) {
        print('B2');
    }
}

class C extends B1 {
}


for (let i = 0; i < 11; i++)
    C.y = 42;
```

I also tried making B2.y a value instead of a setter and got the expected result.

[Filip Pizlo]

(In reply to Tadeu Zagallo from comment #5)
> FWIW, here's what I tried, but got the expected behavior. Does this cover
> the static properties case you mentioned?
> 
> ```
> let counter = 0;
> class A {
>     static set y(x) {
>         if (counter++ === 9) {
>             print('A');
>             this.__proto__ = B2;

Maybe you need to do the flatten thing here, after setting __proto__. 

Also, this changes the structure of this. I think that I was wrong to suggest turning o->q->p into o->r->p. You need one more indirection maybe: o->n->q->p into o->n->r->p so o’s struct doesn’t change. 

>         }
>     }
> }
> 
> class B1 extends A {
> }
> 
> class B2 extends A {
>     static set y(x) {
>         print('B2');
>     }
> }
> 
> class C extends B1 {
> }
> 
> 
> for (let i = 0; i < 11; i++)
>     C.y = 42;
> ```
> 
> I also tried making B2.y a value instead of a setter and got the expected
> result.

[Tadeu Zagallo]

(In reply to Filip Pizlo from comment #6)
> Maybe you need to do the flatten thing here, after setting __proto__. 

I tried flattening `this`, but it was not a dictionary.

> Also, this changes the structure of this. I think that I was wrong to
> suggest turning o->q->p into o->r->p. You need one more indirection maybe:
> o->n->q->p into o->n->r->p so o’s struct doesn’t change. 

Also tried adding the extra level, but it seem to be caught by `generateConditionsForPrototypePropertyHit`. We check for the absence of `y` through the whole chain, and it fails when it finds it in B2.

[Tadeu Zagallo]

Created attachment 369117 [details]
Patch

[EWS Watchlist]

Comment on attachment 369117 [details]
Patch

Attachment 369117 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12111816

New failing tests:
http/tests/css/filters-on-iframes.html

[EWS Watchlist]

Created attachment 369129 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 369117 [details]
Patch

Clearing flags on attachment: 369117

Committed r245018: <https://trac.webkit.org/changeset/245018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.