[webkit-reviews] review granted: [Bug 129393] ASSERTION FAILED: m_numBreakpoints >= numBreakpoints when deleting breakpoints : [Attachment 225616] patch 2: eagerly reaps zombie CodeBlocks, and waits for compilations to complete before proceeding with debugger/profiler work

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 129393: ASSERTION FAILED: m_numBreakpoints >= numBreakpoints when deleting
breakpoints
https://bugs.webkit.org/show_bug.cgi?id=129393

Attachment 225616: patch 2: eagerly reaps zombie CodeBlocks, and waits for
compilations to complete before proceeding with debugger/profiler work
https://bugs.webkit.org/attachment.cgi?id=225616&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225616&action=review


r=me

> Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp:41
> +void DeferredCompilationCallback::compilationDidComplete(CodeBlock*
codeBlock, CompilationResult result)
> +{
> +    ASSERT(result == CompilationFailed || result == CompilationInvalidated
|| result == CompilationSuccessful);
> +    if (result != CompilationSuccessful)
> +	   codeBlock->heap()->removeCodeBlock(codeBlock);
> +}

I would use a switch statement here. That way, the compiler will check your
assertion at compile time.

> Source/JavaScriptCore/heap/CodeBlockSet.h:70
> +    // Remove a CodeBlock. This is only called when compilation is not
successful
> +    // in generating the jitCode for the CodeBlock.
> +    void remove(CodeBlock*);

It's a layering violation to make promises about who will call this function
and / or when. Let's remove this comment.

> Source/JavaScriptCore/runtime/VM.h:502
> -	   void prepareToDiscardCode();
> +	   void prepareToDiscardCode() { waitForCompilationsToComplete(); }
> +	   void waitForCompilationsToComplete();

Instead of having two names for the same thing, let's pick one and have all
callers use it.