Bug 234188 – Automatically forbid JS execution when we throw a TerminationException.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

Created attachment 446848 [details]
proposed patch.

[Yusuke Suzuki]

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]

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

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

Created attachment 446851 [details]
[fast-cq] patch for landing.

[Mark Lam]

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]

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