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>
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.
Created attachment 344845 [details] proposed patch.
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
(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().
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.
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
Thanks for the review. Landed in r233772: <http://trac.webkit.org/r233772>.