Bug 197311 - tryCachePutByID should not crash if target offset changes
Summary: tryCachePutByID should not crash if target offset changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-26 08:47 PDT by Tadeu Zagallo
Modified: 2019-05-07 11:39 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.37 KB, patch)
2019-04-26 08:52 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (4.41 KB, patch)
2019-04-27 09:46 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (9.22 KB, patch)
2019-05-06 03:00 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.70 MB, application/zip)
2019-05-06 09:52 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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 Build Bot 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.