Bug 223777 – Handle OOM in ScriptExecutionContext::reportUnhandledPromiseRejection

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Tadeu Zagallo]

<rdar://66271491>

[Tadeu Zagallo]

Created attachment 424307 [details]
Patch

[Tadeu Zagallo]

Created attachment 424364 [details]
Patch

[Tadeu Zagallo]

Created attachment 425166 [details]
Patch

[Tadeu Zagallo]

Created attachment 425199 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 425199 [details]
Patch

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

r=me

> Source/WebCore/dom/ScriptExecutionContext.cpp:408
> +        if (resultMessage.is8Bit())
> +            return tryMakeString("Unhandled Promise Rejection: ", StringView { resultMessage.characters8(), length }, addEllipsis ? "..." : "");
> +        return tryMakeString("Unhandled Promise Rejection: ", StringView { resultMessage.characters16(), length }, addEllipsis ? "..." : "");

We can write

StringView(resultMessage).left(length)

> Source/WebCore/dom/ScriptExecutionContext.cpp:420
> +        errorMessage = "Unhandled Promise Rejection";

Let's use "Unhandled Promise Rejection"_s so that we can avoid allocation for string storage.

[Tadeu Zagallo]

Created attachment 425276 [details]
Patch

[EWS]

commit-queue failed to commit attachment 425276 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

Committed r275521: <https://commits.webkit.org/r275521>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425276 [details].

[Darin Adler]

Comment on attachment 425276 [details]
Patch

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

> Source/WebCore/dom/ScriptExecutionContext.cpp:414
> +        if (!errorMessage && resultMessage.length() > shortLength)
> +            errorMessage = tryMakeErrorString(shortLength);

Is this really helpful? I think the other two things (maximum length 200, failure case when even that fails) are enough. The 10 character one seems overkill!

[Tadeu Zagallo]

(In reply to Darin Adler from comment #9)
> Comment on attachment 425276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425276&action=review
> 
> > Source/WebCore/dom/ScriptExecutionContext.cpp:414
> > +        if (!errorMessage && resultMessage.length() > shortLength)
> > +            errorMessage = tryMakeErrorString(shortLength);
> 
> Is this really helpful? I think the other two things (maximum length 200,
> failure case when even that fails) are enough. The 10 character one seems
> overkill!

That's a fair point, it's unlikely this path will ever be taken. I was mostly following what we do in JavaScriptCore::LiteralParser.

[Darin Adler]

(In reply to Tadeu Zagallo from comment #10)
> I was mostly following what we do in JavaScriptCore::LiteralParser.

I haven’t looked, but it seems likely it is too much there, too. So tempting to over-engineer!