Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Implements Unwrap(CrossThreadWeakPersistent) that returns a CrossThreadPersistent so the object doesn't get collected during callback invocation.

BUG=591606
Committed: https://crrev.com/56b559a7548bb3928df3ac0d57a57b84dc170826
Cr-Commit-Position: refs/heads/master@{#418516}

[keishi, 2016-08-18 05:23:57]

Description was changed from

==========
Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

BUG=
==========

to

==========
Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Adds CrossThreadPersistent constructor for CrossThreadWeakPersistent that locks
the CrossThreadPersistentRegion mutex before transferring the reference.

Implements Unwrap(CrossThreadWeakPersistent) that returns a
CrossThreadPersistent so the object doesn't get collected during callback
invocation.

BUG=591606
==========

[keishi, 2016-08-18 05:27:32]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org, tzik@chromium.org

[keishi, 2016-08-18 05:27:33]

PTAL

tzik@: You were worried about Unwrap being called after shutdown and creating a
new CrossThreadPersistent. But ProcessHeap::crossThreadPersistentRegion is using
DEFINE_THREAD_SAFE_STATIC_LOCAL so I think we should be able to create a
CrossThreadPersistent without problem.

[haraken, 2016-08-18 08:43:27]

> Implements Unwrap(CrossThreadWeakPersistent) that returns a
CrossThreadPersistent so the object doesn't get collected during callback
invocation.

Does this mean that the task holds a strong Persistent to the underlying
function? If that's the case, it would be problematic because we sometimes use
CrossThreadWeakPersistent in order not to let the task hold a strong Persistent
to the underlying function.

[tzik, 2016-08-18 13:02:35]

On 2016/08/18 08:43:27, haraken wrote:
> > Implements Unwrap(CrossThreadWeakPersistent) that returns a
> CrossThreadPersistent so the object doesn't get collected during callback
> invocation.
> 
> Does this mean that the task holds a strong Persistent to the underlying
> function? If that's the case, it would be problematic because we sometimes use
> CrossThreadWeakPersistent in order not to let the task hold a strong
Persistent
> to the underlying function.

The Unwrap(CTWP) implies that the task gets a strong reference right before its
invocation. It still holds a weak reference while it is in queue or held by
other objects, and the GCed object can still be collected.

[haraken, 2016-08-18 13:15:31]

https://codereview.chromium.org/2207333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2207333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Persistent.h:78:
CrossThreadPersistentRegion::LockScope
persistentLock(ProcessHeap::crossThreadPersistentRegion());

I wonder why you need to add the LockScope here. Isn't the MutexLock in
CrossThreadPersistentRegion::allocatePersistentNode enough?

[keishi, 2016-08-22 09:48:55]

https://codereview.chromium.org/2207333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2207333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Persistent.h:78:
CrossThreadPersistentRegion::LockScope
persistentLock(ProcessHeap::crossThreadPersistentRegion());
On 2016/08/18 13:15:31, haraken wrote:
> 
> I wonder why you need to add the LockScope here. Isn't the MutexLock in
> CrossThreadPersistentRegion::allocatePersistentNode enough?
> 

The original code sets m_raw then calls allocatePersistentNode. I thought the
object might be collected before the call to allocatePersistentNode. (if per
thread heap is enabled)

[haraken, 2016-08-22 13:59:59]

https://codereview.chromium.org/2207333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2207333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Persistent.h:78:
CrossThreadPersistentRegion::LockScope
persistentLock(ProcessHeap::crossThreadPersistentRegion());
On 2016/08/22 09:48:55, keishi wrote:
> On 2016/08/18 13:15:31, haraken wrote:
> > 
> > I wonder why you need to add the LockScope here. Isn't the MutexLock in
> > CrossThreadPersistentRegion::allocatePersistentNode enough?
> > 
> 
> The original code sets m_raw then calls allocatePersistentNode. I thought the
> object might be collected before the call to allocatePersistentNode. (if per
> thread heap is enabled)

I'd say that if that happens, it's a bug of the caller site. The caller site
must keep holding a strong reference to the |other| pointer until the assignment
has been complete.

Note that even if you have the LockScope, it cannot rescue the following
scenario:

...; // If a per-thread GC happens in another thread, the |other| pointer will
be collected.
LockScope scope(region()); // This is no help.
m_raw = other;

[keishi, 2016-09-14 07:27:48]

Description was changed from

==========
Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Adds CrossThreadPersistent constructor for CrossThreadWeakPersistent that locks
the CrossThreadPersistentRegion mutex before transferring the reference.

Implements Unwrap(CrossThreadWeakPersistent) that returns a
CrossThreadPersistent so the object doesn't get collected during callback
invocation.

BUG=591606
==========

to

==========
Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Implements Unwrap(CrossThreadWeakPersistent) that returns a
CrossThreadPersistent so the object doesn't get collected during callback
invocation.

BUG=591606
==========

[keishi, 2016-09-14 07:32:08]

For example MessagePort::messageAvailable() does:
getExecutionContext()->postTask(BLINK_FROM_HERE,
createCrossThreadTask(&MessagePort::dispatchMessages,
wrapCrossThreadWeakPersistent(this)));

To ensure |this| is not destroyed while the task is invoked, I think we need
this Unwrap override.
For this new PS I use the CrossThreadPersistentRegion lock directly.

[haraken, 2016-09-14 07:38:45]

LGTM

[keishi, 2016-09-14 07:40:59]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-14 07:41:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 09:16:07]

Description was changed from

==========
Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Implements Unwrap(CrossThreadWeakPersistent) that returns a
CrossThreadPersistent so the object doesn't get collected during callback
invocation.

BUG=591606
==========

to

==========
Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Implements Unwrap(CrossThreadWeakPersistent) that returns a
CrossThreadPersistent so the object doesn't get collected during callback
invocation.

BUG=591606
==========

[commit-bot: I haz the power, 2016-09-14 09:16:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-14 09:17:51]

Description was changed from

==========
Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Implements Unwrap(CrossThreadWeakPersistent) that returns a
CrossThreadPersistent so the object doesn't get collected during callback
invocation.

BUG=591606
==========

to

==========
Support CrossThreadWeakPersistent PostTask on per thread heap enabled thread

Implements Unwrap(CrossThreadWeakPersistent) that returns a
CrossThreadPersistent so the object doesn't get collected during callback
invocation.

BUG=591606

Committed: https://crrev.com/56b559a7548bb3928df3ac0d57a57b84dc170826
Cr-Commit-Position: refs/heads/master@{#418516}
==========

[commit-bot: I haz the power, 2016-09-14 09:17:53]

Patchset 4 (id:??) landed as
https://crrev.com/56b559a7548bb3928df3ac0d57a57b84dc170826
Cr-Commit-Position: refs/heads/master@{#418516}