WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
234188
Automatically forbid JS execution when we throw a TerminationException.
https://bugs.webkit.org/show_bug.cgi?id=234188
Summary
Automatically forbid JS execution when we throw a TerminationException.
Mark Lam
Reported
2021-12-10 17:10:26 PST
For Worker threads, we throw a TerminationException when Worker.terminate() is called. Once the TerminationException is thrown, we expect to completely unwind out of any JS frames on the stack, and we also expect the client to never call into JS again. Previously, WebCore will call VM:setExecutionForbidden() to flag that we should not re-enter the VM anymore. On JSC side, this executionForbidden() is used to prevent micro-tasks from firing. On WebCore side, it is used to prevent many things from running, including firing events. Previously, we reply on WebCore side to catch the TerminationException, determine that it is the TerminationException, and then call VM:setExecutionForbidden(). This is tedious and error prone as there may be places in WebCore that should call VM:setExecutionForbidden() but is missed. This has been the source of some bugs with the handling of the Worker termination in the past. In this patch, we change VM to setExecutionForbidden() immediately if when we throw the TerminationException, but only if VM::m_forbidExecutionOnTermination is set. Currently, we'll only set VM:m_forbidExecutionOnTermination for Workers because for legacy reasons, other clients of JSC has the ability to re-enter the VM after a TerminationException unwinds out (which is ok to do when used under some controlled conditions). Until we can determine that it is safe to adopt this "forbid execution on termination behavior" universally, we'll adopt it only for workers. In a subsequent patch, we can also look into removing all the places in WebCore that checks for TerminationException in order to call VM:setExecutionForbidden(). We'll leave those in place for now though they should be redundant after this patch.
Attachments
proposed patch.
(6.07 KB, patch)
2021-12-10 17:19 PST
,
Mark Lam
ysuzuki
: review+
Details
Formatted Diff
Diff
[fast-cq] patch for landing.
(6.22 KB, patch)
2021-12-10 17:45 PST
,
Mark Lam
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Mark Lam
Comment 1
2021-12-10 17:19:24 PST
Created
attachment 446848
[details]
proposed patch.
Yusuke Suzuki
Comment 2
2021-12-10 17:32:16 PST
Comment on
attachment 446848
[details]
proposed patch. View in context:
https://bugs.webkit.org/attachment.cgi?id=446848&action=review
r=me
> Source/JavaScriptCore/runtime/VM.h:1123 > + bool m_forbidExecutionOnTermination { false };
Since forbidExecutionOnTermination is verb and there is m_executionForbidden, I think renaming it to m_executionOnTerminationForbidden is better.
Mark Lam
Comment 3
2021-12-10 17:35:55 PST
(In reply to Yusuke Suzuki from
comment #2
)
> Comment on
attachment 446848
[details]
> proposed patch. > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=446848&action=review
> > r=me
Thanks.
> > Source/JavaScriptCore/runtime/VM.h:1123 > > + bool m_forbidExecutionOnTermination { false }; > > Since forbidExecutionOnTermination is verb and there is > m_executionForbidden, I think renaming it to > m_executionOnTerminationForbidden is better.
How about m_executionForbiddenOnTermination instead?
Mark Lam
Comment 4
2021-12-10 17:39:48 PST
(In reply to Mark Lam from
comment #3
)
> > > Source/JavaScriptCore/runtime/VM.h:1123 > > > + bool m_forbidExecutionOnTermination { false }; > > > > Since forbidExecutionOnTermination is verb and there is > > m_executionForbidden, I think renaming it to > > m_executionOnTerminationForbidden is better. > > How about m_executionForbiddenOnTermination instead?
I think I'll just go with this i.e. m_executionForbiddenOnTermination.
Mark Lam
Comment 5
2021-12-10 17:45:27 PST
Created
attachment 446851
[details]
[fast-cq] patch for landing.
Mark Lam
Comment 6
2021-12-11 08:56:25 PST
The jsc-armv7-tests EWS bot was showing red, but this simple patch should have 0 behavior change on JSC tests. So, the failures has to be due to something else. Also, the jsc-armv7-tests EWS bot appears to be showing severe infrastructure failures. Hence, I'm just going to move forward and land this.
EWS
Comment 7
2021-12-11 09:00:20 PST
Committed
r286913
(
245139@main
): <
https://commits.webkit.org/245139@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 446851
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug