Remove unwanted eager finalization of some mediastream objects.

Remove unwanted eager finalization of some mediastream objects.

Adjust the use of EAGERLY_FINALIZE() for mediastream objects:

 - MediaStreamComponent will be eagerly finalized and notify its
   MediaStreamTrack observers. Consequently, MediaStreamTrack cannot
   and need not be eagerly finalized.
 - MediaStream no longer need to eagerly unregister as a client from
   its MediaStreamDescriptor member.

Not doing so results in mutual dependencies between finalizers run by
eagerly finalized objects, causing use-after-frees.

R=haraken
BUG=496535
Committed: https://crrev.com/32d0f75fd0961f4889eee472f8d80f57d1cb11c3
Cr-Commit-Position: refs/heads/master@{#358062}

[sof, 2015-11-04 15:56:45]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-04 15:56:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421373007/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421373007/1

[sof, 2015-11-04 16:04:01]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-11-04 16:04:02]

please take a look.

Addressing the flaky crashers

 
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...

I suspect it might address crbug.com/541338 also.

[sof, 2015-11-04 16:54:51]

Description was changed from

==========
Drop unnecessary and harmful eager finalization of mediastream objects.

Adjust the use of EAGERLY_FINALIZE() for mediastream objects:

 - MediaStreamComponent will be eagerly finalized and notify its
   MediaStreamTrack observers. Consequently, MediaStreamTrack cannot
   and need not be eagerly finalized.
 - MediaStream no longer need to eagerly unregister as a client from
   its MediaStreamDescriptor member.

Not doing so results in mutual dependencies between finalizers run by
eagerly finalized objects, causing already finalized objects being
accessed.

R=
BUG=496535
==========

to

==========
Drop unnecessary and harmful eager finalization of mediastream objects.

Adjust the use of EAGERLY_FINALIZE() for mediastream objects:

 - MediaStreamComponent will be eagerly finalized and notify its
   MediaStreamTrack observers. Consequently, MediaStreamTrack cannot
   and need not be eagerly finalized.
 - MediaStream no longer need to eagerly unregister as a client from
   its MediaStreamDescriptor member.

Not doing so results in mutual dependencies between finalizers run by
eagerly finalized objects, causing use-after-frees.

R=
BUG=496535
==========

[commit-bot: I haz the power, 2015-11-04 17:33:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-04 17:33:19]

Dry run: This issue passed the CQ dry run.

[sof, 2015-11-04 20:39:50]

Description was changed from

==========
Drop unnecessary and harmful eager finalization of mediastream objects.

Adjust the use of EAGERLY_FINALIZE() for mediastream objects:

 - MediaStreamComponent will be eagerly finalized and notify its
   MediaStreamTrack observers. Consequently, MediaStreamTrack cannot
   and need not be eagerly finalized.
 - MediaStream no longer need to eagerly unregister as a client from
   its MediaStreamDescriptor member.

Not doing so results in mutual dependencies between finalizers run by
eagerly finalized objects, causing use-after-frees.

R=
BUG=496535
==========

to

==========
Remove unwanted eager finalization of some mediastream objects.

Adjust the use of EAGERLY_FINALIZE() for mediastream objects:

 - MediaStreamComponent will be eagerly finalized and notify its
   MediaStreamTrack observers. Consequently, MediaStreamTrack cannot
   and need not be eagerly finalized.
 - MediaStream no longer need to eagerly unregister as a client from
   its MediaStreamDescriptor member.

Not doing so results in mutual dependencies between finalizers run by
eagerly finalized objects, causing use-after-frees.

R=
BUG=496535
==========

[peria, 2015-11-04 23:47:04]

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

[peria, 2015-11-04 23:47:04]

The crash of crbug.com/541338 happens in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
so we have to consider the dtor of MediaStreamComponent indirectly touches also
on MediaStreamDescriptor.

[haraken, 2015-11-05 00:07:51]

On 2015/11/04 23:47:04, peria wrote:
> The crash of crbug.com/541338 happens in
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> so we have to consider the dtor of MediaStreamComponent indirectly touches
also
> on MediaStreamDescriptor.

For safety, I'd propose to add pre-finalizer to both MediaStreamComponent and
MediaStreamDescriptor and clear the ExtraData (and thus the raw pointer held by
the ExtraData) in the pre-finalizer.

[sof, 2015-11-05 06:40:59]

On 2015/11/05 00:07:51, haraken wrote:
> On 2015/11/04 23:47:04, peria wrote:
> > The crash of crbug.com/541338 happens in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > so we have to consider the dtor of MediaStreamComponent indirectly touches
> also
> > on MediaStreamDescriptor.
> 
> For safety, I'd propose to add pre-finalizer to both MediaStreamComponent and
> MediaStreamDescriptor and clear the ExtraData (and thus the raw pointer held
by
> the ExtraData) in the pre-finalizer.

It doesn't solve anything, afaict.

[haraken, 2015-11-05 09:32:14]

On 2015/11/05 06:40:59, sof wrote:
> On 2015/11/05 00:07:51, haraken wrote:
> > On 2015/11/04 23:47:04, peria wrote:
> > > The crash of crbug.com/541338 happens in
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > so we have to consider the dtor of MediaStreamComponent indirectly touches
> > also
> > > on MediaStreamDescriptor.
> > 
> > For safety, I'd propose to add pre-finalizer to both MediaStreamComponent
and
> > MediaStreamDescriptor and clear the ExtraData (and thus the raw pointer held
> by
> > the ExtraData) in the pre-finalizer.
> 
> It doesn't solve anything, afaict.

Sorry, my explanation was not correct.

I think the root cause of bug 541338 is that WebMediaStreamSource::m_owner is
left as a stale pointer
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
We need to run a pre-finalizer to clear the m_owner when MediaStreamSource gets
destructed.

Either way, this CL LGTM.

[sof, 2015-11-05 15:51:01]

On 2015/11/05 09:32:14, haraken wrote:
> On 2015/11/05 06:40:59, sof wrote:
> > On 2015/11/05 00:07:51, haraken wrote:
> > > On 2015/11/04 23:47:04, peria wrote:
> > > > The crash of crbug.com/541338 happens in
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > so we have to consider the dtor of MediaStreamComponent indirectly
touches
> > > also
> > > > on MediaStreamDescriptor.
> > > 
> > > For safety, I'd propose to add pre-finalizer to both MediaStreamComponent
> and
> > > MediaStreamDescriptor and clear the ExtraData (and thus the raw pointer
held
> > by
> > > the ExtraData) in the pre-finalizer.
> > 
> > It doesn't solve anything, afaict.
> 
> Sorry, my explanation was not correct.
> 
> I think the root cause of bug 541338 is that WebMediaStreamSource::m_owner is
> left as a stale pointer
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> We need to run a pre-finalizer to clear the m_owner when MediaStreamSource
gets
> destructed.
> 
> Either way, this CL LGTM.

I agree, the finalization details here are separate to 541338's. But a tangled
web it is.

[sof, 2015-11-05 15:51:15]

Description was changed from

==========
Remove unwanted eager finalization of some mediastream objects.

Adjust the use of EAGERLY_FINALIZE() for mediastream objects:

 - MediaStreamComponent will be eagerly finalized and notify its
   MediaStreamTrack observers. Consequently, MediaStreamTrack cannot
   and need not be eagerly finalized.
 - MediaStream no longer need to eagerly unregister as a client from
   its MediaStreamDescriptor member.

Not doing so results in mutual dependencies between finalizers run by
eagerly finalized objects, causing use-after-frees.

R=
BUG=496535
==========

to

==========
Remove unwanted eager finalization of some mediastream objects.

Adjust the use of EAGERLY_FINALIZE() for mediastream objects:

 - MediaStreamComponent will be eagerly finalized and notify its
   MediaStreamTrack observers. Consequently, MediaStreamTrack cannot
   and need not be eagerly finalized.
 - MediaStream no longer need to eagerly unregister as a client from
   its MediaStreamDescriptor member.

Not doing so results in mutual dependencies between finalizers run by
eagerly finalized objects, causing use-after-frees.

R=haraken
BUG=496535
==========

[sof, 2015-11-05 15:51:23]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-05 15:51:44]

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

[commit-bot: I haz the power, 2015-11-05 15:56:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-11-05 15:57:06]

Patchset 1 (id:??) landed as
https://crrev.com/32d0f75fd0961f4889eee472f8d80f57d1cb11c3
Cr-Commit-Position: refs/heads/master@{#358062}