[webkit-reviews] review granted: [Bug 200792] CodeBlock destructor should clear all of its watchpoints. : [Attachment 376482] proposed patch.

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 200792: CodeBlock destructor should clear all of its watchpoints.
https://bugs.webkit.org/show_bug.cgi?id=200792

Attachment 376482: proposed patch.

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




--- Comment #3 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 376482
  --> https://bugs.webkit.org/attachment.cgi?id=376482
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?