Need to handle CodeBlock::replacement() being null.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

CodeBlock::replacement() may return a nullptr.  Some of our code already checks for this while others do not.  We should add null checks in all the places that need it.

<rdar://problem/41468692>

[Mark Lam]

Created attachment 344792 [details]
proposed patch.

Let's try this on the EWS.  I feel that I still need to think it through a bit more before it's ready for review.  But feel free to comment if you see something not right.  Thanks.

[Mark Lam]

Created attachment 344845 [details]
proposed patch.

[Saam Barati]

Comment on attachment 344845 [details]
proposed patch.

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

Weren’t you saying there was another place that needed a null check yesterday? What was the function you were seeing crash?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2427
> +    bool hasReplacement = (replacement && replacement != this);

No need for null check

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2428
> +    if ((result == CompilationSuccessful) != hasReplacement) {

I don’t think your change here is doing anything since this can’t be null

[Mark Lam]

(In reply to Saam Barati from comment #3)
> Comment on attachment 344845 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344845&action=review
> 
> Weren’t you saying there was another place that needed a null check
> yesterday? What was the function you were seeing crash?

The other place is operationOptimize().  That case is covered by the check added in CodeBlock::hasOptimizedReplacement().

[Mark Lam]

Comment on attachment 344845 [details]
proposed patch.

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2427
>> +    bool hasReplacement = (replacement && replacement != this);
> 
> No need for null check

Why?  Technically, we don't need the whole "if ((result == CompilationSuccessful) != (theReplacement != this))" check either because CodeBlock::setOptimizationThresholdBasedOnCompilationResult() is only called with result == CompilationSuccessful from JITToDFGDeferredCompilationCallback::compilationDidComplete() right after it installed the replacement.  The only reason that this check is here is to catch a bug that violates the invariant that we must have a replacement if result == CompilationSuccessful, and to print a meaningful error message when that bug is detected.  So, why not check for a null check as well that is also a violation of that invariant?

Note: (replacement != this) only detects that we have a replacement if and only if replacement is not null.

[Saam Barati]

Comment on attachment 344845 [details]
proposed patch.

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

>>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2427
>>> +    bool hasReplacement = (replacement && replacement != this);
>> 
>> No need for null check
> 
> Why?  Technically, we don't need the whole "if ((result == CompilationSuccessful) != (theReplacement != this))" check either because CodeBlock::setOptimizationThresholdBasedOnCompilationResult() is only called with result == CompilationSuccessful from JITToDFGDeferredCompilationCallback::compilationDidComplete() right after it installed the replacement.  The only reason that this check is here is to catch a bug that violates the invariant that we must have a replacement if result == CompilationSuccessful, and to print a meaningful error message when that bug is detected.  So, why not check for a null check as well that is also a violation of that invariant?
> 
> Note: (replacement != this) only detects that we have a replacement if and only if replacement is not null.

I misread this. Ignore me

[Mark Lam]

Thanks for the review.  Landed in r233772: <http://trac.webkit.org/r233772>.