Retain strong reference of BlobCallback in CanvasAsyncBlobCreator

Retain strong reference of BlobCallback in CanvasAsyncBlobCreator

CanvasAsyncBlobCreator holds a BlobCallback and runs it asynchronously.
However, it can be destroyed before the callback invocation, and the
callback doesn't retain its ownership while it's in the task queue.

This CL makes the bound task to retain the shared ownership of
BlobCallback.

BUG=621547
Committed: https://crrev.com/e7d195955d69a096ba28bcb4be85df1d64ffa2a8
Cr-Commit-Position: refs/heads/master@{#401052}

[tzik, 2016-06-21 14:00:21]

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

[tzik, 2016-06-21 14:00:22]

PTAL

[haraken, 2016-06-21 14:02:04]

LGTM

[tzik, 2016-06-21 14:02:44]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2016-06-21 14:03:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2081873003/1

[sof, 2016-06-21 14:08:38]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-06-21 14:08:39]

https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:267:
Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE,
bind(&BlobCallback::handleEvent, wrapCrossThreadPersistent(m_callback.get()),
resultBlob));
I don't understand; the m_callback was created on the main thread and we're
posting it to the main thread again here -- where's the cross-thread aspect?

[tzik, 2016-06-21 14:09:57]

The CQ bit was unchecked by tzik@chromium.org

[tzik, 2016-06-21 14:14:56]

https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:267:
Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE,
bind(&BlobCallback::handleEvent, wrapCrossThreadPersistent(m_callback.get()),
resultBlob));
On 2016/06/21 14:08:38, sof wrote:
> I don't understand; the m_callback was created on the main thread and we're
> posting it to the main thread again here -- where's the cross-thread aspect?

Oops, I missed the ASSERT above and thought they are same to
encodeImageOnEncoderThread().
Fixed.

[sof, 2016-06-21 14:21:04]

On 2016/06/21 14:14:56, tzik wrote:
>
https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
> (right):
> 
>
https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:267:
>
Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE,
> bind(&BlobCallback::handleEvent, wrapCrossThreadPersistent(m_callback.get()),
> resultBlob));
> On 2016/06/21 14:08:38, sof wrote:
> > I don't understand; the m_callback was created on the main thread and we're
> > posting it to the main thread again here -- where's the cross-thread aspect?
> 
> Oops, I missed the ASSERT above and thought they are same to
> encodeImageOnEncoderThread().
> Fixed.

The rules are in flux here, so I may have out-of-date info, but don't we get
persistent/retained for free for T* (where T is a GCed type)?

[tzik, 2016-06-21 14:25:42]

On 2016/06/21 14:21:04, sof wrote:
> On 2016/06/21 14:14:56, tzik wrote:
> >
>
https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:267:
> >
>
Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE,
> > bind(&BlobCallback::handleEvent,
wrapCrossThreadPersistent(m_callback.get()),
> > resultBlob));
> > On 2016/06/21 14:08:38, sof wrote:
> > > I don't understand; the m_callback was created on the main thread and
we're
> > > posting it to the main thread again here -- where's the cross-thread
aspect?
> > 
> > Oops, I missed the ASSERT above and thought they are same to
> > encodeImageOnEncoderThread().
> > Fixed.
> 
> The rules are in flux here, so I may have out-of-date info, but don't we get
> persistent/retained for free for T* (where T is a GCed type)?

It's true for now, but will no longer true soon. I'm removing raw pointers from
WTF::bind.

[sof, 2016-06-21 14:33:04]

On 2016/06/21 14:25:42, tzik wrote:
> On 2016/06/21 14:21:04, sof wrote:
> > On 2016/06/21 14:14:56, tzik wrote:
> > >
> >
>
https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2081873003/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:267:
> > >
> >
>
Platform::current()->mainThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE,
> > > bind(&BlobCallback::handleEvent,
> wrapCrossThreadPersistent(m_callback.get()),
> > > resultBlob));
> > > On 2016/06/21 14:08:38, sof wrote:
> > > > I don't understand; the m_callback was created on the main thread and
> we're
> > > > posting it to the main thread again here -- where's the cross-thread
> aspect?
> > > 
> > > Oops, I missed the ASSERT above and thought they are same to
> > > encodeImageOnEncoderThread().
> > > Fixed.
> > 
> > The rules are in flux here, so I may have out-of-date info, but don't we get
> > persistent/retained for free for T* (where T is a GCed type)?
> 
> It's true for now, but will no longer true soon. I'm removing raw pointers
from
> WTF::bind.

ok, being explicit is worth it here. But disallowing the use of Member<> in
bind() seems like a well-worthy followup.

[sof, 2016-06-21 14:33:22]

lgtm

[haraken, 2016-06-21 14:34:19]

Thanks for pointing it out. PS2 looks correct.

[tzik, 2016-06-21 15:06:57]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-06-21 15:06:57]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2081873003/#ps20001
(title: "s/wrapCrossThreadPersistent/wrapPersistent/")

[commit-bot: I haz the power, 2016-06-21 15:07:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2081873003/20001

[commit-bot: I haz the power, 2016-06-21 18:43:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-21 18:46:31]

Description was changed from

==========
Retain strong reference of BlobCallback in CanvasAsyncBlobCreator

CanvasAsyncBlobCreator holds a BlobCallback and runs it asynchronously.
However, it can be destroyed before the callback invocation, and the
callback doesn't retain its ownership while it's in the task queue.

This CL makes the bound task to retain the shared ownership of
BlobCallback.

BUG=621547
==========

to

==========
Retain strong reference of BlobCallback in CanvasAsyncBlobCreator

CanvasAsyncBlobCreator holds a BlobCallback and runs it asynchronously.
However, it can be destroyed before the callback invocation, and the
callback doesn't retain its ownership while it's in the task queue.

This CL makes the bound task to retain the shared ownership of
BlobCallback.

BUG=621547

Committed: https://crrev.com/e7d195955d69a096ba28bcb4be85df1d64ffa2a8
Cr-Commit-Position: refs/heads/master@{#401052}
==========

[commit-bot: I haz the power, 2016-06-21 18:46:34]

Patchset 2 (id:??) landed as
https://crrev.com/e7d195955d69a096ba28bcb4be85df1d64ffa2a8
Cr-Commit-Position: refs/heads/master@{#401052}