Safely iterate over MediaStreamSource observers.

Safely iterate over MediaStreamSource observers.

When changing the ready state of this object, the resultant dispatching of
events by its observers may extend the observer set. Take a snapshot of
the observers, so as to be able safely iterate over it across additions.

R=
BUG=602273
Committed: https://crrev.com/bc3ebdc42cd0b29fd770213d02a99a81b84dc69c
Cr-Commit-Position: refs/heads/master@{#387243}

[sof, 2016-04-13 20:23:59]

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

[sof, 2016-04-13 20:24:00]

please take a look.

[Guido Urdaneta, 2016-04-13 21:19:09]

lgtm

[haraken, 2016-04-14 00:46:46]

LGTM

[haraken, 2016-04-14 02:40:42]

Not related to this CL, would it be possible to add a runtime verification that
checks that a hash table is not modifiled while an iterator of the hash table is
on the stack?

[sof, 2016-04-14 05:19:36]

On 2016/04/14 02:40:42, haraken wrote:
> Not related to this CL, would it be possible to add a runtime verification
that
> checks that a hash table is not modifiled while an iterator of the hash table
is
> on the stack?

The ASan builds here are running without asserts enabled afaik, but this could
be addressed by adjusting the meaning of HashTable::m_accessForbidden.

We tried to use it for diagnosing some potential
access-table-while-deleting-bucket-entry bugs, but that didn't show up anything
(I need to double check details before saying for sure though.) It could be
repurposed, _if_ the current checking for table modifications in the iterator
isn't sufficient. It would give a "better" stack to catch out the hash table
modification itself.

But perhaps you had something else in mind? These kinds of bugs aren't the
easiest to pin point without asserts, so any improvement would be welcome. That
it lead to an ASan poisoned access of freed heap objects is worth keeping in
mind - not run into that before.

[sof, 2016-04-14 05:19:43]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-04-14 05:19:51]

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

[haraken, 2016-04-14 05:22:04]

On 2016/04/14 05:19:36, sof wrote:
> On 2016/04/14 02:40:42, haraken wrote:
> > Not related to this CL, would it be possible to add a runtime verification
> that
> > checks that a hash table is not modifiled while an iterator of the hash
table
> is
> > on the stack?
> 
> The ASan builds here are running without asserts enabled afaik, but this could
> be addressed by adjusting the meaning of HashTable::m_accessForbidden.
> 
> We tried to use it for diagnosing some potential
> access-table-while-deleting-bucket-entry bugs, but that didn't show up
anything
> (I need to double check details before saying for sure though.) It could be
> repurposed, _if_ the current checking for table modifications in the iterator
> isn't sufficient. It would give a "better" stack to catch out the hash table
> modification itself.

This is exactly what I have in mind :)

> 
> But perhaps you had something else in mind? These kinds of bugs aren't the
> easiest to pin point without asserts, so any improvement would be welcome.
That
> it lead to an ASan poisoned access of freed heap objects is worth keeping in
> mind - not run into that before.

[commit-bot: I haz the power, 2016-04-14 05:34:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-14 05:36:30]

Description was changed from

==========
Safely iterate over MediaStreamSource observers.

When changing the ready state of this object, the resultant dispatching of
events by its observers may extend the observer set. Take a snapshot of
the observers, so as to be able safely iterate over it across additions.

R=
BUG=602273
==========

to

==========
Safely iterate over MediaStreamSource observers.

When changing the ready state of this object, the resultant dispatching of
events by its observers may extend the observer set. Take a snapshot of
the observers, so as to be able safely iterate over it across additions.

R=
BUG=602273

Committed: https://crrev.com/bc3ebdc42cd0b29fd770213d02a99a81b84dc69c
Cr-Commit-Position: refs/heads/master@{#387243}
==========

[commit-bot: I haz the power, 2016-04-14 05:36:31]

Patchset 2 (id:??) landed as
https://crrev.com/bc3ebdc42cd0b29fd770213d02a99a81b84dc69c
Cr-Commit-Position: refs/heads/master@{#387243}

[sof, 2016-04-15 21:45:11]

On 2016/04/14 05:22:04, haraken wrote:
> On 2016/04/14 05:19:36, sof wrote:
> > On 2016/04/14 02:40:42, haraken wrote:
> > > Not related to this CL, would it be possible to add a runtime verification
> > that
> > > checks that a hash table is not modifiled while an iterator of the hash
> table
> > is
> > > on the stack?
> > 
> > The ASan builds here are running without asserts enabled afaik, but this
could
> > be addressed by adjusting the meaning of HashTable::m_accessForbidden.
> > 
> > We tried to use it for diagnosing some potential
> > access-table-while-deleting-bucket-entry bugs, but that didn't show up
> anything
> > (I need to double check details before saying for sure though.) It could be
> > repurposed, _if_ the current checking for table modifications in the
iterator
> > isn't sufficient. It would give a "better" stack to catch out the hash table
> > modification itself.
> 
> This is exactly what I have in mind :)
> 

I had a go, but while working on https://codereview.chromium.org/1892533003/ I
realized that using iterator stack lifetime to ban hash table modification
doesn't match well with how iterators are actually used (see CL description.)

i.e., it is not a bug to update a hash table while an iterator has access to
that hash table..it only becomes a bug when you attempt to access the iterator
again. Which is what the current modifications tracking achieves.