Bug 207423 - Throw OutOfMemory exception instead of crashing if DirectArguments/ScopedArguments can't be created
Summary: Throw OutOfMemory exception instead of crashing if DirectArguments/ScopedArgu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-07 17:33 PST by Robin Morisset
Modified: 2020-02-07 22:09 PST (History)
8 users (show)

See Also:


Attachments
Patch (19.19 KB, patch)
2020-02-07 17:51 PST, Robin Morisset
mark.lam: review+
mark.lam: commit-queue-
Details | Formatted Diff | Diff
Patch (18.54 KB, patch)
2020-02-07 21:25 PST, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2020-02-07 17:33:25 PST
rdar://problem/58945874
Comment 1 Robin Morisset 2020-02-07 17:51:51 PST
Created attachment 390155 [details]
Patch
Comment 2 Mark Lam 2020-02-07 20:41:18 PST
Comment on attachment 390155 [details]
Patch

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

r=me

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1107
> +        LLINT_CHECK_EXCEPTION();

This is redundant because there's a LLINT_CHECK_EXCEPTION() immediately following this if statement.  Please remove.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:984
> +        CHECK_EXCEPTION();

I think you should put this after the if statement to match LLINT_SLOW_PATH_DECL(slow_path_del_by_val).  Looks like currently, we're missing an exception check after the call to deleteProperty().

> Source/JavaScriptCore/runtime/DirectArguments.cpp:125
> +    if (!backingStore) {

UNLIKELY()?

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:175
> +    scope.release();

Hmmm, use RELEASE_AND_RETURN instead of return in both cases below instead?  Not a big difference right now, but I think that is less error prone if any of the cases below expand to have more logic that might throw.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:285
> +        if (!backingStore) {

UNLIKELY()?
Comment 3 Robin Morisset 2020-02-07 21:25:41 PST
Created attachment 390165 [details]
Patch

Thanks for the review!
I applied all of your suggestions.
Comment 4 WebKit Commit Bot 2020-02-07 22:08:40 PST
Comment on attachment 390165 [details]
Patch

Clearing flags on attachment: 390165

Committed r256087: <https://trac.webkit.org/changeset/256087>
Comment 5 WebKit Commit Bot 2020-02-07 22:08:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2020-02-07 22:09:17 PST
<rdar://problem/59283467>