Patch coming. <rdar://problem/35116160>
Created attachment 325532 [details] proposed patch. Let's get some EWS testing on this patch.
Comment on attachment 325532 [details] proposed patch. Attachment 325532 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/5057947 New failing tests: microbenchmarks/bound-function-call.js.ftl-no-cjit-small-pool stress/tail-call-recognize.js.ftl-no-cjit-small-pool stress/sampling-profiler-bound-function-name.js.no-llint microbenchmarks/instanceof-bound.js.no-cjit-collect-continuously stress/generational-opaque-roots.js.ftl-eager-no-cjit-b3o1 stress/tail-call-recognize.js.ftl-eager-no-cjit-b3o1 stress/sampling-profiler-bound-function-name.js.ftl-no-cjit-no-inline-validate stress/async-arrow-functions-lexical-binding-in-class.js.no-cjit-collect-continuously slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-bind-no-inlining.js.default stress/tail-call-recognize.js.dfg-eager stress/call-apply-builtin-functions-dont-use-iterators.js.ftl-no-cjit-no-inline-validate stress/call-apply-builtin-functions-dont-use-iterators.js.dfg-maximal-flush-validate-no-cjit microbenchmarks/bound-function-call.js.ftl-no-cjit-no-put-stack-validate stress/generational-opaque-roots.js.no-cjit-validate-phases microbenchmarks/bound-function-call.js.ftl-no-cjit-no-inline-validate mozilla-tests.yaml/ecma_3/Date/15.9.5.5.js.mozilla-llint microbenchmarks/getter-richards-try-catch.js.no-ftl jsc-layout-tests.yaml/js/script-tests/caller-property.js.layout-no-ftl stress/tail-call-recognize.js.ftl-no-cjit-validate-sampling-profiler stress/tail-call-recognize.js.no-ftl stress/tail-call-recognize.js.ftl-no-cjit-b3o1 stress/call-apply-builtin-functions-dont-use-iterators.js.ftl-no-cjit-b3o1 stress/generational-opaque-roots.js.dfg-eager-no-cjit-validate jsc-layout-tests.yaml/js/script-tests/caller-property.js.layout-no-cjit microbenchmarks/instanceof-bound.js.ftl-eager-no-cjit microbenchmarks/instanceof-bound.js.ftl-no-cjit-small-pool stress/tail-call-recognize.js.dfg-eager-no-cjit-validate stress/sampling-profiler-bound-function-name.js.dfg-eager microbenchmarks/bound-function-call.js.no-ftl microbenchmarks/instanceof-bound.js.no-llint stress/generational-opaque-roots.js.no-llint stress/tail-call-recognize.js.no-cjit-collect-continuously stress/generational-opaque-roots.js.dfg-eager stress/call-apply-builtin-functions-dont-use-iterators.js.no-llint stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager-no-cjit-b3o1 ChakraCore.yaml/ChakraCore/test/Function/undefThis.js.default stress/async-arrow-functions-lexical-binding-in-class.js.no-cjit-validate-phases stress/sampling-profiler-bound-function-name.js.no-ftl stress/generational-opaque-roots.js.ftl-no-cjit-b3o1 stress/generational-opaque-roots.js.ftl-no-cjit-no-inline-validate microbenchmarks/instanceof-bound.js.no-cjit-validate-phases stress/generational-opaque-roots.js.default stress/generational-opaque-roots.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/bound-function-call.js.ftl-no-cjit-validate-sampling-profiler slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-bind-inlining.js.no-ftl microbenchmarks/bound-function-call.js.ftl-eager-no-cjit microbenchmarks/instanceof-bound.js.dfg-eager-no-cjit-validate stress/generational-opaque-roots.js.ftl-no-cjit-no-put-stack-validate stress/tail-call-recognize.js.dfg-maximal-flush-validate-no-cjit stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-b3o1 microbenchmarks/bound-function-call.js.no-llint microbenchmarks/getter-richards-try-catch.js.no-cjit stress/sampling-profiler-bound-function-name.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/bound-function-call.js.dfg-maximal-flush-validate-no-cjit stress/async-arrow-functions-lexical-binding-in-class.js.dfg-eager stress/tail-call-recognize.js.no-llint jsc-layout-tests.yaml/js/script-tests/caller-property.js.layout-dfg-eager-no-cjit stress/generational-opaque-roots.js.ftl-no-cjit-small-pool microbenchmarks/instanceof-bound.js.ftl-no-cjit-b3o1 jsc-layout-tests.yaml/js/script-tests/caller-property.js.layout-no-llint microbenchmarks/instanceof-bound.js.ftl-no-cjit-no-inline-validate microbenchmarks/getter-richards-try-catch.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/instanceof-bound.js.dfg-eager microbenchmarks/instanceof-bound.js.dfg-maximal-flush-validate-no-cjit stress/call-apply-builtin-functions-dont-use-iterators.js.dfg-eager stress/sampling-profiler-bound-function-name.js.ftl-no-cjit-small-pool stress/call-apply-builtin-functions-dont-use-iterators.js.ftl-eager stress/sampling-profiler-bound-function-name.js.no-cjit-validate-phases jsc-layout-tests.yaml/js/script-tests/caller-property.js.layout-ftl-eager-no-cjit microbenchmarks/instanceof-bound.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/bound-function-call.js.dfg-eager stress/sampling-profiler-bound-function-name.js.default stress/tail-call-recognize.js.ftl-no-cjit-no-inline-validate stress/call-apply-builtin-functions-dont-use-iterators.js.default stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-validate-sampling-profiler stress/sampling-profiler-bound-function-name.js.ftl-no-cjit-b3o1 jsc-layout-tests.yaml/js/script-tests/caller-property.js.layout-ftl-no-cjit stress/sampling-profiler-bound-function-name.js.dfg-maximal-flush-validate-no-cjit stress/call-apply-builtin-functions-dont-use-iterators.js.ftl-eager-no-cjit stress/generational-opaque-roots.js.ftl-eager-no-cjit microbenchmarks/bound-function-call.js.ftl-eager-no-cjit-b3o1 microbenchmarks/bound-function-call.js.dfg-eager-no-cjit-validate stress/sampling-profiler-bound-function-name.js.dfg-eager-no-cjit-validate stress/call-apply-builtin-functions-dont-use-iterators.js.no-cjit-collect-continuously slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-bind-inlining.js.no-cjit mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint stress/tail-call-recognize.js.ftl-eager-no-cjit microbenchmarks/instanceof-bound.js.ftl-eager jsc-layout-tests.yaml/js/script-tests/caller-property.js.layout stress/tail-call-recognize.js.no-cjit-validate-phases stress/sampling-profiler-bound-function-name.js.ftl-eager ChakraCore.yaml/ChakraCore/test/Function/bind.js.default stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager-no-cjit stress/call-apply-builtin-functions-dont-use-iterators.js.ftl-no-cjit-no-put-stack-validate stress/call-apply-builtin-functions-dont-use-iterators.js.ftl-no-cjit-small-pool slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-bind-no-inlining.js.no-ftl stress/sampling-profiler-bound-function-name.js.no-cjit-collect-continuously microbenchmarks/bound-function-call.js.ftl-eager slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-bind-no-inlining.js.ftl-no-cjit-validate-sampling-profiler stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-no-put-stack-validate microbenchmarks/bound-function-call.js.default stress/call-apply-builtin-functions-dont-use-iterators.js.ftl-no-cjit-validate-sampling-profiler stress/tail-call-recognize.js.ftl-eager microbenchmarks/bound-function-call.js.no-cjit-validate-phases microbenchmarks/getter-richards-try-catch.js.default stress/generational-opaque-roots.js.no-ftl stress/generational-opaque-roots.js.dfg-maximal-flush-validate-no-cjit microbenchmarks/bound-function-call.js.no-cjit-collect-continuously microbenchmarks/bound-function-call.js.ftl-no-cjit-b3o1 stress/sampling-profiler-bound-function-name.js.ftl-eager-no-cjit-b3o1 slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-bind-inlining.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/instanceof-bound.js.no-ftl stress/tail-call-recognize.js.ftl-no-cjit-no-put-stack-validate stress/sampling-profiler-bound-function-name.js.ftl-no-cjit-no-put-stack-validate stress/call-apply-builtin-functions-dont-use-iterators.js.no-cjit-validate-phases stress/call-apply-builtin-functions-dont-use-iterators.js.no-ftl microbenchmarks/instanceof-bound.js.ftl-eager-no-cjit-b3o1 stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-small-pool stress/generational-opaque-roots.js.no-cjit-collect-continuously slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-bind-no-inlining.js.no-cjit microbenchmarks/instanceof-bound.js.ftl-no-cjit-validate-sampling-profiler stress/async-arrow-functions-lexical-binding-in-class.js.dfg-eager-no-cjit-validate microbenchmarks/instanceof-bound.js.default stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-no-inline-validate stress/async-arrow-functions-lexical-binding-in-class.js.dfg-maximal-flush-validate-no-cjit stress/tail-call-recognize.js.default stress/call-apply-builtin-functions-dont-use-iterators.js.ftl-eager-no-cjit-b3o1 stress/call-apply-builtin-functions-dont-use-iterators.js.dfg-eager-no-cjit-validate slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-bind-inlining.js.default stress/generational-opaque-roots.js.ftl-eager stress/sampling-profiler-bound-function-name.js.ftl-eager-no-cjit
Comment on attachment 325532 [details] proposed patch. Attachment 325532 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5058028 Number of test failures exceeded the failure limit.
Created attachment 325536 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325532 [details] proposed patch. Attachment 325532 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5058011 Number of test failures exceeded the failure limit.
Created attachment 325537 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 325532 [details] proposed patch. Attachment 325532 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5058087 Number of test failures exceeded the failure limit.
Created attachment 325538 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 325585 [details] proposed patch. Fixed a few bugs. Let's try the EWS again.
Created attachment 325597 [details] proposed patch. More bug fixes. I think this should be it. Let's verify again with the EWS.
Comment on attachment 325597 [details] proposed patch. Attachment 325597 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5064727 New failing tests: fast/text/font-weight-fallback.html imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html fast/text/font-face-crash-2.html imported/w3c/web-platform-tests/url/urlsearchparams-foreach.html imported/w3c/web-platform-tests/fetch/api/headers/headers-errors.html fast/text/font-face-set-javascript.html
Created attachment 325612 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325597 [details] proposed patch. Attachment 325597 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5064812 New failing tests: fast/text/font-weight-fallback.html imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html fast/text/font-face-crash-2.html imported/w3c/web-platform-tests/url/urlsearchparams-foreach.html imported/w3c/web-platform-tests/fetch/api/headers/headers-errors.html fast/text/font-face-set-javascript.html
Created attachment 325614 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 325597 [details] proposed patch. Attachment 325597 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5064898 New failing tests: fast/text/font-weight-fallback.html imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html fast/text/font-face-crash-2.html imported/w3c/web-platform-tests/url/urlsearchparams-foreach.html imported/w3c/web-platform-tests/fetch/api/headers/headers-errors.html fast/text/font-face-set-javascript.html
Created attachment 325617 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325597 [details] proposed patch. Attachment 325597 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5065070 New failing tests: fast/text/font-weight-fallback.html imported/w3c/web-platform-tests/fetch/api/headers/headers-basic.html fast/text/font-face-crash-2.html imported/w3c/web-platform-tests/url/urlsearchparams-foreach.html imported/w3c/web-platform-tests/fetch/api/headers/headers-errors.html fast/text/font-face-set-javascript.html
Created attachment 325618 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 325625 [details] proposed patch. One bug snuck by me, now fixed. Let's get some EWS testing again.
Comment on attachment 325625 [details] proposed patch. The EWS is happy. Let's get a review.
Comment on attachment 325625 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=325625&action=review a couple comments. Will review more > Source/JavaScriptCore/API/JSObjectRef.cpp:189 > + if (UNLIKELY(argList.hasOverflowed())) { > + auto throwScope = DECLARE_THROW_SCOPE(vm); > + throwOutOfMemoryError(exec, throwScope); > + handleExceptionIfNeeded(scope, exec, exception); > + return 0; > + } I wonder if we should have a different API for such a workload where we know exactly how many arguments we're going to append. Something like: if (MarkedArgumentBuffer::willSizeOverflow(argumentCount)) throwOOM; > Source/JavaScriptCore/inspector/InjectedScriptManager.cpp:162 > + if (UNLIKELY(args.hasOverflowed())) > + return nullptr; This are three arguments here. Seems like this should be an assert.
Comment on attachment 325625 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=325625&action=review >> Source/JavaScriptCore/API/JSObjectRef.cpp:189 >> + } > > I wonder if we should have a different API for such a workload where we know exactly how many arguments we're going to append. Something like: > if (MarkedArgumentBuffer::willSizeOverflow(argumentCount)) throwOOM; Hmmm, I'll look into it. >> Source/JavaScriptCore/inspector/InjectedScriptManager.cpp:162 >> + return nullptr; > > This are three arguments here. Seems like this should be an assert. You're right. Will fix.
Comment on attachment 325625 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=325625&action=review > Source/JavaScriptCore/runtime/ArgList.cpp:72 > + int newCapacity = checkedNewCapacity.unsafeGet(); why give this a variable name? > Source/JavaScriptCore/runtime/ArgList.cpp:82 > + int newCapacity = checkedNewCapacity.unsafeGet(); ditto > Source/JavaScriptCore/runtime/ArgList.h:31 > +class MarkedArgumentBuffer : public RecordOverflow { Not sure if it's worth it, but we could make this more efficient just by stealing the sign bit of m_capacity instead of adding an unsigned char field.
Comment on attachment 325625 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=325625&action=review r=me I think some of the debug ASSERTs on lists with statically known sizes like 1-10 seems like noise, but it's up to you to keep them or not. I strongly vote against keeping the ASSERT when the list is never appended to. > Source/JavaScriptCore/runtime/StringPrototype.cpp:724 > + return encodedJSValue(); return { }? > Source/WebCore/ChangeLog:10 > + ridiculously long time, which renders it unsuitable for for automated tests. for for => for > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:130 > + ASSERT(!args.hasOverflowed()); this is noise IMO > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:202 > + ASSERT(!args.hasOverflowed()); this is noise IMO > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.cpp:68 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunctionRethrow.cpp:71 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunctionWithThisObject.cpp:71 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunctionWithTypedefs.cpp:72 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:177 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert or remove > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:204 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:232 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:259 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:286 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:314 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:340 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:370 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:398 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:428 > + RELEASE_ASSERT(!args.hasOverflowed()); debug assert > Source/WebCore/bindings/scripts/test/JS/JSTestVoidCallbackFunction.cpp:83 > + RELEASE_ASSERT(!args.hasOverflowed()); I vote for debug assert since it's only 6 arguments. > Source/WebCore/bridge/NP_jsobject.cpp:250 > getListFromVariantArgs(exec, args, argCount, rootObject, argList); You can make this function do the check itself so you consolidate 3 RELEASE_ASSERTs into one. > Source/WebCore/html/HTMLMediaElement.cpp:7170 > + ASSERT(!argList.hasOverflowed()); This seems like unnecessary noise. This class would have no point if this were true. you could just add this to MarkedArgumentBuffer's constructor > Source/WebCore/html/HTMLMediaElement.cpp:7210 > + ASSERT(!argList.hasOverflowed()); ditto > Source/WebCore/html/HTMLPlugInImageElement.cpp:380 > + ASSERT(!argList.hasOverflowed()); It seems like you could get rid of a bunch of asserts if you just put this assert inside call/profileCall or whatever they're called.
Comment on attachment 325625 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=325625&action=review >>> Source/JavaScriptCore/API/JSObjectRef.cpp:189 >>> + } >> >> I wonder if we should have a different API for such a workload where we know exactly how many arguments we're going to append. Something like: >> if (MarkedArgumentBuffer::willSizeOverflow(argumentCount)) throwOOM; > > Hmmm, I'll look into it. I don't feel strongly about this. What you have is probably OK. I think it's probably more cognitive overhead to have 2 ways to check for overflow.
Comment on attachment 325625 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=325625&action=review >> Source/JavaScriptCore/runtime/ArgList.cpp:72 >> + int newCapacity = checkedNewCapacity.unsafeGet(); > > why give this a variable name? The original code computed newCapacity. I can fold it into the statement below. >> Source/JavaScriptCore/runtime/ArgList.cpp:82 >> + int newCapacity = checkedNewCapacity.unsafeGet(); > > ditto Same as above. >> Source/JavaScriptCore/runtime/ArgList.h:31 >> +class MarkedArgumentBuffer : public RecordOverflow { > > Not sure if it's worth it, but we could make this more efficient just by stealing the sign bit of m_capacity instead of adding an unsigned char field. MarkedArgumentBuffer is also always used as a transient stack object. I think it's not worth it. Extending RecordOverflow also makes it clear what we're doing. >> Source/JavaScriptCore/runtime/StringPrototype.cpp:724 >> + return encodedJSValue(); > > return { }? Returning encodedJSValue() is more correct than returning { }. For 64-bit, it does the same thing, but on 32-bit it's not the same. Regardless, using encodedJSValue() is consistent with other returned values in this function. >> Source/WebCore/ChangeLog:10 >> + ridiculously long time, which renders it unsuitable for for automated tests. > > for for => for Will fix. >> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:130 >> + ASSERT(!args.hasOverflowed()); > > this is noise IMO I was debating between having it there to document the need to do overflow checks on MarkedArgumentBuffer (in most cases) vs removing it because no args were added. I chose to document in case args get added in the future. This also prevents bugs introduced where someone may copy this code but neglects to add the overflow check even when a variable number of args are appended. Sure, the assertion in the MarkedArgumentBuffer destructor will catch it if we have sufficient code coverage that exercises the new code path, but I think that the explicit documentation is more clear about the need. >> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:202 >> + ASSERT(!args.hasOverflowed()); > > this is noise IMO Explained above. >> Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.cpp:68 >> + RELEASE_ASSERT(!args.hasOverflowed()); > > debug assert Will fix. >> Source/WebCore/bindings/scripts/test/JS/JSTestCallbackInterface.cpp:177 >> + RELEASE_ASSERT(!args.hasOverflowed()); > > debug assert or remove Will use a debug assert. >> Source/WebCore/bindings/scripts/test/JS/JSTestVoidCallbackFunction.cpp:83 >> + RELEASE_ASSERT(!args.hasOverflowed()); > > I vote for debug assert since it's only 6 arguments. Will fix. >> Source/WebCore/bridge/NP_jsobject.cpp:250 >> getListFromVariantArgs(exec, args, argCount, rootObject, argList); > > You can make this function do the check itself so you consolidate 3 RELEASE_ASSERTs into one. Good idea. However, I would like to keep the assert here to document that MarkedArgumentBuffers need overflow checks. >> Source/WebCore/html/HTMLMediaElement.cpp:7170 >> + ASSERT(!argList.hasOverflowed()); > > This seems like unnecessary noise. This class would have no point if this were true. you could just add this to MarkedArgumentBuffer's constructor Explained above. Keeping for documentation. >> Source/WebCore/html/HTMLPlugInImageElement.cpp:380 >> + ASSERT(!argList.hasOverflowed()); > > It seems like you could get rid of a bunch of asserts if you just put this assert inside call/profileCall or whatever they're called. I like that it's explicit because it documents the contract that MarkedArgumentBuffer requires overflow checks, which is good to have for reasons explained above.
Comment on attachment 325625 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=325625&action=review >>>> Source/JavaScriptCore/API/JSObjectRef.cpp:189 >>>> + } >>> >>> I wonder if we should have a different API for such a workload where we know exactly how many arguments we're going to append. Something like: >>> if (MarkedArgumentBuffer::willSizeOverflow(argumentCount)) throwOOM; >> >> Hmmm, I'll look into it. > > I don't feel strongly about this. What you have is probably OK. I think it's probably more cognitive overhead to have 2 ways to check for overflow. I'll keep it as is for now.
Comment on attachment 325625 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=325625&action=review >>> Source/WebCore/bindings/scripts/test/JS/JSTestCallbackFunction.cpp:68 >>> + RELEASE_ASSERT(!args.hasOverflowed()); >> >> debug assert > > Will fix. I just realized that this is auto-generated by CodeGeneratorJS.pm. I opted for a RELEASE_ASSERT to be conservative, but after taking a second look, the code generator generates the appending of args based on the IDL. I'm sure that we never have interfaces with a number args could overflow the MarkedArgumentBuffer. I'll change CodeGeneratorJS.pm to use a debug assert, and re-generate these bindings test results.
Created attachment 325655 [details] patch for landing.
Thanks for the review. Landed in r224309: <http://trac.webkit.org/r224309>.
It's caused WPE to crash on start: #0 0x00007f0551670fcf in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:270 #1 0x00007f054e68caee in JSC::MarkedArgumentBuffer::~MarkedArgumentBuffer ( this=0x7ffd9c210038, __in_chrg=<optimized out>) at ../../Source/JavaScriptCore/runtime/ArgList.h:55 #2 0x00007f0551226322 in JSC::CachedCall::~CachedCall (this=0x7ffd9c20ffd0, __in_chrg=<optimized out>) at ../../Source/JavaScriptCore/interpreter/CachedCall.h:38 #3 0x00007f0551216d63 in JSC::replaceUsingRegExpSearch (vm=..., exec=0x7ffd9c210650, string=0x7f04e9d72060, searchValue=..., callData=..., callType=<incomplete type>, replacementString=..., replaceValue=...) at ../../Source/JavaScriptCore/runtime/StringPrototype.cpp:674 #4 0x00007f0551217a41 in JSC::replaceUsingRegExpSearch (vm=..., exec=0x7ffd9c210650, string=0x7f04e9d72060, searchValue=..., replaceValue=...) at ../../Source/JavaScriptCore/runtime/StringPrototype.cpp:818 #5 0x00007f05512185d5 in JSC::stringProtoFuncReplaceUsingRegExp ( exec=0x7ffd9c210650) at ../../Source/JavaScriptCore/runtime/StringPrototype.cpp:964 #6 0x00007f04fa7ff028 in ?? () #7 0x00007ffd9c2106f0 in ?? () #8 0x00007f0550ed7d23 in llint_entry () at ../../Source/JavaScriptCore/runtime/PropertySlot.h:139 Backtrace stopped: frame did not save the PC I guess it's a preexisting bug exposed by this patch, correct? Can it be handled here, or should I open a new bug with a full backtrace?
Forgot to include the assertion: ASSERTION FAILED: !m_needsOverflowCheck ../../Source/JavaScriptCore/runtime/ArgList.h(55) : JSC::MarkedArgumentBuffer::~MarkedArgumentBuffer()
(In reply to Michael Catanzaro from comment #31) > It's caused WPE to crash on start: > I guess it's a preexisting bug exposed by this patch, correct? > > Can it be handled here, or should I open a new bug with a full backtrace? Please file a new bug and assign it to me. Thanks.
For the record, Michael C filed https://bugs.webkit.org/show_bug.cgi?id=179185, and the fix for that was landed in r224399: <https://trac.webkit.org/changeset/224399>.