Bug 200792 – CodeBlock destructor should clear all of its watchpoints.

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

[Mark Lam]

<rdar://problem/53947800>

[Mark Lam]

Created attachment 376434 [details]
proposed patch.

[Mark Lam]

Created attachment 376482 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 376482 [details]
proposed patch.

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

r=me with comments.
And can you ensure that all the watchpoints having CodeBlock as an owner are cleared by DFG::Common::clearWatchpoints?
LLIntPrototypeLoadAdaptiveStructureWatchpoint is not included, but it is OK since it is held by CodeBlock directly.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:814
> +#if ENABLE(DFG_JIT)

Can you add a comment describing this pathological thing here?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:815
> +    if (JITCode::isOptimizingJIT(jitType()))

Is baseline JIT OK?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:816
> +        jitCode()->dfgCommon()->clearWatchpoints();

The watchpoints are typically registered to jettison CodeBlock when the critical invariant is broken.
After CodeBlock is destroyed, I think that JITCode code itself is completely invalid.
Is it possible that the JITCode is executed while CodeBlock is destroyed. If it happens, why is it safe? If it does not happen, can we add assertion to ensure it in JITCode or some places?

[Mark Lam]

Thanks for the review.

(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 376482 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376482&action=review
> 
> r=me with comments.
> And can you ensure that all the watchpoints having CodeBlock as an owner are
> cleared by DFG::Common::clearWatchpoints?
> LLIntPrototypeLoadAdaptiveStructureWatchpoint is not included, but it is OK
> since it is held by CodeBlock directly.

I've audited all the Watchpoint types and verified that any watchpoints associated with a CodeBlock is handled by this patch.  I'll documented the audit details in the ChangeLog.


> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:814
> > +#if ENABLE(DFG_JIT)
> 
> Can you add a comment describing this pathological thing here?

Added.
 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:815
> > +    if (JITCode::isOptimizingJIT(jitType()))
> 
> Is baseline JIT OK?

Yes.  baseline JIT ha a null dfgCommon(), and hence, cannot have any of the watchpoints in there.

> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:816
> > +        jitCode()->dfgCommon()->clearWatchpoints();
> 
> The watchpoints are typically registered to jettison CodeBlock when the
> critical invariant is broken.
> After CodeBlock is destroyed, I think that JITCode code itself is completely
> invalid.
> Is it possible that the JITCode is executed while CodeBlock is destroyed. If
> it happens, why is it safe? If it does not happen, can we add assertion to
> ensure it in JITCode or some places?

I don't think we execute any code in the JITCode after the CodeBlock is destroyed.  We don't have control flow that will lead to us stepping on the dead code.  However, we do need to clear the watchpoints because we do have control flow that can lead to us stepping on the dead watchpoints if we don't eagerly clear them out.

To ensure that the dead JITCode is not executed after the CodeBlock is destroyed, we can do 1 of the following:
1. on debug builds, fill the JITCode with breakpoint traps on CodeBlock destruction if JITCode's refCount is greater than 1.  This ensure that we'll crash if someone tries to execute any instructions in there.  Or, ...
2. re-architect the system to ensure that CodeBlock is the sole owner of JITCode, and can cleanly delete it on destruction.

I think option 2 is the more efficient solution, but this is out of scope for this patch.  I can look into this later on.

I'll land this patch with the added comments.

[Mark Lam]

Created attachment 376550 [details]
patch for landing.

[Mark Lam]

Landed in r248800: <http://trac.webkit.org/r248800>.