Bug 180190 – [DFG][FTL] operationHasIndexedProperty does not consider negative int32_t

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Reported by @azu_re. https://twitter.com/azu_re/status/936187554235170816

var k = -1;  // k >= 0 is no problem, k < 0 is problem
var o = {};
o[k] = true;

function f() {
    // `(k + "") in o` is no problem
    if (k in o) {
        return;
    }
    // `k in o` must be true, but Safari 11 reach here
    throw new Error("Never reach");
}
noInline(f);

var i = 0;
try {
    for (i = 0; i < 100000; i++) {
        f();
    }
} catch (e) {
    // Reach here is problem
    // Safari on iOS 11.1.2
    // i: 5000~20000
    // Safari 11.0.1 (12604.3.5.1.1) on mac sierra
    // i: 1000~5000
    print(e.message + " : " + JSON.stringify({
        "i": i,
        "k in o": (k in o),
        "(''+k) in o": (('' + k) in o),
        "o.hasOwnProperty(k)": o.hasOwnProperty(k)
    }, null, 2));
}

print("Done");

[Yusuke Suzuki]

Created attachment 327969 [details]
Patch

WIP: add more tests

[Yusuke Suzuki]

Created attachment 327972 [details]
Patch

[Mark Lam]

Comment on attachment 327972 [details]
Patch

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

r=me.  Is it possible to defer landing this until after https://bugs.webkit.org/show_bug.cgi?id=180192 is fixed?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:1817
> +        // Go the slowest way possible becase negative indices don't use indexed storage.

typo: /becase/because/

[Yusuke Suzuki]

Comment on attachment 327972 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:22
> +        While fixing this bug, we found that our op_in does not record OutOfBound feedback.
> +        This causes repeated OSR exit and significantly regresses the performance. We opened
> +        a bug to track this issue[1].
> +
> +        [1]: https://bugs.webkit.org/show_bug.cgi?id=180192

Oops, sorry for my misleading comment. This patch does not introduce the above performance regression.
Our ToT already has the above performance issue.

Why I noted the above thing is that I found this issue when adding a test in this patch.
The test in this patch shows the above inefficient behavior.

So, I think landing this patch is OK since this patch's issue is not related to the above perf issue.
And this patch itself does not cause/fix the above performance issue. This patch's intent is just fixing negative int case.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:1817
>> +        // Go the slowest way possible becase negative indices don't use indexed storage.
> 
> typo: /becase/because/

Thanks, fixed.

[Yusuke Suzuki]

Committed r225342: <https://trac.webkit.org/changeset/225342>

[Radar WebKit Bug Importer]

<rdar://problem/35779895>