[JSC] Check TypeOfShouldCallGetCallData before calling getCallData when we would like to check whether a given object is callable
Created attachment 340290 [details] Patch
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
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 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
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 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 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
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
Created attachment 340295 [details] Patch
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 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 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.
Created attachment 340323 [details] Patch
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 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 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 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 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 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 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.
Created attachment 340329 [details] Patch
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
Created attachment 340331 [details] Patch
Created attachment 340333 [details] Patch
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.
(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.
Created attachment 340393 [details] Patch
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 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.
Committed r231839: <https://trac.webkit.org/changeset/231839>
<rdar://problem/40286270>