Bug 223777

Summary:

Handle OOM in ScriptExecutionContext::reportUnhandledPromiseRejection

Product:

WebKit

Reporter:

Tadeu Zagallo <tzagallo>

Component:

WebCore JavaScript

Assignee:

Tadeu Zagallo <tzagallo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, darin, esprehn+autocc, ews-watchlist, hi, kangil.han, mkwst, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=225258

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Description Tadeu Zagallo 2021-03-25 17:36:57 PDT

<rdar://66271491>

Comment 1 Tadeu Zagallo 2021-03-25 17:40:04 PDT

Created attachment 424307 [details]
Patch

Comment 2 Tadeu Zagallo 2021-03-26 09:26:07 PDT

Created attachment 424364 [details]
Patch

Comment 3 Tadeu Zagallo 2021-04-05 10:15:25 PDT

Created attachment 425166 [details]
Patch

Comment 4 Tadeu Zagallo 2021-04-05 13:32:24 PDT

Created attachment 425199 [details]
Patch

Comment 5 Yusuke Suzuki 2021-04-05 13:55:36 PDT

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.

Comment 6 Tadeu Zagallo 2021-04-06 07:29:17 PDT

Created attachment 425276 [details]
Patch

Comment 7 EWS 2021-04-06 09:00:42 PDT

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

Comment 8 EWS 2021-04-06 09:05:06 PDT

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

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

Comment 9 Darin Adler 2021-04-17 21:47:04 PDT

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!

Comment 10 Tadeu Zagallo 2021-04-19 07:53:20 PDT

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

Comment 11 Darin Adler 2021-04-19 09:29:43 PDT

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