Oilpan: A thread that is not attached to Oilpan shouldn't create a CrossThreadPersistent

Oilpan: A thread that is not attached to Oilpan shouldn't create a CrossThreadPersistent

When the main thread passes a pointer of GarbageCollected object to a WebAudio/HRTFDatabase thread (via postTask), the WebAudio/HRTFDatabase thread creates a CrossThreadPersistent handle for the pointer. However, this is not allowed because the WebAudio/HRTFDatabase thread is not attached to Oilpan.

This CL introduces UnretainedPtr and uses it when posting a task to the WebAudio/HRTFDatabase thread. UnretainedPtr is a way to specify that the pointer doesn't need to be protected with a CrossThreadPersistent handle.

BUG=425306, 425768
TEST=None. The tests in the crash reports are not minimized.

[haraken, 2014-10-22 07:33:20]

haraken@chromium.org changed reviewers:
+ ager@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org

[haraken, 2014-10-22 07:33:28]

PTAL

[tkent, 2014-10-22 08:48:08]

Why don't we add RootsAccessor::Lock to each function updating
Persistent::m_raw?

[Mads Ager (chromium), 2014-10-22 09:13:52]

On 2014/10/22 08:48:08, tkent wrote:
> Why don't we add RootsAccessor::Lock to each function updating
> Persistent::m_raw?

Wouldn't that block the audio thread during GC (because the GC would need to
take that lock)?

[haraken, 2014-10-22 10:34:35]

On 2014/10/22 09:13:52, Mads Ager (chromium) wrote:
> On 2014/10/22 08:48:08, tkent wrote:
> > Why don't we add RootsAccessor::Lock to each function updating
> > Persistent::m_raw?
> 
> Wouldn't that block the audio thread during GC (because the GC would need to
> take that lock)?

Yeah, also I don't think it's a good idea to allow a non-oilpaned thread to
create a CrossThreadPersistent handle.

[tkent, 2014-10-23 01:09:50]

On 2014/10/22 09:13:52, Mads Ager (chromium) wrote:
> On 2014/10/22 08:48:08, tkent wrote:
> > Why don't we add RootsAccessor::Lock to each function updating
> > Persistent::m_raw?
> 
> Wouldn't that block the audio thread during GC (because the GC would need to
> take that lock)?

Ah, it would be a problem.  We should not block the audio thread with a mutex at
all.

[tkent, 2014-10-23 01:33:59]

How to avoid to garbage-collecting |this| after postTask() and before calling
the bound function?

[haraken, 2014-10-23 02:01:26]

On 2014/10/23 01:33:59, tkent wrote:
> How to avoid to garbage-collecting |this| after postTask() and before calling
> the bound function?

That's a responsibility of the caller side to keep |this| alive. (The fact that
the caller uses "UnretainedPtr" implies that the caller knows that the pointer
is OK to be not retained.)

For example, in case of HRTFDatabaseLoader, HRTFDatabaseLoader is kept alive
until the task is processed because the main thread waits for the task execution
in HRTFDatabaseLoader::waitForLoaderThreadCompletion.

[tkent, 2014-10-23 03:18:43]

On 2014/10/23 02:01:26, haraken wrote:
> On 2014/10/23 01:33:59, tkent wrote:
> > How to avoid to garbage-collecting |this| after postTask() and before
calling
> > the bound function?
> 
> That's a responsibility of the caller side to keep |this| alive. (The fact
that
> the caller uses "UnretainedPtr" implies that the caller knows that the pointer
> is OK to be not retained.)
> 
> For example, in case of HRTFDatabaseLoader, HRTFDatabaseLoader is kept alive
> until the task is processed because the main thread waits for the task
execution
> in HRTFDatabaseLoader::waitForLoaderThreadCompletion.

HRTFDatabaseLoader is not important.  We can add Oilpan support to the data
loading thread.

As for AudioScheduledSourceNode, OfflineAudioDestinationNode, and
ScriptProcessorNode, I'm not sure if we don't need to protect |this|.  I
remember some of them used to have ref()-deref() and |Persistent<...>
m_keepAlive|.

[haraken, 2014-10-23 04:01:26]

On 2014/10/23 03:18:43, tkent wrote:
> On 2014/10/23 02:01:26, haraken wrote:
> > On 2014/10/23 01:33:59, tkent wrote:
> > > How to avoid to garbage-collecting |this| after postTask() and before
> calling
> > > the bound function?
> > 
> > That's a responsibility of the caller side to keep |this| alive. (The fact
> that
> > the caller uses "UnretainedPtr" implies that the caller knows that the
pointer
> > is OK to be not retained.)
> > 
> > For example, in case of HRTFDatabaseLoader, HRTFDatabaseLoader is kept alive
> > until the task is processed because the main thread waits for the task
> execution
> > in HRTFDatabaseLoader::waitForLoaderThreadCompletion.
> 
> HRTFDatabaseLoader is not important.  We can add Oilpan support to the data
> loading thread.
> 
> As for AudioScheduledSourceNode, OfflineAudioDestinationNode, and
> ScriptProcessorNode, I'm not sure if we don't need to protect |this|.  I
> remember some of them used to have ref()-deref() and |Persistent<...>
> m_keepAlive|.

Hmm, good point. It's not clear to me either how the |this| pointer is
protected. Also this is not the only place where postTask(..., this) is used in
webaudio/. AudioScheduledSourceNode and OfflineAudioDestinationNode have the
same problem.

Instead of introducing UnretaindPtr, can we probably grab a RootsAccessor::Lock
when creating a CrossThreadPersistent if the thread is not attached to Oilpan?
(It won't cause dead lock.)