WebAudio: Fix AudioNode leak in a case that AudioNode is not disconnected from the graph explicitly.

WebAudio: Fix AudioNode leak in a case that AudioNode is not disconnected from the graph explicitly.

The main purpose of this CL is to introduce ThreadState::markAsZombie
and ThreadState::purifyZombies, and to apply them for WebAudio.
Objects marked as zombies are not finalized until purifyZombies is
called.

This CL also adds MarkingTasks, which enable to run tasks before/
after Oilpan marking. AudioContext implements MarkingTask in order to
call purifyZombie before marking, and simplify AudioContext::trace.


BUG=434136, 455993
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190366

[tkent, 2014-12-12 05:36:57]

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

[tkent, 2014-12-12 05:36:58]

I need to go out now.  I'll add more information to the description later.

[haraken, 2014-12-12 06:27:41]

I understand what this CL is doing, but it seems that the approach has some
fundamental issues.

- The zombie marking runs after threads are resumed. This is not safe because
the zombie marking can trace an object of another thread that is mutating the
object. Global marking is allowed only when all threads are in safe points.

- Neither weak processing nor prefinalizer runs after the zombie marking. This
will break the semantics of weak processing and prefinalizer.

I think I'm understanding the problem you want to solve, so let me think about
it a bit more.

[tkent, 2014-12-12 08:02:52]

On 2014/12/12 06:27:41, haraken wrote:
> I understand what this CL is doing, but it seems that the approach has some
> fundamental issues.
> 
> - The zombie marking runs after threads are resumed. This is not safe because
> the zombie marking can trace an object of another thread that is mutating the
> object. Global marking is allowed only when all threads are in safe points.

Right.  So, we should apply zombie to object graphs consisted of objects owned
by the thread.

> - Neither weak processing nor prefinalizer runs after the zombie marking. This
> will break the semantics of weak processing and prefinalizer.

It's intentional.  Zombie marking happens after weak processing and
prefinalizer.  We should not apply them to an object twice.

[haraken, 2014-12-15 01:25:13]

I'm getting convinced that something like the zombie you proposed is not
avoidable.

Here are a couple of suggestions to minimize the complexity added to the GC
infrastructure:

- Add visitor->resurrect(object). This API can be used in weak processing to
resurrect a dead object. We can weakly observe Audio nodes in m_referencingNodes
and resurrect the Audio node in weak processing. Then we won't need
registerZombie or MarkingTask.

- It is a responsibility for a mutator to keep the resurrected objects alive.
Specifically, the main thread should put the resurrected Audio nodes into a
HeapVector<Member<AudioNode>> to keep the objects alive. The main thread clears
the vector in AudioContext::deleteDisconnectedNodes(). Then we won't need
purifyZombie.

- To avoid this change from causing unexpected bugs in trunk, limit the object
type accepted by visitor->resurrect() to AudioNode. Specifically,
visitor->resurrect(AudioNode*).

- Implement something like ThreadLocalGC and add RELEASE_ASSERT() when the
thread-specific marking is going to enter an object graph owned by another
thread.

(That being said, this is still a very flimsy change. For example, weak
processing and/or pre-finalizers can be called for objects that are not actually
dead, because the objects can be resurrected in the thread-specific marking.)

[tkent, 2014-12-16 06:31:37]

On 2014/12/15 01:25:13, haraken wrote:
> - Add visitor->resurrect(object). This API can be used in weak processing to
> resurrect a dead object. We can weakly observe Audio nodes in
m_referencingNodes
> and resurrect the Audio node in weak processing. Then we won't need
> registerZombie or MarkingTask.

Do you intend that resurrect() traces the specified object and objects referred
by the specified object immediately, or stores the specified object to
thread-local storage and traces them later?

I added the explanation of MarkingTask to the CL description.  We can't remove
MarkingTask even with your idea.

[haraken, 2014-12-16 06:43:10]

On 2014/12/16 06:31:37, unavailable until Feburuary wrote:
> On 2014/12/15 01:25:13, haraken wrote:
> > - Add visitor->resurrect(object). This API can be used in weak processing to
> > resurrect a dead object. We can weakly observe Audio nodes in
> m_referencingNodes
> > and resurrect the Audio node in weak processing. Then we won't need
> > registerZombie or MarkingTask.
> 
> Do you intend that resurrect() traces the specified object and objects
referred
> by the specified object immediately, or stores the specified object to
> thread-local storage and traces them later?

I'm intending to trace all reachable objects from the object specified in
resurrect().

I'm just curious but is there any way to delay tracing the specified objects?

[tkent, 2014-12-16 06:53:09]

On 2014/12/16 06:43:10, haraken wrote:
> I'm intending to trace all reachable objects from the object specified in
> resurrect().

Is it safe to trace objects in weak processing?  I'm afraid a disaster happens
If an object has a WeakMember.

> I'm just curious but is there any way to delay tracing the specified objects?

I guess it's possible if we add a lot of code.

[haraken, 2014-12-16 08:06:41]

On 2014/12/16 06:53:09, unavailable until Feburuary wrote:
> On 2014/12/16 06:43:10, haraken wrote:
> > I'm intending to trace all reachable objects from the object specified in
> > resurrect().
> 
> Is it safe to trace objects in weak processing?  I'm afraid a disaster happens
> If an object has a WeakMember.

What I have in mind is:

1) A mutator calls object->resurrect() for objects that need to be resurrected.
The mutator puts the objects into HeapVector<Member> to keep the objects alive
in subsequent GCs.
2) Once all the weak processings are complete, ThreadState starts thread-local
marking from the resurrected objects.
3) The mutator clears the HeapVector<Member> once the objects get unnecessary.

Will it work?

[tkent, 2014-12-16 08:16:47]

On 2014/12/16 08:06:41, haraken wrote:
> 1) A mutator calls object->resurrect() for objects that need to be
resurrected.
> The mutator puts the objects into HeapVector<Member> to keep the objects alive
> in subsequent GCs.
> 2) Once all the weak processings are complete, ThreadState starts thread-local
> marking from the resurrected objects.

So, ThreadState::resurrect(object) would be much simpler than
Visitor::resurrect(object) because ThreadState needs to keep zombie list
temporarily.

> 3) The mutator clears the HeapVector<Member> once the objects get unnecessary.
> 
> Will it work?

I think so.

[haraken, 2014-12-16 08:28:36]

On 2014/12/16 08:16:47, unavailable until Feburuary wrote:
> On 2014/12/16 08:06:41, haraken wrote:
> > 1) A mutator calls object->resurrect() for objects that need to be
> resurrected.
> > The mutator puts the objects into HeapVector<Member> to keep the objects
alive
> > in subsequent GCs.
> > 2) Once all the weak processings are complete, ThreadState starts
thread-local
> > marking from the resurrected objects.
> 
> So, ThreadState::resurrect(object) would be much simpler than
> Visitor::resurrect(object) because ThreadState needs to keep zombie list
> temporarily.

Ah, got it! Agreed. ThreadState::resurrect(object) should be simpler, and I now
understand this is exactly what your original CL is trying to do.

[tkent, 2015-02-05 04:13:28]

On 2014/12/16 08:28:36, haraken wrote:
> On 2014/12/16 08:16:47, unavailable until Feburuary wrote:
> > On 2014/12/16 08:06:41, haraken wrote:
> > > 1) A mutator calls object->resurrect() for objects that need to be
> > resurrected.
> > > The mutator puts the objects into HeapVector<Member> to keep the objects
> alive
> > > in subsequent GCs.
> > > 2) Once all the weak processings are complete, ThreadState starts
> thread-local
> > > marking from the resurrected objects.
> > 
> > So, ThreadState::resurrect(object) would be much simpler than
> > Visitor::resurrect(object) because ThreadState needs to keep zombie list
> > temporarily.
> 
> Ah, got it! Agreed. ThreadState::resurrect(object) should be simpler, and I
now
> understand this is exactly what your original CL is trying to do.

Also, we can't update HeapVector in weak processing because it's in
NoAllocationScope.
So, the current TODO for this CL is only "thread-local tracing" asked in comment
#5.

[tkent, 2015-02-06 04:07:45]

Patchset #2 (id:20001) has been deleted

[tkent, 2015-02-06 04:12:40]

I have updated the patch.  Please take a look at it.

Updates:

* Add new thread-local marking mode which asserts foreign objects
  It uses ASSERT, not RELEASE_ASSERT because findPageFromAddress is not
available in Release build.
  I confirmed Oilpan Debug build didn't fail at the assertion.

* Do not use CrossThreadTask in AudioContext.  The new code is much simpler.

[haraken, 2015-02-06 07:57:23]

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

[haraken, 2015-02-06 07:57:24]

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:169:
ThreadState::current()->removeMarkingTask(this);

Can we move this to AudioContext::uninitialize()?

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:1165: void
AudioContext::willStartMarking(ThreadState& threadState)

willStartMarking is called not only in the ZombieMarking but also in a normal
global marking. Is it OK (if so, why)?

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:1167: lock();

What happens if a GC is triggered during AudioContext's constructor before the
lock gets initialized?

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:1170: if (m_lastZombie !=
m_lastRemovableZombie)

I don't quite get what m_lastZombie and m_lastRemovableZombie point to. Would
you elaborate?

[tkent, 2015-02-09 07:00:50]

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:169:
ThreadState::current()->removeMarkingTask(this);
On 2015/02/06 07:57:23, haraken wrote:
> 
> Can we move this to AudioContext::uninitialize()?

Yes.  It's consistent because we call addMarkingTask in initialize().  Done.

We need to call purifyZombies() before a marking after the last
AudioContext::handlePostRenderTasks().  uninitialize().  So, we also need to add
purifyZombies() to uninitialize().

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:1165: void
AudioContext::willStartMarking(ThreadState& threadState)
On 2015/02/06 07:57:23, haraken wrote:
> 
> willStartMarking is called not only in the ZombieMarking but also in a normal
> global marking. Is it OK (if so, why)?

It's ok.
There are two purposes of this hook:

* Simplify AudioContext::trace by calling lock()/unlock() in the task
  We need this for both of global marking and zombie marking.

* Calls purifyZombies() before zombie marking
  purifyZombies() on one of the markings is enough, and purifyZombies() on both
of markings is harmless.   The current code applies the latter for code
simplicity.

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:1167: lock();
On 2015/02/06 07:57:23, haraken wrote:
> 
> What happens if a GC is triggered during AudioContext's constructor before the
> lock gets initialized?

It won't happen.  This task is registered after the mutex initialization.

https://codereview.chromium.org/802593004/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioContext.cpp:1170: if (m_lastZombie !=
m_lastRemovableZombie)
On 2015/02/06 07:57:23, haraken wrote:
> 
> I don't quite get what m_lastZombie and m_lastRemovableZombie point to. Would
> you elaborate?

We need to avoid the following scenario:

1. GC happens
2. Some AudioNodes are unreachable, and their weak processing makes them
zombies.
3. Another GC happens.  We can't call purifyZombies() yet because
AudioContext::handlePostRenderTasks() is not called yet.
4. AudioContext::handlePostRenderTasks() is called on audio thread.  We may
purify all existing zombies, but we can't purify immediately because we're on
audio thread.
5. Another GC happens.  More AudioNodes are unreachable and their weak
processing adds new zombies.
6. Now, we can purify zombies made at step 2, but shouldn't purify zombies made
at step 5.

There are some ways to resolve this issue.  The current code would be the
simplest one.

[tkent, 2015-02-17 08:24:39]

rebased.

https://codereview.chromium.org/802593004/diff/100001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/802593004/diff/100001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:603: // Do not use this function.  This
feature is a temporal workaround for
Clarified "temporal workaround"

[haraken, 2015-02-17 08:37:03]

LGTM! Thanks for taking a lot of your time on this.

We discussed offline and concluded that it's hard to avoid this (scary) hack in
short term. In long term, we're planning to create a separate tree of AudioNodes
that can be exclusively touched by the audio thread. Then the
"AudioNode-sharing" problem will be gone and we can remove the hack added in
this CL.

We should seriously fix the problem because the existence of the zombie marking
can block our upcoming optimizations to the GC infrastructure.

https://codereview.chromium.org/802593004/diff/100001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/802593004/diff/100001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:707: HashSet<void*> m_zombies;

Another idea (that will reduce the complexity you need to add to the GC
infrastructure) would be to make the mutator keep the zombie objects alive.

- The responsibility of a GC is just to resurrect zombie objects in each marking
phase.
- The responsibility of a mutator is to hook postMarkingTasks and put the zombie
objects into PersistentVector<Member<AudioNode>>. The mutator drops the
AudioNode from the vector when it becomes really unnecessary.
- Then you need to remove m_zombies and the purifyZombie API from the GC
infrastructure.

That being said, this is a temporary hack and thus I don't much care about it.
If the current code is simpler overall, I'm fine with it.

[tkent, 2015-02-17 23:43:57]

https://codereview.chromium.org/802593004/diff/100001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/802593004/diff/100001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:707: HashSet<void*> m_zombies;
On 2015/02/17 08:37:03, haraken wrote:
> 
> Another idea (that will reduce the complexity you need to add to the GC
> infrastructure) would be to make the mutator keep the zombie objects alive.
> 
> - The responsibility of a GC is just to resurrect zombie objects in each
marking
> phase.
> - The responsibility of a mutator is to hook postMarkingTasks and put the
zombie
> objects into PersistentVector<Member<AudioNode>>. The mutator drops the
> AudioNode from the vector when it becomes really unnecessary.
> - Then you need to remove m_zombies and the purifyZombie API from the GC
> infrastructure.
> 
> That being said, this is a temporary hack and thus I don't much care about it.
> If the current code is simpler overall, I'm fine with it.

Your idea would be much complex.

- ThreadState needs to have a list of new zombies anyway until ZombieMarking is
done.
- We can't update PersistentVector<Member<AudioNode>> in weak processing because
we can't allocate new collection backing.

[tkent, 2015-02-17 23:44:01]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2015-02-17 23:44:35]

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

[commit-bot: I haz the power, 2015-02-17 23:48:22]

Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190366

[tkent, 2015-02-20 00:47:18]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/940963005/ by tkent@chromium.org.

The reason for reverting is: Caused multiple problems.  crbug.com/459605
crbug.com/459862 crbug.com/460254.