Fix data race problem in createImageBitmap(Blob)

Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.
Also, an existing layout tests that have data race condition
is changed.

BUG=593224
Committed: https://crrev.com/283d18607e366095c377ddf72d196a8c4e256429
Cr-Commit-Position: refs/heads/master@{#380609}

[xidachen, 2016-03-09 16:24:19]

Description was changed from

==========
Fix data race problem in createImageBitmap(Blob)

Previously there was a CL that changed createImageBitmap(Blob) from
using the ImageSource to ImageDecoder so that we can explicitly set the
options of premultiplyAlpha and colorspaceConversion. However, this
change seems to cause data race problem to an existing layout test, and
this data race is caught by a tsan bot.

This CL should fix the data race problem. In particular, the ImageSource
internally uses a DeferredImageDecoder, so this CL changes ImageDecoder
to DeferredImageDecoder, where we can still offer the options of
premultiplyAlpha and colorspaceConversion.

Note that this CL doesn't change behavior. We will use tsan bot for the
layout tests to make sure that the data rance is gone. Also, we have
existing layout tests for createImageBitmap(Blob) of premultiplyAlpha=
false, this CL should not break that.

BUG=593224
==========

to

==========
Fix data race problem in createImageBitmap(Blob)

Previously there was a CL that changed createImageBitmap(Blob) from
using the ImageSource to ImageDecoder so that we can explicitly set the
options of premultiplyAlpha and colorspaceConversion. However, this
change seems to cause data race problem to an existing layout test, and
this data race is caught by a tsan bot.

This CL should fix the data race problem. In particular, the ImageSource
internally uses a DeferredImageDecoder, so this CL changes ImageDecoder
to DeferredImageDecoder, where we can still offer the options of
premultiplyAlpha and colorspaceConversion.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.

BUG=593224
==========

[xidachen, 2016-03-09 16:29:48]

xidachen@chromium.org changed reviewers:
+ junov@chromium.org

[xidachen, 2016-03-09 16:29:48]

PTAL.

junov@: I tried to use the idea of creating the ImageDecoder on the main thread,
but there is a implementation difficulty here. 

ImageDecoder::create() requires the buffer data, and ImageDecoder::setData()
which does the decoding also requires the buffer data. The problem here is that
the buffer data is a RefPtr type and cannot be passed across the thread by using
postTask.

I noticed that this CL:
https://codereview.chromium.org/1696753002
was the one caused this data race problem. Before that CL, we were passing an
ImageSource (which contains a DeferredImageDecoder internally) across thread and
that was fine.

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

https://codereview.chromium.org/1781613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1781613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:237:
RefPtr<SkImage> frame = decoder->createFrameAtIndex(0);
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]

https://codereview.chromium.org/1781613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1781613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:237:
RefPtr<SkImage> frame = decoder->createFrameAtIndex(0);
On 2016/03/09 16:41:03, Justin Novosad wrote:
> 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.

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?

[Justin Novosad, 2016-03-09 17:57:14]

On 2016/03/09 17:49:26, xidachen wrote:
>
https://codereview.chromium.org/1781613002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
> (right):
> 
>
https://codereview.chromium.org/1781613002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:237:
> RefPtr<SkImage> frame = decoder->createFrameAtIndex(0);
> On 2016/03/09 16:41:03, Justin Novosad wrote:
> > 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.
> 
> 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.

Number 2. sounds reasonable to me. After all SkRefCnt uses atomic ref counting,
so it is already threadsafe.

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

Of course that is reasonable. Originally, platform was *the* place for putting
dependencies on skia.

[xidachen, 2016-03-09 18:44:24]

Description was changed from

==========
Fix data race problem in createImageBitmap(Blob)

Previously there was a CL that changed createImageBitmap(Blob) from
using the ImageSource to ImageDecoder so that we can explicitly set the
options of premultiplyAlpha and colorspaceConversion. However, this
change seems to cause data race problem to an existing layout test, and
this data race is caught by a tsan bot.

This CL should fix the data race problem. In particular, the ImageSource
internally uses a DeferredImageDecoder, so this CL changes ImageDecoder
to DeferredImageDecoder, where we can still offer the options of
premultiplyAlpha and colorspaceConversion.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.

BUG=593224
==========

to

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.

BUG=593224
==========

[xidachen, 2016-03-09 18:54:36]

xidachen@chromium.org changed reviewers:
+ haraken@chromium.org

[xidachen, 2016-03-09 18:54:36]

junov@: Right now the ImageDecoder is created an destroyed in the background
thread, and a SkImage is passed back to the main thread.

haraken@: I changed the CrossThreadCopier so that postTask() can pass an object
that is SkRefCnt which is thread-safe RefCnted.

PTAL. Thanks.

[Justin Novosad, 2016-03-09 19:10:53]

junov@chromium.org changed reviewers:
+ reed@chromium.org

[Justin Novosad, 2016-03-09 19:10:54]

+reed as an FYI : This CL makes blink recognize that SkRefCnt does ref counting
in a thread safe way.

[Justin Novosad, 2016-03-09 19:13:24]

core/ lgtm

[reed1, 2016-03-09 19:16:11]

reed@google.com changed reviewers:
+ bungeman@google.com, fmalita@chromium.org, mtklein@google.com, reed@google.com

[reed1, 2016-03-09 19:16:12]

+more skia refcnt experts

[haraken, 2016-03-09 23:27:27]

platform/ LGTM

[xidachen, 2016-03-10 00:22:22]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 00:22:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1781613002/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1781613002/50001

[xidachen, 2016-03-10 02:53:30]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-03-10 02:54:50]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-03-10 02:55:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1781613002/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1781613002/50001

[commit-bot: I haz the power, 2016-03-10 05:34:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-10 05:34:33]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xidachen, 2016-03-10 16:34:32]

Description was changed from

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.

BUG=593224
==========

to

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away. Also, an existing
layout tests that have data race condition 
is changed.

BUG=593224
==========

[xidachen, 2016-03-10 16:35:53]

junov@: the latest PS fix the layout test that has data race condition. 

PTAL.

[xidachen, 2016-03-10 16:38:39]

Description was changed from

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away. Also, an existing
layout tests that have data race condition 
is changed.

BUG=593224
==========

to

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.
Also, an existing layout tests that have data race condition 
is changed.

BUG=593224
==========

[xidachen, 2016-03-10 18:39:14]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 18:39:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1781613002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1781613002/70001

[xidachen, 2016-03-10 19:30:35]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-03-11 12:21:21]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-03-11 12:21:21]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1781613002/#ps70001
(title: "fix data rance layout test")

[commit-bot: I haz the power, 2016-03-11 12:21:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1781613002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1781613002/70001

[commit-bot: I haz the power, 2016-03-11 12:26:18]

Description was changed from

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.
Also, an existing layout tests that have data race condition 
is changed.

BUG=593224
==========

to

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.
Also, an existing layout tests that have data race condition 
is changed.

BUG=593224
==========

[commit-bot: I haz the power, 2016-03-11 12:26:19]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-03-11 12:27:12]

Description was changed from

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.
Also, an existing layout tests that have data race condition 
is changed.

BUG=593224
==========

to

==========
Fix data race problem in createImageBitmap(Blob)

In createImageBitmap(Blob), the method creates an ImageDecoder
in a background thread, does the image decoding in the same thread, and
pass the ImageDecoder back to the main thread for creating an ImageBitmap.
This is causing the data race problem because the ImageDecoder
does some non-atomic reference counts internally.

This CL should fix the data race problem. In particular, we let the
ImageDecoder does image decoding in the background thread, and creates
a SkImage from the ImageDecoder, and pass the SkImage across threads.
Because SkImage is thread-safe RefCnted, this solves the data race
problem. In order to let postTask taking a SkImage, this CL changes
the CrossThreadCopier so that it can take an object that is SkRefCnt.

Locally I was able to repro the test case reported in the bug by
using the tsan build. With this CL the data race goes away.
Also, an existing layout tests that have data race condition
is changed.

BUG=593224

Committed: https://crrev.com/283d18607e366095c377ddf72d196a8c4e256429
Cr-Commit-Position: refs/heads/master@{#380609}
==========

[commit-bot: I haz the power, 2016-03-11 12:27:13]

Patchset 5 (id:??) landed as
https://crrev.com/283d18607e366095c377ddf72d196a8c4e256429
Cr-Commit-Position: refs/heads/master@{#380609}