Bug 234190 - WebCore::createDOMException() should abort early if termination is pending.
Summary: WebCore::createDOMException() should abort early if termination is pending.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-10 18:07 PST by Mark Lam
Modified: 2021-12-11 08:57 PST (History)
4 users (show)

See Also:


Attachments
[fast-cq] proposed patch. (1.61 KB, patch)
2021-12-10 18:16 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2021-12-10 18:07:12 PST
Attempting to create Error objects may re-enter the VM, which we should not do when termination is pending.
Comment 1 Mark Lam 2021-12-10 18:16:38 PST
Created attachment 446853 [details]
[fast-cq] proposed patch.
Comment 2 Darin Adler 2021-12-10 18:29:14 PST
Comment on attachment 446853 [details]
[fast-cq] proposed patch.

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

> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:145
> +    VM& vm = lexicalGlobalObject->vm();
> +    if (UNLIKELY(vm.hasPendingTerminationException()))
> +        return jsUndefined();

What about the other similar functions, like the ones called by this function? For example, createSyntaxError?

I don’t think we need to put "vm" into a local variable, even though we do that often, since we are only using it once here.
Comment 3 Darin Adler 2021-12-10 18:30:19 PST
I suspect we’ll need this test for hasPendingTerminationException in more places. I don’t think this one function could possibly be the only one with a unique need for it.
Comment 4 Darin Adler 2021-12-10 18:31:34 PST
We should think about exactly which level is responsible for this check, and possibly move it elsewhere.
Comment 5 Mark Lam 2021-12-10 19:11:35 PST
I was hoping createDOMException() would be a good choke point, but I didn't do the due diligence.  You are correct: the underlying factory methods are called from so many places in WebCore.  On JSC side, we have regimented exception checks which would prevent these from being called.  But on WebCore side, perhaps we need something more.  I'll look into to moving the check lower, or see if I can think of a more elegant solution.
Comment 6 Mark Lam 2021-12-11 08:53:02 PST
On second thought, this fix works, and will prevent http/wpt/fetch/ tests from failing flakily due to this issue.  So, let's land this first to help alleviate the bots while we think of better solutions.
Comment 7 EWS 2021-12-11 08:56:27 PST
Committed r286912 (245138@main): <https://commits.webkit.org/245138@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446853 [details].
Comment 8 Radar WebKit Bug Importer 2021-12-11 08:57:16 PST
<rdar://problem/86365930>