Bug 209777

Summary: Delete IC incorrectly caches for proxies
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Justin Michaud <justin>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, fpizlo, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 209870    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Mark Lam 2020-03-30 14:49:20 PDT
We're seeing a ASSERTION FAILED: m_cases[i - 1] < m_cases[I] due to DeleteIC.  Here's the test case:

    function foo() {
        let j = 0;
        while (j++ < 2)
            delete this.x;
        Object.defineProperty(this, "x", {});
    }

    for (let i = 0; i < 5; i++)
        foo();

Run with --jitPolicyScale=0 --useDFGJIT=0 --useConcurrentJIT=0.

The duplicate cases are:
DeleteMiss:(Committed, ident = 'uid:(x)', structure = 0x10c4faee0:[0xf4c5, JSProxy, {}, NonArray, Proto:0x106dfa368, Leaf])
DeleteNonConfigurable:(Committed, ident = 'uid:(x)', structure = 0x10c4faee0:[0xf4c5, JSProxy, {}, NonArray, Proto:0x106dfa368, Leaf])

<rdar://problem/61051902>
Comment 1 Justin Michaud 2020-03-31 10:16:38 PDT
Created attachment 395061 [details]
Patch
Comment 2 Mark Lam 2020-03-31 10:21:42 PDT
Comment on attachment 395061 [details]
Patch

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

r=me

> JSTests/ChangeLog:8
> +        * stress/delete-property-ic-compatable.js: Added.

Can you also add variants of this test for the other proxy types?

> JSTests/stress/delete-property-ic-compatable.js:1
> +//@ requireOptions("--jitPolicyScale=0", "--useDFGJIT=0", "--useConcurrentJIT=0")

I think you can drop the "--useConcurrentJIT=0".  There will be a test configuration that handles that already.
Comment 3 Justin Michaud 2020-03-31 11:00:15 PDT
Created attachment 395069 [details]
Patch
Comment 4 Mark Lam 2020-03-31 11:07:33 PDT
Comment on attachment 395069 [details]
Patch

Please fix test name /compatable/compatible/.  Please also update the bug title in the ChangeLog since it has changed in bugzilla.  r=me
Comment 5 Justin Michaud 2020-03-31 11:20:30 PDT
Created attachment 395074 [details]
Patch
Comment 6 Saam Barati 2020-03-31 15:12:04 PDT
Comment on attachment 395074 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        Proxy's do not change their structure ID when properties are added, so we cannot cache deletes
> +        for them.

why can't we cache delete on their target though (I mean w.r.t proxy like window, not like Proxy object in ES6)?
Comment 7 Justin Michaud 2020-04-01 12:00:22 PDT
(In reply to Saam Barati from comment #6)
> Comment on attachment 395074 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395074&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        Proxy's do not change their structure ID when properties are added, so we cannot cache deletes
> > +        for them.
> 
> why can't we cache delete on their target though (I mean w.r.t proxy like
> window, not like Proxy object in ES6)?

Filed a bug and related it to this bug.
Comment 8 EWS 2020-04-01 12:10:25 PDT
Committed r259357: <https://trac.webkit.org/changeset/259357>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395074 [details].