[webkit-reviews] review denied: [Bug 171614] When the image decoding thread makes a callOnMainThread(), ensure all the objects it needs are protected : [Attachment 309959] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 171614: When the image decoding thread makes a callOnMainThread(), ensure
all the objects it needs are protected
https://bugs.webkit.org/show_bug.cgi?id=171614

Attachment 309959: Patch

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




--- Comment #8 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 309959
  --> https://bugs.webkit.org/attachment.cgi?id=309959
Patch

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

> Source/WebCore/ChangeLog:8
> +	   The asynchronous image decoding was design to not block the main
thread if

Typo:  "design" => "designed"

> Source/WebCore/ChangeLog:18
> +	   This might lead to two kinds of crashes:
> +	   1. A SIG fault inside the ImageDecoder trying to access one of its
member
> +	   2. A SIG fault inside the ImageFrameCache trying to access one of
its frames

Instead of "SIG fault" I'd use "segfault" (or SIGSEGV).

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:298
>	       // Update the cached frames on the main thread to avoid updating
the MemoryCache from a different thread.
> -	       callOnMainThread([this, protectedQueue =
protectedQueue.copyRef(), nativeImage, frameRequest] () mutable {
> +	       callOnMainThread([this, protectedThis = protectedThis.copyRef(),
protectedQueue = protectedQueue.copyRef(), protectedDecoder =
protectedDecoder.copyRef(), nativeImage, frameRequest] () mutable {
>		   // The queue may be closed if after we got the frame
NativeImage, stopAsyncDecodingQueue() was called
>		   if (protectedQueue.ptr() == m_decodingQueue) {
>		       ASSERT(m_frameCommitQueue.first() == frameRequest);

I don't think adding the protectedThings to the lambda is enough.  You need to
actually use them in the code inside the lambda, maybe something like this
(UNTESTED):

		// The queue may be closed if after we got the frame
NativeImage, stopAsyncDecodingQueue() was called
		if (protectedQueue.ptr() == protectedThis->m_decodingQueue) {
		    ASSERT(protectedThis->m_frameCommitQueue.first() ==
frameRequest);
		    protectedThis->m_frameCommitQueue.removeFirst();
		   
protectedThis->cacheNativeImageAtIndexAsync(WTFMove(nativeImage),
frameRequest.index, frameRequest.subsamplingLevel,
frameRequest.decodingOptions, frameRequest.decodingStatus);
		} else
		    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld
will not cached]", __FUNCTION__, protectedThis,
sourceURL().string().utf8().data(), frameRequest.index);

Where is protectedDecoder used here?  If it's not called by this block of code,
it probably isn't needed.  If it is called from this block of code, it may need
to be passed through (via function arguments) to where it's used.