Bug 197311

Summary: tryCachePutByID should not crash if target offset changes
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews213 for win-future none

Description Tadeu Zagallo 2019-04-26 08:47:46 PDT
<rdar://problem/48033612>
Comment 1 Tadeu Zagallo 2019-04-26 08:52:18 PDT
Created attachment 368318 [details]
Patch
Comment 2 Filip Pizlo 2019-04-26 09:20:19 PDT
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.
Comment 3 Tadeu Zagallo 2019-04-27 09:46:56 PDT
Created attachment 368401 [details]
Patch
Comment 4 Tadeu Zagallo 2019-04-27 09:52:07 PDT
(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...
Comment 5 Tadeu Zagallo 2019-04-27 10:23:34 PDT
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.
Comment 6 Filip Pizlo 2019-04-27 11:55:51 PDT
(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.
Comment 7 Tadeu Zagallo 2019-04-27 14:42:53 PDT
(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.
Comment 8 Tadeu Zagallo 2019-05-06 03:00:50 PDT
Created attachment 369117 [details]
Patch
Comment 9 EWS Watchlist 2019-05-06 09:52:25 PDT
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
Comment 10 EWS Watchlist 2019-05-06 09:52:27 PDT
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
Comment 11 WebKit Commit Bot 2019-05-07 11:39:33 PDT
Comment on attachment 369117 [details]
Patch

Clearing flags on attachment: 369117

Committed r245018: <https://trac.webkit.org/changeset/245018>
Comment 12 WebKit Commit Bot 2019-05-07 11:39:35 PDT
All reviewed patches have been landed.  Closing bug.