Bug 185601 - [JSC] Check TypeInfo first before calling getCallData when we would like to check whether given object is a function
Summary: [JSC] Check TypeInfo first before calling getCallData when we would like to c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-13 22:25 PDT by Yusuke Suzuki
Modified: 2018-05-16 00:06 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.79 KB, patch)
2018-05-13 22:29 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.43 MB, application/zip)
2018-05-13 23:32 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.93 MB, application/zip)
2018-05-13 23:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.32 MB, application/zip)
2018-05-13 23:49 PDT, EWS Watchlist
no flags Details
Patch (3.49 KB, patch)
2018-05-14 00:09 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (43.44 KB, patch)
2018-05-14 09:59 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (62.66 KB, patch)
2018-05-14 10:51 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (62.66 KB, patch)
2018-05-14 11:02 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (62.73 KB, patch)
2018-05-14 11:13 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (64.68 KB, patch)
2018-05-14 20:49 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-05-13 22:25:19 PDT
[JSC] Check TypeOfShouldCallGetCallData before calling getCallData when we would like to check whether a given object is callable
Comment 1 Yusuke Suzuki 2018-05-13 22:29:06 PDT
Created attachment 340290 [details]
Patch
Comment 2 EWS Watchlist 2018-05-13 23:32:38 PDT
Comment on attachment 340290 [details]
Patch

Attachment 340290 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7674112

New failing tests:
fast/storage/serialized-script-value.html
js/typeof-constant-string.html
svg/carto.net/window.svg
js/JSON-stringify-replacer.html
ietestcenter/Javascript/15.12.3-11-14.html
fast/canvas/webgl/context-creation-attributes.html
js/dom/JSON-stringify.html
Comment 3 EWS Watchlist 2018-05-13 23:32:39 PDT
Created attachment 340291 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 4 EWS Watchlist 2018-05-13 23:40:11 PDT
Comment on attachment 340290 [details]
Patch

Attachment 340290 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7674129

New failing tests:
imported/w3c/web-platform-tests/webrtc/RTCRtpTransceiver-setCodecPreferences.html
fast/storage/serialized-script-value.html
js/typeof-constant-string.html
svg/carto.net/window.svg
js/JSON-stringify-replacer.html
ietestcenter/Javascript/15.12.3-11-14.html
js/dom/JSON-stringify.html
fast/canvas/webgl/context-creation-attributes.html
http/tests/workers/service/WorkerNavigator_serviceWorker.html
Comment 5 EWS Watchlist 2018-05-13 23:40:12 PDT
Created attachment 340292 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 6 EWS Watchlist 2018-05-13 23:43:31 PDT
Comment on attachment 340290 [details]
Patch

Attachment 340290 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7674082

New failing tests:
microbenchmarks/is-object-or-null-trickier-function.js.ftl-no-cjit-no-inline-validate
microbenchmarks/is-object-or-null-trickier-function.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/is-object-or-null-tricky-function.js.no-llint
stress/object-literal-methods.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/is-object-or-null-fold-functions.js.no-cjit-validate-phases
microbenchmarks/is-object-or-null-tricky-function.js.no-cjit-validate-phases
stress/array-copywithin.js.dfg-eager-no-cjit-validate
microbenchmarks/is-object-or-null-fold-functions.js.ftl-no-cjit-small-pool
stress/static-getter-descriptors.js.ftl-eager
ChakraCore.yaml/ChakraCore/test/JSON/jx2.js.default
stress/static-getter-descriptors.js.ftl-no-cjit-b3o1
stress/array-copywithin.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/typeof-constant-string.js.layout-no-ftl
stress/type-of-functions-and-objects.js.ftl-no-cjit-no-inline-validate
stress/reflect-get-own-property.js.no-ftl
stress/static-getter-descriptors.js.ftl-no-cjit-no-put-stack-validate
stress/type-of-functions-and-objects.js.no-cjit-collect-continuously
jsc-layout-tests.yaml/js/script-tests/JSON-stringify-replacer.js.layout-no-llint
stress/json-stringify-with-non-jsarray-array.js.ftl-eager
microbenchmarks/is-object-or-null-tricky-function.js.dfg-eager-no-cjit-validate
stress/reflect-get-own-property.js.default
jsc-layout-tests.yaml/js/script-tests/JSON-stringify-replacer.js.layout-no-cjit
microbenchmarks/is-object-or-null-trickier-function.js.no-cjit-validate-phases
stress/object-literal-methods.js.ftl-no-cjit-no-inline-validate
microbenchmarks/is-object-or-null-tricky-function.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/JSON-stringify-replacer.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/typeof-constant-string.js.layout-no-llint
stress/reflect-get-own-property.js.dfg-eager
stress/type-of-functions-and-objects.js.default
microbenchmarks/is-object-or-null-fold-functions.js.dfg-eager
stress/type-of-functions-and-objects.js.ftl-no-cjit-small-pool
stress/static-getter-descriptors.js.ftl-eager-no-cjit
stress/array-copywithin.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/is-object-or-null-trickier-function.js.ftl-eager
microbenchmarks/is-object-or-null-trickier-function.js.ftl-eager-no-cjit-b3o1
stress/type-of-functions-and-objects.js.ftl-eager
stress/type-of-functions-and-objects.js.ftl-no-cjit-b3o1
stress/json-stringify-with-non-jsarray-array.js.ftl-eager-no-cjit
stress/object-literal-methods.js.default
stress/reflect-get-own-property.js.dfg-eager-no-cjit-validate
microbenchmarks/is-object-or-null-tricky-function.js.ftl-eager-no-cjit-b3o1
stress/static-getter-descriptors.js.default
microbenchmarks/is-object-or-null-tricky-function.js.ftl-eager-no-cjit
stress/object-literal-methods.js.dfg-maximal-flush-validate-no-cjit
stress/static-getter-descriptors.js.dfg-eager-no-cjit-validate
microbenchmarks/is-object-or-null-fold-functions.js.no-llint
stress/static-getter-descriptors.js.dfg-eager
microbenchmarks/is-object-or-null-tricky-function.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/is-object-or-null-fold-functions.js.no-ftl
microbenchmarks/is-object-or-null-fold-functions.js.ftl-no-cjit-validate-sampling-profiler
stress/json-stringify-with-non-jsarray-array.js.dfg-eager
jsc-layout-tests.yaml/js/script-tests/typeof-constant-string.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/typeof-constant-string.js.layout-ftl-eager-no-cjit
stress/array-copywithin.js.ftl-eager-no-cjit
microbenchmarks/is-object-or-null-trickier-function.js.dfg-eager-no-cjit-validate
stress/type-of-functions-and-objects.js.dfg-maximal-flush-validate-no-cjit
stress/type-of-functions-and-objects.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/is-object-or-null-tricky-function.js.dfg-eager
stress/json-stringify-with-non-jsarray-array.js.dfg-eager-no-cjit-validate
stress/json-stringify-with-non-jsarray-array.js.no-cjit-validate-phases
stress/array-copywithin.js.ftl-no-cjit-no-put-stack-validate
ChakraCore.yaml/ChakraCore/test/Basics/typeofcombi.js.default
microbenchmarks/is-object-or-null-fold-functions.js.ftl-eager-no-cjit-b3o1
microbenchmarks/is-object-or-null-fold-functions.js.default
stress/reflect-get-own-property.js.ftl-no-cjit-small-pool
stress/object-literal-methods.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/typeof-constant-string.js.layout-dfg-eager-no-cjit
stress/json-stringify-with-non-jsarray-array.js.ftl-no-cjit-small-pool
stress/array-copywithin.js.ftl-no-cjit-validate-sampling-profiler
stress/array-copywithin.js.ftl-no-cjit-small-pool
stress/type-of-functions-and-objects.js.ftl-no-cjit-validate-sampling-profiler
stress/array-copywithin.js.no-cjit-validate-phases
stress/object-literal-methods.js.dfg-eager
microbenchmarks/is-object-or-null-fold-functions.js.ftl-eager
microbenchmarks/is-object-or-null-trickier-function.js.ftl-no-cjit-b3o1
microbenchmarks/is-object-or-null-tricky-function.js.no-cjit-collect-continuously
microbenchmarks/is-object-or-null-fold-functions.js.ftl-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/typeof-constant-string.js.layout-no-cjit
microbenchmarks/is-object-or-null-trickier-function.js.no-llint
stress/reflect-get-own-property.js.no-cjit-validate-phases
microbenchmarks/is-object-or-null-trickier-function.js.default
stress/object-literal-methods.js.ftl-no-cjit-no-put-stack-validate
stress/type-of-functions-and-objects.js.no-cjit-validate-phases
stress/json-stringify-with-non-jsarray-array.js.no-llint
microbenchmarks/is-object-or-null-tricky-function.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/is-object-or-null-tricky-function.js.default
stress/object-literal-methods.js.ftl-eager
microbenchmarks/is-object-or-null-fold-functions.js.no-cjit-collect-continuously
stress/object-literal-methods.js.dfg-eager-no-cjit-validate
microbenchmarks/is-object-or-null-trickier-function.js.ftl-no-cjit-validate-sampling-profiler
stress/reflect-get-own-property.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/JSON-stringify-replacer.js.layout-ftl-eager-no-cjit
stress/object-literal-methods.js.ftl-eager-no-cjit-b3o1
stress/array-copywithin.js.no-ftl
stress/reflect-get-own-property.js.dfg-maximal-flush-validate-no-cjit
stress/type-of-functions-and-objects.js.ftl-eager-no-cjit-b3o1
stress/array-copywithin.js.ftl-eager
microbenchmarks/is-object-or-null-fold-functions.js.dfg-maximal-flush-validate-no-cjit
stress/json-stringify-with-non-jsarray-array.js.ftl-no-cjit-no-inline-validate
microbenchmarks/is-object-or-null-fold-functions.js.ftl-no-cjit-no-put-stack-validate
stress/object-literal-methods.js.no-llint
stress/static-getter-descriptors.js.no-ftl
microbenchmarks/is-object-or-null-trickier-function.js.no-cjit-collect-continuously
jsc-layout-tests.yaml/js/script-tests/JSON-stringify-replacer.js.layout-dfg-eager-no-cjit
stress/static-getter-descriptors.js.ftl-eager-no-cjit-b3o1
microbenchmarks/is-object-or-null-fold-functions.js.dfg-eager-no-cjit-validate
microbenchmarks/is-object-or-null-tricky-function.js.no-ftl
jsc-layout-tests.yaml/js/script-tests/JSON-stringify-replacer.js.layout-no-ftl
microbenchmarks/is-object-or-null-trickier-function.js.ftl-eager-no-cjit
stress/array-copywithin.js.no-llint
stress/array-copywithin.js.dfg-eager
stress/json-stringify-with-non-jsarray-array.js.ftl-no-cjit-no-put-stack-validate
stress/array-copywithin.js.default
stress/reflect-get-own-property.js.ftl-no-cjit-validate-sampling-profiler
stress/static-getter-descriptors.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/is-object-or-null-trickier-function.js.no-ftl
microbenchmarks/is-object-or-null-tricky-function.js.ftl-no-cjit-no-inline-validate
stress/object-literal-methods.js.no-ftl
stress/array-copywithin.js.ftl-no-cjit-b3o1
stress/reflect-get-own-property.js.ftl-eager-no-cjit-b3o1
stress/object-literal-methods.js.no-cjit-collect-continuously
microbenchmarks/is-object-or-null-tricky-function.js.ftl-eager
stress/reflect-get-own-property.js.ftl-no-cjit-no-inline-validate
microbenchmarks/is-object-or-null-tricky-function.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/is-object-or-null-fold-functions.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/JSON-stringify-replacer.js.layout
stress/static-getter-descriptors.js.no-cjit-collect-continuously
stress/json-stringify-with-non-jsarray-array.js.ftl-no-cjit-validate-sampling-profiler
stress/reflect-get-own-property.js.ftl-no-cjit-b3o1
stress/array-copywithin.js.no-cjit-collect-continuously
stress/json-stringify-with-non-jsarray-array.js.default
stress/object-literal-methods.js.ftl-no-cjit-small-pool
microbenchmarks/is-object-or-null-tricky-function.js.ftl-no-cjit-b3o1
microbenchmarks/is-object-or-null-trickier-function.js.ftl-no-cjit-no-put-stack-validate
stress/object-literal-methods.js.no-cjit-validate-phases
stress/reflect-get-own-property.js.ftl-no-cjit-no-put-stack-validate
stress/type-of-functions-and-objects.js.dfg-eager
stress/reflect-get-own-property.js.no-llint
jsc-layout-tests.yaml/js/script-tests/typeof-constant-string.js.layout
stress/json-stringify-with-non-jsarray-array.js.no-cjit-collect-continuously
stress/type-of-functions-and-objects.js.dfg-eager-no-cjit-validate
stress/reflect-get-own-property.js.no-cjit-collect-continuously
stress/object-literal-methods.js.ftl-no-cjit-b3o1
stress/static-getter-descriptors.js.ftl-no-cjit-small-pool
stress/static-getter-descriptors.js.no-llint
stress/type-of-functions-and-objects.js.no-llint
stress/json-stringify-with-non-jsarray-array.js.ftl-no-cjit-b3o1
stress/static-getter-descriptors.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/is-object-or-null-fold-functions.js.ftl-no-cjit-no-inline-validate
stress/static-getter-descriptors.js.no-cjit-validate-phases
stress/json-stringify-with-non-jsarray-array.js.dfg-maximal-flush-validate-no-cjit
stress/static-getter-descriptors.js.ftl-no-cjit-no-inline-validate
stress/json-stringify-with-non-jsarray-array.js.no-ftl
stress/type-of-functions-and-objects.js.no-ftl
stress/reflect-get-own-property.js.ftl-eager
microbenchmarks/is-object-or-null-trickier-function.js.dfg-eager
microbenchmarks/is-object-or-null-trickier-function.js.ftl-no-cjit-small-pool
stress/type-of-functions-and-objects.js.ftl-eager-no-cjit
stress/json-stringify-with-non-jsarray-array.js.ftl-eager-no-cjit-b3o1
stress/array-copywithin.js.ftl-no-cjit-no-inline-validate
apiTests
Comment 7 EWS Watchlist 2018-05-13 23:49:27 PDT
Comment on attachment 340290 [details]
Patch

Attachment 340290 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7674079

New failing tests:
fast/storage/serialized-script-value.html
js/typeof-constant-string.html
svg/carto.net/window.svg
js/JSON-stringify-replacer.html
ietestcenter/Javascript/15.12.3-11-14.html
fast/canvas/webgl/context-creation-attributes.html
js/dom/JSON-stringify.html
Comment 8 EWS Watchlist 2018-05-13 23:49:29 PDT
Created attachment 340294 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 Yusuke Suzuki 2018-05-14 00:09:50 PDT
Created attachment 340295 [details]
Patch
Comment 10 EWS Watchlist 2018-05-14 01:26:13 PDT
Comment on attachment 340295 [details]
Patch

Attachment 340295 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7674678

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-cjit
apiTests
Comment 11 Saam Barati 2018-05-14 08:58:43 PDT
Comment on attachment 340295 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSONObject.cpp:385
> +    if (object->inherits<JSFunction>(vm)

Just doing this optimization for JSFunction inside of JSON code feels like the wrong level of abstraction. I’d advocate for writing a new helper for getCallSata and you can put optimization’s in that, and switch to using that helper here (and maybe elsewhere)
Comment 12 Yusuke Suzuki 2018-05-14 09:33:25 PDT
Comment on attachment 340295 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSONObject.cpp:385
>> +    if (object->inherits<JSFunction>(vm)
> 
> Just doing this optimization for JSFunction inside of JSON code feels like the wrong level of abstraction. I’d advocate for writing a new helper for getCallSata and you can put optimization’s in that, and switch to using that helper here (and maybe elsewhere)

OK, I'll refactor JSValue::isFunction/JSValue::isCallable to be efficient.
Comment 13 Yusuke Suzuki 2018-05-14 09:59:47 PDT
Created attachment 340323 [details]
Patch
Comment 14 Yusuke Suzuki 2018-05-14 10:10:42 PDT
Comment on attachment 340323 [details]
Patch

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

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1618
>      FunctionExecutable* executable = (jsDynamicCast<JSFunction*>(vm, functionValue.asCell()->getObject()))->jsExecutable();

Ditto.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1637
>      FunctionExecutable* executable = (jsDynamicCast<JSFunction*>(vm, functionValue.asCell()->getObject()))->jsExecutable();

Ditto.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1659
>      FunctionExecutable* executable = (jsDynamicCast<JSFunction*>(vm, functionValue.asCell()->getObject()))->jsExecutable();

Ditto.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:1678
>      FunctionExecutable* executable = (jsDynamicCast<JSFunction*>(vm, functionValue.asCell()->getObject()))->jsExecutable();

Ditto.

> Source/WebCore/testing/Internals.cpp:2044
> +    } else if (code.isFunction(vm)) {

I think this is incorrect: Even if `code.isFunction()` says "yeah, this is a function", it does not mean that this is JSFunction. (e.g. InternalFunction exists).
I would like to fix these things in a separate patch.
Comment 15 Saam Barati 2018-05-14 10:17:44 PDT
Comment on attachment 340323 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] Check TypeOfShouldCallGetCallData before calling getCallData when we would like to check whether a given object is callable

I would make the name of this line up with your functions:
"given object is callable" => "given object is a function"
Comment 16 Saam Barati 2018-05-14 10:19:15 PDT
Comment on attachment 340323 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:16
> +        To do this cleanly, we refactor JSValue::{isFunction,isCallable}. We add JSCell::{isFunction,isCallable}
> +        and JSValue ones call into these functions. Inside JSCell::{isFunction,isCallable}, we perform
> +        TypeOfShouldCallGetCallData checking before calling getCallData.

Hold on, this doesn't seem quite right. I don't think being callable should depend on the TypeOfShouldCallGetCallData being set.
Comment 17 Saam Barati 2018-05-14 10:20:04 PDT
Comment on attachment 340323 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:16
>> +        TypeOfShouldCallGetCallData checking before calling getCallData.
> 
> Hold on, this doesn't seem quite right. I don't think being callable should depend on the TypeOfShouldCallGetCallData being set.

If this is true, this flag needs a new name. You could also conditionally not call it if it's JSCell's implementation of getCallData.
Comment 18 Yusuke Suzuki 2018-05-14 10:25:59 PDT
Comment on attachment 340323 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:3
>> +        [JSC] Check TypeOfShouldCallGetCallData before calling getCallData when we would like to check whether a given object is callable
> 
> I would make the name of this line up with your functions:
> "given object is callable" => "given object is a function"

Sounds fine, fixed.

>>> Source/JavaScriptCore/ChangeLog:16
>>> +        TypeOfShouldCallGetCallData checking before calling getCallData.
>> 
>> Hold on, this doesn't seem quite right. I don't think being callable should depend on the TypeOfShouldCallGetCallData being set.
> 
> If this is true, this flag needs a new name. You could also conditionally not call it if it's JSCell's implementation of getCallData.

I think this condition should be kept since [[Callable]] interface in ECMAScript should return "function" for typeof.
So, requiring this flag if the object could implement getCallData is sane (except for JSFunction).
So, maybe, renaming this flag would be better. Like, "MayBeCallable". And setting this to JSFunction too would be fine (but even though, JSFunctionType branch in the current code is good for optimization).
Comment 19 Yusuke Suzuki 2018-05-14 10:40:08 PDT
Comment on attachment 340323 [details]
Patch

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

>>>> Source/JavaScriptCore/ChangeLog:16
>>>> +        TypeOfShouldCallGetCallData checking before calling getCallData.
>>> 
>>> Hold on, this doesn't seem quite right. I don't think being callable should depend on the TypeOfShouldCallGetCallData being set.
>> 
>> If this is true, this flag needs a new name. You could also conditionally not call it if it's JSCell's implementation of getCallData.
> 
> I think this condition should be kept since [[Callable]] interface in ECMAScript should return "function" for typeof.
> So, requiring this flag if the object could implement getCallData is sane (except for JSFunction).
> So, maybe, renaming this flag would be better. Like, "MayBeCallable". And setting this to JSFunction too would be fine (but even though, JSFunctionType branch in the current code is good for optimization).

I'm not sure whether "conditionally not call it if it's JSCell's implementation of getCallData" is useful b/c basically these cases are filtered out by this TypeInfo checking.
Comment 20 Yusuke Suzuki 2018-05-14 10:47:13 PDT
Comment on attachment 340323 [details]
Patch

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

>>>>> Source/JavaScriptCore/ChangeLog:16
>>>>> +        TypeOfShouldCallGetCallData checking before calling getCallData.
>>>> 
>>>> Hold on, this doesn't seem quite right. I don't think being callable should depend on the TypeOfShouldCallGetCallData being set.
>>> 
>>> If this is true, this flag needs a new name. You could also conditionally not call it if it's JSCell's implementation of getCallData.
>> 
>> I think this condition should be kept since [[Callable]] interface in ECMAScript should return "function" for typeof.
>> So, requiring this flag if the object could implement getCallData is sane (except for JSFunction).
>> So, maybe, renaming this flag would be better. Like, "MayBeCallable". And setting this to JSFunction too would be fine (but even though, JSFunctionType branch in the current code is good for optimization).
> 
> I'm not sure whether "conditionally not call it if it's JSCell's implementation of getCallData" is useful b/c basically these cases are filtered out by this TypeInfo checking.

Or "OverridesGetCallData" sounds fine. Fixed.
Comment 21 Yusuke Suzuki 2018-05-14 10:51:12 PDT
Created attachment 340329 [details]
Patch
Comment 22 EWS Watchlist 2018-05-14 10:56:49 PDT
Comment on attachment 340329 [details]
Patch

Attachment 340329 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/7679208

New failing tests:
(JS) JSTestObj.h
(JS) JSTestPluginInterface.h
Comment 23 Yusuke Suzuki 2018-05-14 11:02:19 PDT
Created attachment 340331 [details]
Patch
Comment 24 Yusuke Suzuki 2018-05-14 11:13:27 PDT
Created attachment 340333 [details]
Patch
Comment 25 Saam Barati 2018-05-14 14:38:20 PDT
Comment on attachment 340333 [details]
Patch

Can you add asserts inside Structure constructor that getCallData is either JSCell's getCallData or the OverridesGetCallData bit is set.
Comment 26 Yusuke Suzuki 2018-05-14 20:46:01 PDT
(In reply to Saam Barati from comment #25)
> Comment on attachment 340333 [details]
> Patch
> 
> Can you add asserts inside Structure constructor that getCallData is either
> JSCell's getCallData or the OverridesGetCallData bit is set.

Fine, added.
Comment 27 Yusuke Suzuki 2018-05-14 20:49:15 PDT
Created attachment 340393 [details]
Patch
Comment 28 Saam Barati 2018-05-15 12:32:28 PDT
Comment on attachment 340393 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Rename TypeOfShouldCallGetCallData to OverridesGetCallData. And check TypeOfShouldCallGetCallData

And check TypeOfShouldCallGetCallData => And check OverridesGetCallData

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:129
> +            if (auto* classInfo = object->classInfo(vm)) {

I know this isn't your code, but it's weird that this could be null. I wonder why this was added.
Comment 29 Yusuke Suzuki 2018-05-16 00:02:22 PDT
Comment on attachment 340393 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Rename TypeOfShouldCallGetCallData to OverridesGetCallData. And check TypeOfShouldCallGetCallData
> 
> And check TypeOfShouldCallGetCallData => And check OverridesGetCallData

Thanks, fixed.

>> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:129
>> +            if (auto* classInfo = object->classInfo(vm)) {
> 
> I know this isn't your code, but it's weird that this could be null. I wonder why this was added.

Ah, nice. Fixed.
Comment 30 Yusuke Suzuki 2018-05-16 00:05:35 PDT
Committed r231839: <https://trac.webkit.org/changeset/231839>
Comment 31 Radar WebKit Bug Importer 2018-05-16 00:06:34 PDT
<rdar://problem/40286270>