Make ContentDecryptionModuleResult cross-thread destructible.

Make ContentDecryptionModuleResult cross-thread destructible.

So as to handle cross-thread uses of the corresponding
WebContentDecryptionModuleResult (WCDMResult), have WCDMResult
keep a cross-thread persistent (CrossThreadPersistent<>) by
way of its WebPrivatePtr<> reference.

CrossThreadPersistent<> can be destructed on a thread other than
the Oilpan thread creating it; the thread does not have to be
attached to Oilpan.

To control if WebPrivatePtr<> should use a cross-thread persistent
or not, it is now parameterized over an enum controlling which.
The default is to use same-thread persistents. If

  WebPrivatePtr<T, AllowCrossThreadDestruction>

is for a ref-counted T, T must derive from ThreadSafeRefCounted<T>. 

R=jrummell,xhwang,haraken,tkent
BUG=509588
Committed: https://crrev.com/106afb0edc37323671b75709967f463095b250f1
git-svn-id: svn://svn.chromium.org/blink/trunk@199421 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[sof, 2015-07-22 09:04:36]

sigbjornf@opera.com changed reviewers:
+ jrummell@chromium.org, oilpan-reviews@chromium.org, xhwang@chromium.org

[sof, 2015-07-22 09:04:37]

please take a look.

tricky terrain, but i think we need to be more careful with Persistent<>
handling & have it remain with Blink (Oilpan) threads entirely.

(This travels along similar lines to https://codereview.chromium.org/1228373006/
, btw.)

[jrummell, 2015-07-22 21:58:29]

Interesting way of doing it. lgtm for cdmResult.

[xhwang, 2015-07-22 22:00:44]

jrummell and I looked at this and it seems cool (though it took us a while to
figure out what you are doing). LGTM from media's perspective. I'll let
oilpan-reviews to review the details.

[haraken, 2015-07-23 01:41:41]

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

[haraken, 2015-07-23 01:41:41]

Thanks for looking into this issue! Appreciated it.

Mostly looks good to me.

https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:125: using type = CrossThreadPersistent<T>;

How about making CrossThreadPersistent by default? It seems that we're hitting
this issue in multiple places.

My long term plan is:

- Split Oilpan's heap (which is currently shared by all Blink threads) to
per-thread heaps.

- Use CrossThreadPersistent to point to an object in a heap of another thread.
This includes not only a CrossThreadPersistent from an oilpaned thread to
another oilpaned thread but also a CrossThreadPersistent from an non-oilpaned
thread to another oilpaned thread.

To make it workable, we need to make CrossThreadPersistent more thread-safe;
specifically:

(a) Acquire a global lock when the CrossThreadPersistent is created &
destructed.

(b) Acquire a gc lock when the CrossThreadPersistent is used (to avoid other
threads from mutating objects in a heap of another thread that is performing a
GC).

(a) is already implemented. (b) is not yet implemented.

This implies the following fact about the safety of the CrossThreadPersistent in
WebPrivatePtr:

- It is safe to create & destruct the WebPrivatePtr in different threads. The
different thread can be a non-oilpaned thread.

- It is unsafe to use the WebPrivatePtr in a non-oilpaned thread (because (b) is
not yet implemented).

I'm assuming that the above two facts hold in the current code base, but is my
understanding correct? (It would be worth having an assert to verify that a
CrossThreadPersistent is not used by a non-oilpaned thread.)

[sof, 2015-07-23 05:24:14]

On 2015/07/23 01:41:41, haraken wrote:
> Thanks for looking into this issue! Appreciated it.
> 
> Mostly looks good to me.
> 
>
https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
> File public/platform/WebPrivatePtr.h (right):
> 
>
https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
> public/platform/WebPrivatePtr.h:125: using type = CrossThreadPersistent<T>;
> 
> How about making CrossThreadPersistent by default? It seems that we're hitting
> this issue in multiple places.
> 
> My long term plan is:
> 
> - Split Oilpan's heap (which is currently shared by all Blink threads) to
> per-thread heaps.
> 
> - Use CrossThreadPersistent to point to an object in a heap of another thread.
> This includes not only a CrossThreadPersistent from an oilpaned thread to
> another oilpaned thread but also a CrossThreadPersistent from an non-oilpaned
> thread to another oilpaned thread.
> 
> To make it workable, we need to make CrossThreadPersistent more thread-safe;
> specifically:
> 
> (a) Acquire a global lock when the CrossThreadPersistent is created &
> destructed.
> 
> (b) Acquire a gc lock when the CrossThreadPersistent is used (to avoid other
> threads from mutating objects in a heap of another thread that is performing a
> GC).
> 
> (a) is already implemented. (b) is not yet implemented.
> 
> This implies the following fact about the safety of the CrossThreadPersistent
in
> WebPrivatePtr:
> 
> - It is safe to create & destruct the WebPrivatePtr in different threads. The
> different thread can be a non-oilpaned thread.
> 
> - It is unsafe to use the WebPrivatePtr in a non-oilpaned thread (because (b)
is
> not yet implemented).
> 
> I'm assuming that the above two facts hold in the current code base, but is my
> understanding correct? (It would be worth having an assert to verify that a
> CrossThreadPersistent is not used by a non-oilpaned thread.)

CrossThreadPersistent<T> intent was on passing destruction responsibilites from
one Oilpan thread to another Oilpan attached thread, at least that has been my
understanding all along & code comments suggest as much.

I'm wondering if (b) is in place already (cf tracePersistentNodes() for the
cross-thread case), but not at all sure if that's complete & sufficient.

Switching all WebPrivatePtr<>s to use CrossThreadPersistent<> is excessive;
there's two cases where we have cross-thread issues with Result values. Let's
not enforce the overhead of handling them everywhere.

[haraken, 2015-07-23 07:40:47]

On 2015/07/23 05:24:14, sof wrote:
> On 2015/07/23 01:41:41, haraken wrote:
> > Thanks for looking into this issue! Appreciated it.
> > 
> > Mostly looks good to me.
> > 
> >
>
https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
> > File public/platform/WebPrivatePtr.h (right):
> > 
> >
>
https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
> > public/platform/WebPrivatePtr.h:125: using type = CrossThreadPersistent<T>;
> > 
> > How about making CrossThreadPersistent by default? It seems that we're
hitting
> > this issue in multiple places.
> > 
> > My long term plan is:
> > 
> > - Split Oilpan's heap (which is currently shared by all Blink threads) to
> > per-thread heaps.
> > 
> > - Use CrossThreadPersistent to point to an object in a heap of another
thread.
> > This includes not only a CrossThreadPersistent from an oilpaned thread to
> > another oilpaned thread but also a CrossThreadPersistent from an
non-oilpaned
> > thread to another oilpaned thread.
> > 
> > To make it workable, we need to make CrossThreadPersistent more thread-safe;
> > specifically:
> > 
> > (a) Acquire a global lock when the CrossThreadPersistent is created &
> > destructed.
> > 
> > (b) Acquire a gc lock when the CrossThreadPersistent is used (to avoid other
> > threads from mutating objects in a heap of another thread that is performing
a
> > GC).
> > 
> > (a) is already implemented. (b) is not yet implemented.
> > 
> > This implies the following fact about the safety of the
CrossThreadPersistent
> in
> > WebPrivatePtr:
> > 
> > - It is safe to create & destruct the WebPrivatePtr in different threads.
The
> > different thread can be a non-oilpaned thread.
> > 
> > - It is unsafe to use the WebPrivatePtr in a non-oilpaned thread (because
(b)
> is
> > not yet implemented).
> > 
> > I'm assuming that the above two facts hold in the current code base, but is
my
> > understanding correct? (It would be worth having an assert to verify that a
> > CrossThreadPersistent is not used by a non-oilpaned thread.)
> 
> CrossThreadPersistent<T> intent was on passing destruction responsibilites
from
> one Oilpan thread to another Oilpan attached thread, at least that has been my
> understanding all along & code comments suggest as much.
> 
> I'm wondering if (b) is in place already (cf tracePersistentNodes() for the
> cross-thread case), but not at all sure if that's complete & sufficient.
> 
> Switching all WebPrivatePtr<>s to use CrossThreadPersistent<> is excessive;
> there's two cases where we have cross-thread issues with Result values. Let's
> not enforce the overhead of handling them everywhere.

Sounds reasonable to me.

I guess it would be better to introduce CrossThreadWebPrivatePtr to make it
clear that it is holding a cross-thread thing.

- WebPrivatePtr holds a storage with a Persistent (in oilpan) and a RefPtr to a
RefCounted object (in non-oilpan).
- CrossThreadWebPrivatePtr holds a storage with a CrossThreadPersistent (in
oilpan) and a RefPtr to a ThreadSafeRefCounted object (in non-oilpan).

[sof, 2015-07-23 08:59:48]

On 2015/07/23 07:40:47, haraken wrote:
> On 2015/07/23 05:24:14, sof wrote:
> > On 2015/07/23 01:41:41, haraken wrote:
> > > Thanks for looking into this issue! Appreciated it.
> > > 
> > > Mostly looks good to me.
> > > 
> > >
> >
>
https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
> > > File public/platform/WebPrivatePtr.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
> > > public/platform/WebPrivatePtr.h:125: using type =
CrossThreadPersistent<T>;
> > > 
> > > How about making CrossThreadPersistent by default? It seems that we're
> hitting
> > > this issue in multiple places.
> > > 
> > > My long term plan is:
> > > 
> > > - Split Oilpan's heap (which is currently shared by all Blink threads) to
> > > per-thread heaps.
> > > 
> > > - Use CrossThreadPersistent to point to an object in a heap of another
> thread.
> > > This includes not only a CrossThreadPersistent from an oilpaned thread to
> > > another oilpaned thread but also a CrossThreadPersistent from an
> non-oilpaned
> > > thread to another oilpaned thread.
> > > 
> > > To make it workable, we need to make CrossThreadPersistent more
thread-safe;
> > > specifically:
> > > 
> > > (a) Acquire a global lock when the CrossThreadPersistent is created &
> > > destructed.
> > > 
> > > (b) Acquire a gc lock when the CrossThreadPersistent is used (to avoid
other
> > > threads from mutating objects in a heap of another thread that is
performing
> a
> > > GC).
> > > 
> > > (a) is already implemented. (b) is not yet implemented.
> > > 
> > > This implies the following fact about the safety of the
> CrossThreadPersistent
> > in
> > > WebPrivatePtr:
> > > 
> > > - It is safe to create & destruct the WebPrivatePtr in different threads.
> The
> > > different thread can be a non-oilpaned thread.
> > > 
> > > - It is unsafe to use the WebPrivatePtr in a non-oilpaned thread (because
> (b)
> > is
> > > not yet implemented).
> > > 
> > > I'm assuming that the above two facts hold in the current code base, but
is
> my
> > > understanding correct? (It would be worth having an assert to verify that
a
> > > CrossThreadPersistent is not used by a non-oilpaned thread.)
> > 
> > CrossThreadPersistent<T> intent was on passing destruction responsibilites
> from
> > one Oilpan thread to another Oilpan attached thread, at least that has been
my
> > understanding all along & code comments suggest as much.
> > 
> > I'm wondering if (b) is in place already (cf tracePersistentNodes() for the
> > cross-thread case), but not at all sure if that's complete & sufficient.
> > 
> > Switching all WebPrivatePtr<>s to use CrossThreadPersistent<> is excessive;
> > there's two cases where we have cross-thread issues with Result values.
Let's
> > not enforce the overhead of handling them everywhere.
> 
> Sounds reasonable to me.
> 
> I guess it would be better to introduce CrossThreadWebPrivatePtr to make it
> clear that it is holding a cross-thread thing.
> 
> - WebPrivatePtr holds a storage with a Persistent (in oilpan) and a RefPtr to
a
> RefCounted object (in non-oilpan).
> - CrossThreadWebPrivatePtr holds a storage with a CrossThreadPersistent (in
> oilpan) and a RefPtr to a ThreadSafeRefCounted object (in non-oilpan).

Makes sense to push it down into the private pointer like that, but don't like
the code duplication it would entail.

Would it be acceptable to make it an (optional) argument over WebPrivatePtr<>
instead? (WebPrivatePtr<T, AllowCrossThreadDestruction>)

[haraken, 2015-07-23 10:46:10]

On 2015/07/23 08:59:48, sof wrote:
> On 2015/07/23 07:40:47, haraken wrote:
> > On 2015/07/23 05:24:14, sof wrote:
> > > On 2015/07/23 01:41:41, haraken wrote:
> > > > Thanks for looking into this issue! Appreciated it.
> > > > 
> > > > Mostly looks good to me.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
> > > > File public/platform/WebPrivatePtr.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1249913002/diff/60001/public/platform/WebPriv...
> > > > public/platform/WebPrivatePtr.h:125: using type =
> CrossThreadPersistent<T>;
> > > > 
> > > > How about making CrossThreadPersistent by default? It seems that we're
> > hitting
> > > > this issue in multiple places.
> > > > 
> > > > My long term plan is:
> > > > 
> > > > - Split Oilpan's heap (which is currently shared by all Blink threads)
to
> > > > per-thread heaps.
> > > > 
> > > > - Use CrossThreadPersistent to point to an object in a heap of another
> > thread.
> > > > This includes not only a CrossThreadPersistent from an oilpaned thread
to
> > > > another oilpaned thread but also a CrossThreadPersistent from an
> > non-oilpaned
> > > > thread to another oilpaned thread.
> > > > 
> > > > To make it workable, we need to make CrossThreadPersistent more
> thread-safe;
> > > > specifically:
> > > > 
> > > > (a) Acquire a global lock when the CrossThreadPersistent is created &
> > > > destructed.
> > > > 
> > > > (b) Acquire a gc lock when the CrossThreadPersistent is used (to avoid
> other
> > > > threads from mutating objects in a heap of another thread that is
> performing
> > a
> > > > GC).
> > > > 
> > > > (a) is already implemented. (b) is not yet implemented.
> > > > 
> > > > This implies the following fact about the safety of the
> > CrossThreadPersistent
> > > in
> > > > WebPrivatePtr:
> > > > 
> > > > - It is safe to create & destruct the WebPrivatePtr in different
threads.
> > The
> > > > different thread can be a non-oilpaned thread.
> > > > 
> > > > - It is unsafe to use the WebPrivatePtr in a non-oilpaned thread
(because
> > (b)
> > > is
> > > > not yet implemented).
> > > > 
> > > > I'm assuming that the above two facts hold in the current code base, but
> is
> > my
> > > > understanding correct? (It would be worth having an assert to verify
that
> a
> > > > CrossThreadPersistent is not used by a non-oilpaned thread.)
> > > 
> > > CrossThreadPersistent<T> intent was on passing destruction responsibilites
> > from
> > > one Oilpan thread to another Oilpan attached thread, at least that has
been
> my
> > > understanding all along & code comments suggest as much.
> > > 
> > > I'm wondering if (b) is in place already (cf tracePersistentNodes() for
the
> > > cross-thread case), but not at all sure if that's complete & sufficient.
> > > 
> > > Switching all WebPrivatePtr<>s to use CrossThreadPersistent<> is
excessive;
> > > there's two cases where we have cross-thread issues with Result values.
> Let's
> > > not enforce the overhead of handling them everywhere.
> > 
> > Sounds reasonable to me.
> > 
> > I guess it would be better to introduce CrossThreadWebPrivatePtr to make it
> > clear that it is holding a cross-thread thing.
> > 
> > - WebPrivatePtr holds a storage with a Persistent (in oilpan) and a RefPtr
to
> a
> > RefCounted object (in non-oilpan).
> > - CrossThreadWebPrivatePtr holds a storage with a CrossThreadPersistent (in
> > oilpan) and a RefPtr to a ThreadSafeRefCounted object (in non-oilpan).
> 
> Makes sense to push it down into the private pointer like that, but don't like
> the code duplication it would entail.
> 
> Would it be acceptable to make it an (optional) argument over WebPrivatePtr<>
> instead? (WebPrivatePtr<T, AllowCrossThreadDestruction>)

Sounds nice!

[sof, 2015-07-23 11:01:09]

On 2015/07/23 10:46:10, haraken wrote:
> On 2015/07/23 08:59:48, sof wrote:
...
> to
> > a
> > > RefCounted object (in non-oilpan).
> > > - CrossThreadWebPrivatePtr holds a storage with a CrossThreadPersistent
(in
> > > oilpan) and a RefPtr to a ThreadSafeRefCounted object (in non-oilpan).
> > 
> > Makes sense to push it down into the private pointer like that, but don't
like
> > the code duplication it would entail.
> > 
> > Would it be acceptable to make it an (optional) argument over
WebPrivatePtr<>
> > instead? (WebPrivatePtr<T, AllowCrossThreadDestruction>)
> 
> Sounds nice!

Updated WebPrivatePtr<> to do so.

Modulo getting that WebPrivatePtr<> change ready here, the question is if we're
prepared to allow non-Oilpan threads to release Persistent<>s.

Destructing CrossThreadPersistent<> is no different to
ThreadSafeRefCounted<>::deref() that results in same, apart from updating the
shared, cross-thread chain of persistents. The operations over that data
structure are all locked.

[haraken, 2015-07-23 11:44:11]

LGTM

On 2015/07/23 11:01:09, sof wrote:
> On 2015/07/23 10:46:10, haraken wrote:
> > On 2015/07/23 08:59:48, sof wrote:
> ...
> > to
> > > a
> > > > RefCounted object (in non-oilpan).
> > > > - CrossThreadWebPrivatePtr holds a storage with a CrossThreadPersistent
> (in
> > > > oilpan) and a RefPtr to a ThreadSafeRefCounted object (in non-oilpan).
> > > 
> > > Makes sense to push it down into the private pointer like that, but don't
> like
> > > the code duplication it would entail.
> > > 
> > > Would it be acceptable to make it an (optional) argument over
> WebPrivatePtr<>
> > > instead? (WebPrivatePtr<T, AllowCrossThreadDestruction>)
> > 
> > Sounds nice!
> 
> Updated WebPrivatePtr<> to do so.
> 
> Modulo getting that WebPrivatePtr<> change ready here, the question is if
we're
> prepared to allow non-Oilpan threads to release Persistent<>s.

You mean "to release CrossThreadPersistent<>s"?

Releasing Persistent<>s from non-oilpan threads isn't safe, but releasing
CrossThreadPersistent<>s should be safe because all methods of
CrossThreadPersistentRegion are protected with MutexLock.

> Destructing CrossThreadPersistent<> is no different to
> ThreadSafeRefCounted<>::deref() that results in same, apart from updating the
> shared, cross-thread chain of persistents. The operations over that data
> structure are all locked.

[sof, 2015-07-23 12:03:10]

sigbjornf@opera.com changed reviewers:
+ tkent@chromium.org

[sof, 2015-07-23 12:03:11]

haraken wrote:
>
...
> > Modulo getting that WebPrivatePtr<> change ready here, the question is if
we're
> > prepared to allow non-Oilpan threads to release Persistent<>s.
>
> You mean "to release CrossThreadPersistent<>s"?
> 
> Releasing Persistent<>s from non-oilpan threads isn't safe, but releasing
> CrossThreadPersistent<>s should be safe because all methods of
> CrossThreadPersistentRegion are protected with MutexLock.

Yes, sorry, the cross-thread variant is what it would have to be here, hope that
wasn't too unclear.

tkent@: WebPrivatePtr<> changes - willing to take a look?

[tkent, 2015-07-23 23:28:09]

lgtm

https://codereview.chromium.org/1249913002/diff/100001/public/platform/WebPri...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/1249913002/diff/100001/public/platform/WebPri...
public/platform/WebPrivatePtr.h:51: SameThreadDestruction,
The enum items should be  WebPrivatePtrDestructionSameThread and
WebPrivatePtrDestructionAllowCrossThread.

See the enum rule in https://www.chromium.org/blink/public-c-api#TOC-Naming .

[sof, 2015-07-24 05:25:22]

https://codereview.chromium.org/1249913002/diff/100001/public/platform/WebPri...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/1249913002/diff/100001/public/platform/WebPri...
public/platform/WebPrivatePtr.h:51: SameThreadDestruction,
On 2015/07/23 23:28:09, tkent wrote:
> The enum items should be  WebPrivatePtrDestructionSameThread and
> WebPrivatePtrDestructionAllowCrossThread.
> 
> See the enum rule in https://www.chromium.org/blink/public-c-api#TOC-Naming .

Thanks, aligned. And preferable to export & use this enum, rather than have it
be controlled internally to Blink.

[sof, 2015-07-24 05:25:35]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-07-24 05:25:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, xhwang@chromium.org, haraken@chromium.org,
tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1249913002/#ps120001
(title: "Follow enum tag naming scheme")

[commit-bot: I haz the power, 2015-07-24 05:25:50]

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

[commit-bot: I haz the power, 2015-07-24 07:38:22]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199421

[commit-bot: I haz the power, 2015-09-23 11:45:44]

Patchset 7 (id:??) landed as
https://crrev.com/106afb0edc37323671b75709967f463095b250f1