Avoid snapshotting ContextLifecycleObservers when iterating.

Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
a snapshot of the set was taken before iterating over it. So as to
allow observers to unregister themselves while being notified.

Apart from PostMessageTimer unregistering itself while being stop()ed,
the ContextLifecycleObservers do not mutate the observer set, hence
we can avoid the snapshot step and iterate directly over the observers.
Attempts to remove an observer while iterating is caught and
asserted for.

As the observer set is a set of weak references, some care is needed
to keep those references strong while iterating. That and other details
surrounding observer iteration is now handled by the auxiliary scope object
LifecycleNotifier<>::IterationScope.

Should the constraint of not being allowed to remove observers while
iterating prove too cumbersome, supporting lazy removal of observers
(post iteration) would be straightforward.

R=
BUG=451132
Committed: https://crrev.com/5b0b4e4f357e9348870dd5612b663a35894b49f3
Cr-Commit-Position: refs/heads/master@{#402141}

[sof, 2016-06-26 06:53:27]

Description was changed from

==========
Avoid snapshotting ContextLifecycleObserver when iterating.

To allow safe iteration over the set of ExecutionContext observers,
the set might be mutated while iterating, a snapshot is initially
taken, and iteration happens over it.

Avoid creating the snapshot by swapping out the observer set
while iterating, restoring it on completion. To correctly handle
any removals that are performed while iterating, we instead
record those removals and remove them from the observer set
at the end.

As an experiment, we do not perform that last step while
notifying observers that the ExecutionContext has entered a
stopped state.

R=
BUG=
==========

to

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
the set might be mutated while iterating, a snapshot is initially
taken, and iteration happens over it.

Avoid creating the snapshot by swapping out the observer set
while iterating, restoring it on completion. To correctly handle
any removals that are performed while iterating, we instead
record those removals and remove them from the observer set
at the end.

As an experiment, we do not perform that last step while
notifying observers that the ExecutionContext has entered a
stopped state.

R=
BUG=
==========

[sof, 2016-06-26 08:08:21]

Description was changed from

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
the set might be mutated while iterating, a snapshot is initially
taken, and iteration happens over it.

Avoid creating the snapshot by swapping out the observer set
while iterating, restoring it on completion. To correctly handle
any removals that are performed while iterating, we instead
record those removals and remove them from the observer set
at the end.

As an experiment, we do not perform that last step while
notifying observers that the ExecutionContext has entered a
stopped state.

R=
BUG=
==========

to

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
the set might be mutated while iterating, a snapshot is initially
taken, and iteration happens over it.

Avoid creating the snapshot by swapping out the observer set
while iterating, restoring it on completion. To correctly handle
any removals that are performed while iterating, we instead
record those removals and remove them from the observer set
at the end.

R=
BUG=
==========

[sof, 2016-06-26 10:06:46]

Description was changed from

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
the set might be mutated while iterating, a snapshot is initially
taken, and iteration happens over it.

Avoid creating the snapshot by swapping out the observer set
while iterating, restoring it on completion. To correctly handle
any removals that are performed while iterating, we instead
record those removals and remove them from the observer set
at the end.

R=
BUG=
==========

to

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
the set might be mutated while iterating, a snapshot is initially
taken and iteration happens over that snapshot.

Avoid creating the snapshot by swapping out the observer set
while iterating, restoring it on completion. Should an observer
attempt to remove itself while being notified, this is now disallowed.
The codebase appears ready for such a strict regime, but support
for removals-while-iterating could be added should that prove to
be too constraining.

R=
BUG=
==========

[sof, 2016-06-26 10:53:00]

Patchset #6 (id:100001) has been deleted

[sof, 2016-06-26 11:12:21]

Patchset #6 (id:120001) has been deleted

[sof, 2016-06-26 16:44:03]

Patchset #6 (id:140001) has been deleted

[sof, 2016-06-26 16:44:16]

Patchset #6 (id:160001) has been deleted

[sof, 2016-06-26 16:44:25]

Patchset #5 (id:80001) has been deleted

[sof, 2016-06-26 16:57:10]

Description was changed from

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
the set might be mutated while iterating, a snapshot is initially
taken and iteration happens over that snapshot.

Avoid creating the snapshot by swapping out the observer set
while iterating, restoring it on completion. Should an observer
attempt to remove itself while being notified, this is now disallowed.
The codebase appears ready for such a strict regime, but support
for removals-while-iterating could be added should that prove to
be too constraining.

R=
BUG=
==========

to

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
a snapshot of the set was taken before iterating over it. So as to
allow observers to unregister themselves while being notified.

Apart from PostMessageTimer unregistering itself while being stop()ed,
the ContextLifecycleObservers do not mutate the observer set, hence
we can avoid the snapshot step and iterate directly over the observers.
Attempts to remove an observer while iterating is caught and
asserted for.

As the observer set is a set of weak references, some care is needed
to keep those references strong while iterating. That and other details
surrounding observer iteration is now handled by the auxiliary scope object
LifecycleNotifier<>::IterationScope.

Should the constraint of not being allowed to remove observers while
iterating prove too cumbersome, supporting lazy removal of observers
(post iteration) would be straightforward.

R=
BUG=
==========

[sof, 2016-06-26 16:57:32]

Patchset #5 (id:180001) has been deleted

[sof, 2016-06-26 17:00:45]

Description was changed from

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
a snapshot of the set was taken before iterating over it. So as to
allow observers to unregister themselves while being notified.

Apart from PostMessageTimer unregistering itself while being stop()ed,
the ContextLifecycleObservers do not mutate the observer set, hence
we can avoid the snapshot step and iterate directly over the observers.
Attempts to remove an observer while iterating is caught and
asserted for.

As the observer set is a set of weak references, some care is needed
to keep those references strong while iterating. That and other details
surrounding observer iteration is now handled by the auxiliary scope object
LifecycleNotifier<>::IterationScope.

Should the constraint of not being allowed to remove observers while
iterating prove too cumbersome, supporting lazy removal of observers
(post iteration) would be straightforward.

R=
BUG=
==========

to

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
a snapshot of the set was taken before iterating over it. So as to
allow observers to unregister themselves while being notified.

Apart from PostMessageTimer unregistering itself while being stop()ed,
the ContextLifecycleObservers do not mutate the observer set, hence
we can avoid the snapshot step and iterate directly over the observers.
Attempts to remove an observer while iterating is caught and
asserted for.

As the observer set is a set of weak references, some care is needed
to keep those references strong while iterating. That and other details
surrounding observer iteration is now handled by the auxiliary scope object
LifecycleNotifier<>::IterationScope.

Should the constraint of not being allowed to remove observers while
iterating prove too cumbersome, supporting lazy removal of observers
(post iteration) would be straightforward.

R=
BUG=451132
==========

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

sigbjornf@opera.com changed reviewers:
+ dominicc@chromium.org, haraken@chromium.org

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

please take a look.

[dominicc (has gone to gerrit), 2016-06-27 00:44:00]

Thank you for taking over https://codereview.chromium.org/2091023004 and
cleaning up these FIXMEs?

Are you now disallowing removal during ActiveDOMObject iteration where that was
allowed before? Is that feasible?

Could you clean up the duplication in the ActiveDOMObject notification? I think
all of those methods are the same except for the call to the ActiveDOMObject.

[haraken, 2016-06-27 01:06:34]

LGTM with comments.

https://codereview.chromium.org/2094143002/diff/170007/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/LifecycleNotifier.h (right):

https://codereview.chromium.org/2094143002/diff/170007/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/LifecycleNotifier.h:80: explicit
IterationScope(LifecycleNotifier* notifier, IterationState allowable =
AllowingNone)

Is the second parameter used somewhere?

https://codereview.chromium.org/2094143002/diff/170007/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/LifecycleNotifier.h:93:
m_observers.swap(m_notifier->m_observers);

I don't fully understand why we need to explicitly swap m_observers. The caller
side holds an iterator while iterating the hash set. Wouldn't it be enough to
keep the hash set alive?

[sof, 2016-06-27 05:14:15]

https://codereview.chromium.org/2094143002/diff/170007/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/LifecycleNotifier.h (right):

https://codereview.chromium.org/2094143002/diff/170007/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/LifecycleNotifier.h:80: explicit
IterationScope(LifecycleNotifier* notifier, IterationState allowable =
AllowingNone)
On 2016/06/27 01:06:34, haraken wrote:
> 
> Is the second parameter used somewhere?

Line 85

https://codereview.chromium.org/2094143002/diff/170007/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/LifecycleNotifier.h:93:
m_observers.swap(m_notifier->m_observers);
On 2016/06/27 01:06:34, haraken wrote:
> 
> I don't fully understand why we need to explicitly swap m_observers. The
caller
> side holds an iterator while iterating the hash set. Wouldn't it be enough to
> keep the hash set alive?
> 

I was thinking about that overnight, this being too conservative. And looking at
HeapTest.HeapWeakCollectionTypes, we can rely on iterators strongifying the
collection. So let me simplify away IterationScope.

[sof, 2016-06-27 06:34:13]

On 2016/06/27 00:44:00, dominicc wrote:
> Thank you for taking over https://codereview.chromium.org/2091023004 and
> cleaning up these FIXMEs?
> 

I didn't consider the FIXMEs practical, but I'm now convinced that they are.

> Are you now disallowing removal during ActiveDOMObject iteration where that
was
> allowed before? Is that feasible?

Yes, apart from PostMessageTimer, the codebase does seem ready -- going through
the other stop() overrides, I could only find local operations being performed,
and no mutation of the notifier. So I think we should try to disallow removals -
if that proves a step too far on the web, ps#2 is one way to allow removals
while still avoiding snapshotting.

> 
> Could you clean up the duplication in the ActiveDOMObject notification? I
think
> all of those methods are the same except for the call to the ActiveDOMObject.

I'm not planning on factoring out those smaller loop bodies & provide an
ActiveDOMObject-only iterator, at least not here.

[sof, 2016-06-27 06:36:54]

Suggestions welcome for alternate naming of the IterationState enum tags.

https://codereview.chromium.org/2094143002/diff/170007/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/LifecycleNotifier.h (right):

https://codereview.chromium.org/2094143002/diff/170007/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/LifecycleNotifier.h:93:
m_observers.swap(m_notifier->m_observers);
On 2016/06/27 05:14:14, sof wrote:
> On 2016/06/27 01:06:34, haraken wrote:
> > 
> > I don't fully understand why we need to explicitly swap m_observers. The
> caller
> > side holds an iterator while iterating the hash set. Wouldn't it be enough
to
> > keep the hash set alive?
> > 
> 
> I was thinking about that overnight, this being too conservative. And looking
at
> HeapTest.HeapWeakCollectionTypes, we can rely on iterators strongifying the
> collection. So let me simplify away IterationScope.

Now done.

[haraken, 2016-06-27 06:37:32]

LGTM

[sof, 2016-06-27 08:05:32]

Sorry for jumping in a bit on dominicc@'s efforts, but the possibility of
removing all allocations during these iteration steps proved too tempting.

I don't know if it would explain issue 451132, OOM avoidance would be the only
reason why.(?)

[sof, 2016-06-27 08:05:53]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-06-27 08:06:03]

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-06-27 09:02:56]

Description was changed from

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
a snapshot of the set was taken before iterating over it. So as to
allow observers to unregister themselves while being notified.

Apart from PostMessageTimer unregistering itself while being stop()ed,
the ContextLifecycleObservers do not mutate the observer set, hence
we can avoid the snapshot step and iterate directly over the observers.
Attempts to remove an observer while iterating is caught and
asserted for.

As the observer set is a set of weak references, some care is needed
to keep those references strong while iterating. That and other details
surrounding observer iteration is now handled by the auxiliary scope object
LifecycleNotifier<>::IterationScope.

Should the constraint of not being allowed to remove observers while
iterating prove too cumbersome, supporting lazy removal of observers
(post iteration) would be straightforward.

R=
BUG=451132
==========

to

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
a snapshot of the set was taken before iterating over it. So as to
allow observers to unregister themselves while being notified.

Apart from PostMessageTimer unregistering itself while being stop()ed,
the ContextLifecycleObservers do not mutate the observer set, hence
we can avoid the snapshot step and iterate directly over the observers.
Attempts to remove an observer while iterating is caught and
asserted for.

As the observer set is a set of weak references, some care is needed
to keep those references strong while iterating. That and other details
surrounding observer iteration is now handled by the auxiliary scope object
LifecycleNotifier<>::IterationScope.

Should the constraint of not being allowed to remove observers while
iterating prove too cumbersome, supporting lazy removal of observers
(post iteration) would be straightforward.

R=
BUG=451132
==========

[commit-bot: I haz the power, 2016-06-27 09:02:57]

Committed patchset #7 (id:230001)

[commit-bot: I haz the power, 2016-06-27 09:04:56]

Description was changed from

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
a snapshot of the set was taken before iterating over it. So as to
allow observers to unregister themselves while being notified.

Apart from PostMessageTimer unregistering itself while being stop()ed,
the ContextLifecycleObservers do not mutate the observer set, hence
we can avoid the snapshot step and iterate directly over the observers.
Attempts to remove an observer while iterating is caught and
asserted for.

As the observer set is a set of weak references, some care is needed
to keep those references strong while iterating. That and other details
surrounding observer iteration is now handled by the auxiliary scope object
LifecycleNotifier<>::IterationScope.

Should the constraint of not being allowed to remove observers while
iterating prove too cumbersome, supporting lazy removal of observers
(post iteration) would be straightforward.

R=
BUG=451132
==========

to

==========
Avoid snapshotting ContextLifecycleObservers when iterating.

To allow safe iteration over the set of ExecutionContext observers,
a snapshot of the set was taken before iterating over it. So as to
allow observers to unregister themselves while being notified.

Apart from PostMessageTimer unregistering itself while being stop()ed,
the ContextLifecycleObservers do not mutate the observer set, hence
we can avoid the snapshot step and iterate directly over the observers.
Attempts to remove an observer while iterating is caught and
asserted for.

As the observer set is a set of weak references, some care is needed
to keep those references strong while iterating. That and other details
surrounding observer iteration is now handled by the auxiliary scope object
LifecycleNotifier<>::IterationScope.

Should the constraint of not being allowed to remove observers while
iterating prove too cumbersome, supporting lazy removal of observers
(post iteration) would be straightforward.

R=
BUG=451132

Committed: https://crrev.com/5b0b4e4f357e9348870dd5612b663a35894b49f3
Cr-Commit-Position: refs/heads/master@{#402141}
==========

[commit-bot: I haz the power, 2016-06-27 09:04:57]

Patchset 7 (id:??) landed as
https://crrev.com/5b0b4e4f357e9348870dd5612b663a35894b49f3
Cr-Commit-Position: refs/heads/master@{#402141}