[webkit-reviews] review granted: [Bug 179092] Add support to throw OOM if MarkedArgumentBuffer may overflow. : [Attachment 325625] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 179092: Add support to throw OOM if MarkedArgumentBuffer may overflow.
https://bugs.webkit.org/show_bug.cgi?id=179092

Attachment 325625: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=325625&action=review




--- Comment #24 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 325625
  --> https://bugs.webkit.org/attachment.cgi?id=325625
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.cp
p: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.