Bug 179092 – Add support to throw OOM if MarkedArgumentBuffer may overflow.

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

[Mark Lam]

Patch coming.

<rdar://problem/35116160>

[Mark Lam]

Created attachment 325532 [details]
proposed patch.

Let's get some EWS testing on this patch.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Mark Lam]

Created attachment 325585 [details]
proposed patch.

Fixed a few bugs.  Let's try the EWS again.

[Mark Lam]

Created attachment 325597 [details]
proposed patch.

More bug fixes.  I think this should be it.  Let's verify again with the EWS.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

Created attachment 325625 [details]
proposed patch.

One bug snuck by me, now fixed.  Let's get some EWS testing again.

[Mark Lam]

Comment on attachment 325625 [details]
proposed patch.

The EWS is happy.  Let's get a review.

[Saam Barati]

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.

[Mark Lam]

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.

[Saam Barati]

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.

[Saam Barati]

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.

[Saam Barati]

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.

[Mark Lam]

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.

[Mark Lam]

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.

[Mark Lam]

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.

[Mark Lam]

Created attachment 325655 [details]
patch for landing.

[Mark Lam]

Thanks for the review.  Landed in r224309: <http://trac.webkit.org/r224309>.

[Michael Catanzaro]

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?

[Michael Catanzaro]

Forgot to include the assertion:

ASSERTION FAILED: !m_needsOverflowCheck
../../Source/JavaScriptCore/runtime/ArgList.h(55) : JSC::MarkedArgumentBuffer::~MarkedArgumentBuffer()

[Mark Lam]

(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.

[Mark Lam]

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>.