Do not use untraced members while notifying ActiveDOMObjects.

Do not use untraced members while notifying ActiveDOMObjects.

Iterating ActiveDOMObjects has long been a source of crashes (~M29).
Oilpan should have ameliorated these crashes yet they persist. One
possible reason is that the iteration uses untraced members; these do
not provide any guarantee that the object is alive. Note that
LifecycleObserver::m_observers are weak!

This consolidates the snapshot iteration code into one helper function
and uses Members while iterating over them.

BUG=451132

[dominicc (has gone to gerrit), 2016-06-24 05:54:56]

dominicc@chromium.org changed reviewers:
+ kochi@chromium.org

[dominicc (has gone to gerrit), 2016-06-24 05:54:57]

PTAL

[sof, 2016-06-24 05:57:58]

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

[sof, 2016-06-24 05:57:59]

https://codereview.chromium.org/2091023004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/2091023004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp:43:
HeapVector<Member<ContextLifecycleObserver>> snapshotOfObservers;
Could you explain why this is needed? We don't want to do unnecessary heap
allocation.

[haraken, 2016-06-24 06:11:46]

Pointers in on-stack HeapHashSets are automatically protected by Oilpan's
conservative tracing. So this CL should not be needed.

[dominicc (has gone to gerrit), 2016-06-24 07:38:07]

On 2016/06/24 at 06:11:46, haraken wrote:
> Pointers in on-stack HeapHashSets are automatically protected by Oilpan's
conservative tracing. So this CL should not be needed.

Where is the on-stack heap hash set you are referring to? :)

[haraken, 2016-06-24 07:39:49]

On 2016/06/24 07:38:07, dominicc wrote:
> On 2016/06/24 at 06:11:46, haraken wrote:
> > Pointers in on-stack HeapHashSets are automatically protected by Oilpan's
> conservative tracing. So this CL should not be needed.
> 
> Where is the on-stack heap hash set you are referring to? :)

m_observers. The iterator protects the heap hash set.

[sof, 2016-06-24 07:40:00]

On 2016/06/24 07:38:07, dominicc wrote:
> On 2016/06/24 at 06:11:46, haraken wrote:
> > Pointers in on-stack HeapHashSets are automatically protected by Oilpan's
> conservative tracing. So this CL should not be needed.
> 
> Where is the on-stack heap hash set you are referring to? :)

There is none, but the Vector<> of untraceds is checked wrt m_observers before
using, so what's the problem you're seeing & addressing?

[dominicc (has gone to gerrit), 2016-06-24 07:50:23]

On 2016/06/24 at 05:57:59, sigbjornf wrote:
>
https://codereview.chromium.org/2091023004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp (right):
> 
>
https://codereview.chromium.org/2091023004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp:43:
HeapVector<Member<ContextLifecycleObserver>> snapshotOfObservers;
> Could you explain why this is needed? We don't want to do unnecessary heap
allocation.

I believe this is already doing (non-Oilpan) heap allocation with the Vector; do
you mean Oilpan-heap allocation specifically?

Here's my thinking about why this is needed. It's probably wrong, so if you
could point out the wrong assumption that would help me understand Oilpan a bit
better...

UntracedMember says "it must be guaranteed that the pointing on-heap object is
kept alive while the raw pointer* is pointing to the object." [1] (* I think?
"raw pointer" is referring to the UntracedMember. Is that right?)

LifecycleNotifier's m_observers are WeakMembers, so there's nothing in this
class hierarchy that is keeping these observers alive.

So I thought we should ensure the lifetime of these objects while we're invoking
methods on them; it's a simple property we can reason about locally. What we
have relies on some combination of all suspend/resume/stop implementations not
causing garbage collection; not destroying the context they're observing; and
all ContextObserver implementations keep themselves alive as long as the context
observer; etc.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/...

[dominicc (has gone to gerrit), 2016-06-24 07:53:17]

On 2016/06/24 at 07:40:00, sigbjornf wrote:
> On 2016/06/24 07:38:07, dominicc wrote:
> > On 2016/06/24 at 06:11:46, haraken wrote:
> > > Pointers in on-stack HeapHashSets are automatically protected by Oilpan's
> > conservative tracing. So this CL should not be needed.
> > 
> > Where is the on-stack heap hash set you are referring to? :)
> 
> There is none, but the Vector<> of untraceds is checked wrt m_observers before
using, so what's the problem you're seeing & addressing?

As I understand it Oilpan doesn't reserve the addresses of dead objects; it
might reuse them. So that guarantee isn't complete.

The problem I'm seeing is the long history of crashes here. The status quo was
we're waiting to make ContextObservers GCed, which is what that FIXME is about.
And now they are, so we can clean up the FIXME.

[dominicc (has gone to gerrit), 2016-06-24 07:54:04]

On 2016/06/24 at 07:53:17, dominicc wrote:
> On 2016/06/24 at 07:40:00, sigbjornf wrote:
> > On 2016/06/24 07:38:07, dominicc wrote:
> > > On 2016/06/24 at 06:11:46, haraken wrote:
> > > > Pointers in on-stack HeapHashSets are automatically protected by
Oilpan's
> > > conservative tracing. So this CL should not be needed.
> > > 
> > > Where is the on-stack heap hash set you are referring to? :)
> > 
> > There is none, but the Vector<> of untraceds is checked wrt m_observers
before using, so what's the problem you're seeing & addressing?
> 
> As I understand it Oilpan doesn't reserve the addresses of dead objects; it
might reuse them. So that guarantee isn't complete.

Hmm, maybe it is complete given the prohibition against adding observers during
iteration...

> The problem I'm seeing is the long history of crashes here. The status quo was
we're waiting to make ContextObservers GCed, which is what that FIXME is about.
And now they are, so we can clean up the FIXME.

[sof, 2016-06-24 07:55:05]

On 2016/06/24 07:54:04, dominicc wrote:
> On 2016/06/24 at 07:53:17, dominicc wrote:
> > On 2016/06/24 at 07:40:00, sigbjornf wrote:
> > > On 2016/06/24 07:38:07, dominicc wrote:
> > > > On 2016/06/24 at 06:11:46, haraken wrote:
> > > > > Pointers in on-stack HeapHashSets are automatically protected by
> Oilpan's
> > > > conservative tracing. So this CL should not be needed.
> > > > 
> > > > Where is the on-stack heap hash set you are referring to? :)
> > > 
> > > There is none, but the Vector<> of untraceds is checked wrt m_observers
> before using, so what's the problem you're seeing & addressing?
> > 
> > As I understand it Oilpan doesn't reserve the addresses of dead objects; it
> might reuse them. So that guarantee isn't complete.
> 
> Hmm, maybe it is complete given the prohibition against adding observers
during
> iteration...
> 

Yes.

[dominicc (has gone to gerrit), 2016-06-24 08:00:46]

Why doesn't the comment in UntracedMember about the object needing to be
protected while it is used on the stack, *after* the call to
m_observers.contains(observer)? For example if there's GC during a call to
suspend and no Member or Persistent to "this", the stack walk will still save
the object?

[dominicc (has gone to gerrit), 2016-06-24 08:03:31]

On 2016/06/24 at 08:00:46, dominicc wrote:
> Why doesn't the comment in UntracedMember about the object needing to be
protected while it is used on the stack, *apply* *after* the call to
m_observers.contains(observer)? For example if there's GC during a call to
suspend and no Member or Persistent to "this", the stack walk will still save
the object?

*apply

[sof, 2016-06-24 08:14:26]

On 2016/06/24 08:03:31, dominicc wrote:
> On 2016/06/24 at 08:00:46, dominicc wrote:
> > Why doesn't the comment in UntracedMember about the object needing to be
> protected while it is used on the stack, *apply* *after* the call to
> m_observers.contains(observer)? For example if there's GC during a call to
> suspend and no Member or Persistent to "this", the stack walk will still save
> the object?
> 
> *apply

That's right - if a GC would happen at that point, it is a conservative one &
the stack (and register state) is scanned for roots. |observer| will then be
identified and traced (as it points to an object on an Oilpan heap page.)

[sof, 2016-06-25 17:53:01]

On 2016/06/24 07:53:17, dominicc wrote:
> On 2016/06/24 at 07:40:00, sigbjornf wrote:
> > On 2016/06/24 07:38:07, dominicc wrote:
> > > On 2016/06/24 at 06:11:46, haraken wrote:
> > > > Pointers in on-stack HeapHashSets are automatically protected by
Oilpan's
> > > conservative tracing. So this CL should not be needed.
> > > 
> > > Where is the on-stack heap hash set you are referring to? :)
> > 
> > There is none, but the Vector<> of untraceds is checked wrt m_observers
before
> using, so what's the problem you're seeing & addressing?
> 
> As I understand it Oilpan doesn't reserve the addresses of dead objects; it
> might reuse them. So that guarantee isn't complete.
> 
> The problem I'm seeing is the long history of crashes here. The status quo was
> we're waiting to make ContextObservers GCed, which is what that FIXME is
about.
> And now they are, so we can clean up the FIXME.

The FIXMEs in ContextLifecycleNotifier should be removed (similar ones have been
removed from LifecycleNotifier), or clarified along with rethinking the usage of
LifecycleObserver<T>::clearContext(). We don't disallow observers from
unregistering themselves (or objects they control) when e.g., getting a stop()
notification (or contextDestroyed()). AsyncCallTracker (which no longer exists)
in inspector code depended crucially on this, iirc -- but otherwise having code
distinguish between being stop()ed or stopping for other reasons (where you _do_
want to unregister), seems hard to get right.

But if someone can come up with ways to structure observers to allow
snapshot-free iteration, that'd be a clear win.

[sof, 2016-06-25 17:54:50]

If appropriate, could someone Cc: sigbjornf@ on issue 451132 ? tia.

[haraken, 2016-06-26 00:41:01]

On 2016/06/25 17:54:50, sof wrote:
> If appropriate, could someone Cc: sigbjornf@ on issue 451132 ? tia.

CCed.

[sof, 2016-06-26 06:57:19]

On 2016/06/26 00:41:01, haraken wrote:
> On 2016/06/25 17:54:50, sof wrote:
> > If appropriate, could someone Cc: sigbjornf@ on issue 451132 ? tia.
> 
> CCed.

Thanks; https://codereview.chromium.org/2094143002/ avoids snapshotting via
swapping - it might be preferable.

[sof, 2016-06-26 18:39:38]

On 2016/06/26 06:57:19, sof wrote:
> On 2016/06/26 00:41:01, haraken wrote:
> > On 2016/06/25 17:54:50, sof wrote:
> > > If appropriate, could someone Cc: sigbjornf@ on issue 451132 ? tia.
> > 
> > CCed.
> 
> Thanks; https://codereview.chromium.org/2094143002/ avoids snapshotting via
> swapping - it might be preferable.

I think it is ready now; context lifecycle observers are better wrt mutation
during iteration than I previously thought.