Fix data race problem in createImageBitmap(Blob)

third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp

 /*
  * Copyright (c) 2013, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 26 matching lines @@
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/UseCounter.h"
 #include "core/html/HTMLCanvasElement.h"
 #include "core/html/HTMLImageElement.h"
 #include "core/html/HTMLVideoElement.h"
 #include "core/html/ImageData.h"
 #include "core/imagebitmap/ImageBitmapOptions.h"
 #include "core/workers/WorkerGlobalScope.h"
 #include "platform/SharedBuffer.h"
 #include "platform/ThreadSafeFunctional.h"
-#include "platform/image-decoders/ImageDecoder.h"
+#include "platform/graphics/DeferredImageDecoder.h"
 #include "platform/threading/BackgroundTaskRunner.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebThread.h"
 #include "public/platform/WebTraceLocation.h"
 #include <v8.h>
 
 namespace blink {
 
 static inline ImageBitmapSource* toImageBitmapSourceInternal(const ImageBitmapSourceUnion& value)
 {
@@ skipping 157 matching lines @@
 }
 
 void ImageBitmapFactories::ImageBitmapLoader::decodeImageOnDecoderThread(WebTaskRunner* taskRunner)
 {
     ASSERT(!isMainThread());
     RefPtr<SharedBuffer> sharedBuffer = SharedBuffer::create((char*)m_loader.arrayBufferResult()->data(), static_cast<size_t>(m_loader.arrayBufferResult()->byteLength()));
 
     ImageDecoder::AlphaOption alphaOp = ImageDecoder::AlphaPremultiplied;
     if (m_options.premultiplyAlpha() == "none")
         alphaOp = ImageDecoder::AlphaNotPremultiplied;
-    OwnPtr<ImageDecoder> decoder(ImageDecoder::create(*sharedBuffer, alphaOp, ImageDecoder::GammaAndColorProfileApplied));
+    OwnPtr<DeferredImageDecoder> decoder(DeferredImageDecoder::create(*sharedBuffer, alphaOp, ImageDecoder::GammaAndColorProfileApplied));
     if (decoder)
-        decoder->setData(sharedBuffer.get(), true);
+        decoder->setData(*sharedBuffer, true);
     taskRunner->postTask(BLINK_FROM_HERE, threadSafeBind(&ImageBitmapFactories::ImageBitmapLoader::resolvePromiseOnOriginalThread, AllowCrossThreadAccess(this), decoder.release()));
 }
 
-void ImageBitmapFactories::ImageBitmapLoader::resolvePromiseOnOriginalThread(PassOwnPtr<ImageDecoder> decoder)
+void ImageBitmapFactories::ImageBitmapLoader::resolvePromiseOnOriginalThread(PassOwnPtr<DeferredImageDecoder> decoder)
 {
     if (!decoder) {
         rejectPromise();
         return;
     }
-    RefPtr<SkImage> frame = ImageBitmap::getSkImageFromDecoder(decoder);
+    RefPtr<SkImage> frame = decoder->createFrameAtIndex(0);

[Justin Novosad, 2016/03/09 16:41:03]

Have you verified this in tracing? The DeferredImageDecoder uses a lazy decoding
scheme, and I think that in this case, the decoding will only be triggered here,
on the main thread, which is not what you intended.  Perhaps
decodeImageOnDecoderThread() should be passing the final SkImage rather that the
decoder. If you do that, I think you should just use ImageDecoder.

Even though the thread-safeness of DeferredImageDecoder solves the race
condition, you should not use this class unless deferred decoding is your
intent, which is not the case here.

[xidachen, 2016/03/09 17:49:26]

I agree that getting the SkImage on the worker thread and pass it back to the
main thread is the best strategy and I actually tried this when I implemented
decoding in a background thread. The problem is that when passing SkImage across
thread, bindings gives me trouble. To pass a SkImage across thread using
postTask, it goes to the CrossThreadCopier and that requires all the object
passed across thread to be ThreadSafeRefCounted, and SkImage is not, neither
RefPtr is, so that's why I eventually chose to pass the ImageDecoder across
thread. 

I can see that there are two options here:
1. Wrap the RefPtr<SkImage> using a OwnPtr and pass across threads. This doesn't
look that good.
2. Change the CrossThreadCopierBase to be able to pass an object that is
SkRefCnt.

The second one is more reasonable. But I am not sure whether it is reasonable to
include Sk headers under platform/, WDYT?

     ASSERT(!frame || (frame->width() && frame->height()));
     if (!frame) {
         rejectPromise();
         return;
     }
 
     RefPtr<StaticBitmapImage> image = StaticBitmapImage::create(frame);
     image->setOriginClean(true);
     if (!m_cropRect.width() && !m_cropRect.height()) {
         // No cropping variant was called.
@@ skipping 10 matching lines @@
     m_factory->didFinishLoading(this);
 }
 
 DEFINE_TRACE(ImageBitmapFactories::ImageBitmapLoader)
 {
     visitor->trace(m_factory);
     visitor->trace(m_resolver);
 }
 
 } // namespace blink