[Oilpan] Move MediaStream{Source|Component|Descriptor} to Oilpan heap

Move MediaStream{Source|Component|Descriptor} to Oilpan heap

BUG=496535
Committed: https://crrev.com/622d290d58d54d6c1410d19ecb88b5c2dd2d135d
Cr-Commit-Position: refs/heads/master@{#352008}

[peria, 2015-09-30 02:18:16]

peria@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[peria, 2015-09-30 02:18:17]

PTL.

This CL should be same with [1], and the problem which
reverted [1] is expected to be resolved by [2].

For now, I'd like you to review for style.
(This CL must fail with ASAN build until [2] will be landed.)
After [2] is landed, I will run tests again and offer your
approves for committing.


[1] http://crrev.com/552653005
[2] http://crrev.com/1375873002

[sof, 2015-09-30 06:26:33]

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

[sof, 2015-09-30 06:26:35]

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:217:
MediaStreamCenter::instance().didAddMediaStreamTrack(m_descriptor.get(),
track->component());
redundant get()

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:251:
MediaStreamCenter::instance().didRemoveMediaStreamTrack(m_descriptor.get(),
track->component());
redundant get()

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/MediaStreamRegistry.h
(right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStreamRegistry.h:32: #include
"wtf/PassRefPtr.h"
While here, let's remove this.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:210: return
m_component.get();
redundant get() (and can be moved to the class decl?)

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/RTCStatsRequestImpl.cpp
(right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/RTCStatsRequestImpl.cpp:66: return
m_component.get();
redundant get()

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/UserMediaRequest.h:42: #include
"wtf/PassRefPtr.h"
Can be removed.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h
(right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h:98:
MediaStreamDescriptorClient* m_client;
Can you add a comment clarifying lifetime assumptions for m_client ? In
particular, when setClient(nullptr) will be called upon teardown.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:49: class
PLATFORM_EXPORT Observer {
If the Observers are on the heap, shouldn't this be a GC mixin?

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
Vector<Observer*> m_observers;
Are these bare pointers referring to heap objects?

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMediaStreamSource.h:122:
BLINK_PLATFORM_EXPORT WebMediaStreamSource(MediaStreamSource*);
nit: should this be an implicit conversion, or explicit?

[haraken, 2015-09-30 07:18:20]

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

[haraken, 2015-09-30 07:18:21]

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
Vector<Observer*> m_observers;
On 2015/09/30 06:26:35, sof wrote:
> Are these bare pointers referring to heap objects?

The GC plugin should be able to catch this error. yuta-san?

[Yuta Kitamura, 2015-09-30 07:25:09]

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
Vector<Observer*> m_observers;
On 2015/09/30 at 07:18:20, haraken wrote:
> On 2015/09/30 06:26:35, sof wrote:
> > Are these bare pointers referring to heap objects?
> 
> The GC plugin should be able to catch this error. yuta-san?

In the current code, Observer isn't a heap-managed class. That's
not BlinkGCPlugin's fault.

[haraken, 2015-09-30 07:35:32]

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
Vector<Observer*> m_observers;
On 2015/09/30 07:25:09, Yuta Kitamura wrote:
> On 2015/09/30 at 07:18:20, haraken wrote:
> > On 2015/09/30 06:26:35, sof wrote:
> > > Are these bare pointers referring to heap objects?
> > 
> > The GC plugin should be able to catch this error. yuta-san?
> 
> In the current code, Observer isn't a heap-managed class. That's
> not BlinkGCPlugin's fault.

Would it be possible to catch the following case?

class X : public GarbageCollected<X>, public Y { };

class Y { };

class Z : public GarbageCollected<Y> {
  Y* m_y;  // Y should be a GCMixin. This should be Member<Y>.
  Vector<Y*> m_vec;  // Y should be a GCMixin. This should be
HeapVector<Member<Y>>.
};

[peria, 2015-09-30 07:50:05]

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:217:
MediaStreamCenter::instance().didAddMediaStreamTrack(m_descriptor.get(),
track->component());
On 2015/09/30 06:26:34, sof wrote:
> redundant get()

Done.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:251:
MediaStreamCenter::instance().didRemoveMediaStreamTrack(m_descriptor.get(),
track->component());
On 2015/09/30 06:26:34, sof wrote:
> redundant get()

Done.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/MediaStreamRegistry.h
(right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStreamRegistry.h:32: #include
"wtf/PassRefPtr.h"
On 2015/09/30 06:26:34, sof wrote:
> While here, let's remove this.

Done.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:210: return
m_component.get();
On 2015/09/30 06:26:34, sof wrote:
> redundant get() (and can be moved to the class decl?)

Done.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/RTCStatsRequestImpl.cpp
(right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/RTCStatsRequestImpl.cpp:66: return
m_component.get();
On 2015/09/30 06:26:34, sof wrote:
> redundant get()

Done.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/UserMediaRequest.h:42: #include
"wtf/PassRefPtr.h"
On 2015/09/30 06:26:34, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h
(right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h:98:
MediaStreamDescriptorClient* m_client;
On 2015/09/30 06:26:34, sof wrote:
> Can you add a comment clarifying lifetime assumptions for m_client ? In
> particular, when setClient(nullptr) will be called upon teardown.

Done.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:49: class
PLATFORM_EXPORT Observer {
On 2015/09/30 06:26:35, sof wrote:
> If the Observers are on the heap, shouldn't this be a GC mixin?

Done.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
Vector<Observer*> m_observers;
On 2015/09/30 06:26:35, sof wrote:
> Are these bare pointers referring to heap objects?

Done.
Just a note to work for this Vector<>.

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMediaStreamSource.h:122:
BLINK_PLATFORM_EXPORT WebMediaStreamSource(MediaStreamSource*);
On 2015/09/30 06:26:35, sof wrote:
> nit: should this be an implicit conversion, or explicit?

Done.
Style guide tells us to use explicit.

[sof, 2015-09-30 07:54:27]

https://codereview.chromium.org/1363143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
WillBeHeapVector<RawPtrWillBeMember<Observer>> m_observers;
Can't this be HeapVector<Member<Observer>> ?

[Yuta Kitamura, 2015-09-30 08:00:30]

On 2015/09/30 at 07:35:32, haraken wrote:
>
https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h
(right):
> 
>
https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
Vector<Observer*> m_observers;
> On 2015/09/30 07:25:09, Yuta Kitamura wrote:
> > On 2015/09/30 at 07:18:20, haraken wrote:
> > > On 2015/09/30 06:26:35, sof wrote:
> > > > Are these bare pointers referring to heap objects?
> > > 
> > > The GC plugin should be able to catch this error. yuta-san?
> > 
> > In the current code, Observer isn't a heap-managed class. That's
> > not BlinkGCPlugin's fault.
> 
> Would it be possible to catch the following case?
> 
> class X : public GarbageCollected<X>, public Y { };
> 
> class Y { };
> 
> class Z : public GarbageCollected<Y> {
>   Y* m_y;  // Y should be a GCMixin. This should be Member<Y>.
>   Vector<Y*> m_vec;  // Y should be a GCMixin. This should be
HeapVector<Member<Y>>.
> };

In my understanding that's impossible.

(1) When reading X's declaration:
  It's not possible to tell if Y needs to be GCMixin, because at this point
  we don't know there's any use of Y*. If there's no Y* in the code base,
  it's valid to leave Y as is.

(2) When reading Y's declaration:
  It's not possible to tell if Y needs to be GCMixin, because at this point
  we don't know who derives Y (X.h may not be #include-d).

(3) When reading Z's declaration:
  It's not possible to tell if Y needs to be GCMixin, because:
    (a) We don't know who derives Y (X.h may not be #include-d).
    (b) Y's declaration may not be available when we see Vector<Y*>
        (Y.h may not be #include-d).

[sof, 2015-09-30 08:05:59]

On 2015/09/30 08:00:30, Yuta Kitamura wrote:
> On 2015/09/30 at 07:35:32, haraken wrote:
> >
>
https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
> Vector<Observer*> m_observers;
> > On 2015/09/30 07:25:09, Yuta Kitamura wrote:
> > > On 2015/09/30 at 07:18:20, haraken wrote:
> > > > On 2015/09/30 06:26:35, sof wrote:
> > > > > Are these bare pointers referring to heap objects?
> > > > 
> > > > The GC plugin should be able to catch this error. yuta-san?
> > > 
> > > In the current code, Observer isn't a heap-managed class. That's
> > > not BlinkGCPlugin's fault.
> > 
> > Would it be possible to catch the following case?
> > 
> > class X : public GarbageCollected<X>, public Y { };
> > 
> > class Y { };
> > 
> > class Z : public GarbageCollected<Y> {
> >   Y* m_y;  // Y should be a GCMixin. This should be Member<Y>.
> >   Vector<Y*> m_vec;  // Y should be a GCMixin. This should be
> HeapVector<Member<Y>>.
> > };
> 
> In my understanding that's impossible.
> 
> (1) When reading X's declaration:
>   It's not possible to tell if Y needs to be GCMixin, because at this point
>   we don't know there's any use of Y*. If there's no Y* in the code base,
>   it's valid to leave Y as is.
> 
> (2) When reading Y's declaration:
>   It's not possible to tell if Y needs to be GCMixin, because at this point
>   we don't know who derives Y (X.h may not be #include-d).
> 
> (3) When reading Z's declaration:
>   It's not possible to tell if Y needs to be GCMixin, because:
>     (a) We don't know who derives Y (X.h may not be #include-d).
>     (b) Y's declaration may not be available when we see Vector<Y*>
>         (Y.h may not be #include-d).

Re: (b) - I wonder what it would take to shift the GC plugin class declaration
checking until processing the constructor declaration for Z.

Y will most/more likely be in scope then.

[peria, 2015-09-30 08:16:03]

https://codereview.chromium.org/1363143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
WillBeHeapVector<RawPtrWillBeMember<Observer>> m_observers;
On 2015/09/30 07:54:27, sof wrote:
> Can't this be HeapVector<Member<Observer>> ?

Done.
Oops, it must be.
It figures RefCountedGarbageCollected class!

[Yuta Kitamura, 2015-09-30 08:53:46]

On 2015/09/30 at 08:05:59, sigbjornf wrote:
> On 2015/09/30 08:00:30, Yuta Kitamura wrote:
> > On 2015/09/30 at 07:35:32, haraken wrote:
> > >
> >
https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
> > > File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/1363143003/diff/1/third_party/WebKit/Source/p...
> > > third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
> > Vector<Observer*> m_observers;
> > > On 2015/09/30 07:25:09, Yuta Kitamura wrote:
> > > > On 2015/09/30 at 07:18:20, haraken wrote:
> > > > > On 2015/09/30 06:26:35, sof wrote:
> > > > > > Are these bare pointers referring to heap objects?
> > > > > 
> > > > > The GC plugin should be able to catch this error. yuta-san?
> > > > 
> > > > In the current code, Observer isn't a heap-managed class. That's
> > > > not BlinkGCPlugin's fault.
> > > 
> > > Would it be possible to catch the following case?
> > > 
> > > class X : public GarbageCollected<X>, public Y { };
> > > 
> > > class Y { };
> > > 
> > > class Z : public GarbageCollected<Y> {
> > >   Y* m_y;  // Y should be a GCMixin. This should be Member<Y>.
> > >   Vector<Y*> m_vec;  // Y should be a GCMixin. This should be
> > HeapVector<Member<Y>>.
> > > };
> > 
> > In my understanding that's impossible.
> > 
> > (1) When reading X's declaration:
> >   It's not possible to tell if Y needs to be GCMixin, because at this point
> >   we don't know there's any use of Y*. If there's no Y* in the code base,
> >   it's valid to leave Y as is.
> > 
> > (2) When reading Y's declaration:
> >   It's not possible to tell if Y needs to be GCMixin, because at this point
> >   we don't know who derives Y (X.h may not be #include-d).
> > 
> > (3) When reading Z's declaration:
> >   It's not possible to tell if Y needs to be GCMixin, because:
> >     (a) We don't know who derives Y (X.h may not be #include-d).
> >     (b) Y's declaration may not be available when we see Vector<Y*>
> >         (Y.h may not be #include-d).
> 
> Re: (b) - I wonder what it would take to shift the GC plugin class declaration
checking until processing the constructor declaration for Z.
> 
> Y will most/more likely be in scope then.

Yeah, that could be a possibility, but X may still not be available.

So, if we could implement it perfectly, we'd still miss some invalid uses.
I'm not sure how useful that's going to be.

[sof, 2015-09-30 09:27:34]

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:99:
DECLARE_TRACE();
Observers will take care of eagerly removing themselves when being swept out,
but what about m_audioConsumers members? Do we assume that the embedder will
call removeAudioConsumer() for all before this MediaStreamSource* is let go of.

i.e., have you considered how this (now heap) object behaves in the face of lazy
sweeping?

[peria, 2015-10-01 09:11:53]

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:99:
DECLARE_TRACE();
On 2015/09/30 09:27:34, sof wrote:
> Observers will take care of eagerly removing themselves when being swept out,
> but what about m_audioConsumers members? Do we assume that the embedder will
> call removeAudioConsumer() for all before this MediaStreamSource* is let go
of.
> 
> i.e., have you considered how this (now heap) object behaves in the face of
lazy
> sweeping?

Yes, we can assume removeAudioConsure() calls happens while MediaStreamSource
is alive.
removeAudioConsure() is called only in
WebMediaStreamSource::removeAudioConsumer(),
and the pointer |m_private| for MediaStreamSource is checked to be non-null.

[sof, 2015-10-01 18:54:06]

from my point of view, looks good & ready now.

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:99:
DECLARE_TRACE();
On 2015/10/01 09:11:53, peria wrote:
> On 2015/09/30 09:27:34, sof wrote:
> > Observers will take care of eagerly removing themselves when being swept
out,
> > but what about m_audioConsumers members? Do we assume that the embedder will
> > call removeAudioConsumer() for all before this MediaStreamSource* is let go
> of.
> > 
> > i.e., have you considered how this (now heap) object behaves in the face of
> lazy
> > sweeping?
> 
> Yes, we can assume removeAudioConsure() calls happens while MediaStreamSource
> is alive.
> removeAudioConsure() is called only in
> WebMediaStreamSource::removeAudioConsumer(),
> and the pointer |m_private| for MediaStreamSource is checked to be non-null.

To verify, when chasing around abstractions I know little of.. :), I found:

- WebMediaStreamSource keeps a Persistent<MediaStreamSource>
- WebAudioCapturerSource keeps a WebMediaStreamSource.
- It calls removeAudioConsumer() on destruction/stopping.
- WebRtcLocalAudioTrack keeps a scoped_refptr<WebAudioCapturerSource>.
- PeerConnectionDependencyFactory::CreateLocalAudioTrack() creates a
WebRtcLocalAudioTrack.
- It assigns that track object as ExtraData to WebMediaStreamTrack (by way of
setExtraData())
- setExtraData() assigns MediaStreamComponent::m_extraData.

That latter MediaStreamComponent keeps the MediaStreamSource alive by way of all
of these abstractions (which bottom out in a Persistent<> ref.) Hence,
MediaStreamComponent will be finalized first and will have all its audio
consumers removed (and observers on next GC), hence no need for altering the
eagerness of its finalization.

So, assuming that this MediaStreamComponent isn't dually kept alive from the
Persistent<MediaStreamSource> reference, this is all fine.

[peria, 2015-10-02 02:23:34]

On 2015/10/01 18:54:06, sof wrote:
> from my point of view, looks good & ready now.
> 
>
https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h
(right):
> 
>
https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:99:
> DECLARE_TRACE();
> On 2015/10/01 09:11:53, peria wrote:
> > On 2015/09/30 09:27:34, sof wrote:
> > > Observers will take care of eagerly removing themselves when being swept
> out,
> > > but what about m_audioConsumers members? Do we assume that the embedder
will
> > > call removeAudioConsumer() for all before this MediaStreamSource* is let
go
> > of.
> > > 
> > > i.e., have you considered how this (now heap) object behaves in the face
of
> > lazy
> > > sweeping?
> > 
> > Yes, we can assume removeAudioConsure() calls happens while
MediaStreamSource
> > is alive.
> > removeAudioConsure() is called only in
> > WebMediaStreamSource::removeAudioConsumer(),
> > and the pointer |m_private| for MediaStreamSource is checked to be non-null.
> 
> To verify, when chasing around abstractions I know little of.. :), I found:
> 
> - WebMediaStreamSource keeps a Persistent<MediaStreamSource>
> - WebAudioCapturerSource keeps a WebMediaStreamSource.
> - It calls removeAudioConsumer() on destruction/stopping.
> - WebRtcLocalAudioTrack keeps a scoped_refptr<WebAudioCapturerSource>.
> - PeerConnectionDependencyFactory::CreateLocalAudioTrack() creates a
> WebRtcLocalAudioTrack.
> - It assigns that track object as ExtraData to WebMediaStreamTrack (by way of
> setExtraData())
> - setExtraData() assigns MediaStreamComponent::m_extraData.
> 
> That latter MediaStreamComponent keeps the MediaStreamSource alive by way of
all
> of these abstractions (which bottom out in a Persistent<> ref.) Hence,
> MediaStreamComponent will be finalized first and will have all its audio
> consumers removed (and observers on next GC), hence no need for altering the
> eagerness of its finalization.
> 
> So, assuming that this MediaStreamComponent isn't dually kept alive from the
> Persistent<MediaStreamSource> reference, this is all fine.

Thank you for this verification!
This shows the relations very clearly.

[haraken, 2015-10-02 02:26:34]

On 2015/10/02 02:23:34, peria wrote:
> On 2015/10/01 18:54:06, sof wrote:
> > from my point of view, looks good & ready now.
> > 
> >
>
https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:99:
> > DECLARE_TRACE();
> > On 2015/10/01 09:11:53, peria wrote:
> > > On 2015/09/30 09:27:34, sof wrote:
> > > > Observers will take care of eagerly removing themselves when being swept
> > out,
> > > > but what about m_audioConsumers members? Do we assume that the embedder
> will
> > > > call removeAudioConsumer() for all before this MediaStreamSource* is let
> go
> > > of.
> > > > 
> > > > i.e., have you considered how this (now heap) object behaves in the face
> of
> > > lazy
> > > > sweeping?
> > > 
> > > Yes, we can assume removeAudioConsure() calls happens while
> MediaStreamSource
> > > is alive.
> > > removeAudioConsure() is called only in
> > > WebMediaStreamSource::removeAudioConsumer(),
> > > and the pointer |m_private| for MediaStreamSource is checked to be
non-null.
> > 
> > To verify, when chasing around abstractions I know little of.. :), I found:
> > 
> > - WebMediaStreamSource keeps a Persistent<MediaStreamSource>
> > - WebAudioCapturerSource keeps a WebMediaStreamSource.
> > - It calls removeAudioConsumer() on destruction/stopping.
> > - WebRtcLocalAudioTrack keeps a scoped_refptr<WebAudioCapturerSource>.
> > - PeerConnectionDependencyFactory::CreateLocalAudioTrack() creates a
> > WebRtcLocalAudioTrack.
> > - It assigns that track object as ExtraData to WebMediaStreamTrack (by way
of
> > setExtraData())
> > - setExtraData() assigns MediaStreamComponent::m_extraData.
> > 
> > That latter MediaStreamComponent keeps the MediaStreamSource alive by way of
> all
> > of these abstractions (which bottom out in a Persistent<> ref.) Hence,
> > MediaStreamComponent will be finalized first and will have all its audio
> > consumers removed (and observers on next GC), hence no need for altering the
> > eagerness of its finalization.
> > 
> > So, assuming that this MediaStreamComponent isn't dually kept alive from the
> > Persistent<MediaStreamSource> reference, this is all fine.
> 
> Thank you for this verification!
> This shows the relations very clearly.

Thanks sigbjorn for the clarification, and I now understand what you were
concerned.

I think it's better to make MediaStreamComponent eagerly-finalized even if it's
not needed for some complicated reason.

[haraken, 2015-10-02 02:27:16]

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
HeapVector<Member<Observer>> m_observers;

Can we make this HeapHashSet<WeakMember<Observer>>? Then we don't need to
unregister the observers explicitly.

[peria, 2015-10-02 03:19:59]

On 2015/10/02 02:26:34, haraken wrote:
> On 2015/10/02 02:23:34, peria wrote:
> > On 2015/10/01 18:54:06, sof wrote:
> > > from my point of view, looks good & ready now.
> > > 
> > >
> >
>
https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:99:
> > > DECLARE_TRACE();
> > > On 2015/10/01 09:11:53, peria wrote:
> > > > On 2015/09/30 09:27:34, sof wrote:
> > > > > Observers will take care of eagerly removing themselves when being
swept
> > > out,
> > > > > but what about m_audioConsumers members? Do we assume that the
embedder
> > will
> > > > > call removeAudioConsumer() for all before this MediaStreamSource* is
let
> > go
> > > > of.
> > > > > 
> > > > > i.e., have you considered how this (now heap) object behaves in the
face
> > of
> > > > lazy
> > > > > sweeping?
> > > > 
> > > > Yes, we can assume removeAudioConsure() calls happens while
> > MediaStreamSource
> > > > is alive.
> > > > removeAudioConsure() is called only in
> > > > WebMediaStreamSource::removeAudioConsumer(),
> > > > and the pointer |m_private| for MediaStreamSource is checked to be
> non-null.
> > > 
> > > To verify, when chasing around abstractions I know little of.. :), I
found:
> > > 
> > > - WebMediaStreamSource keeps a Persistent<MediaStreamSource>
> > > - WebAudioCapturerSource keeps a WebMediaStreamSource.
> > > - It calls removeAudioConsumer() on destruction/stopping.
> > > - WebRtcLocalAudioTrack keeps a scoped_refptr<WebAudioCapturerSource>.
> > > - PeerConnectionDependencyFactory::CreateLocalAudioTrack() creates a
> > > WebRtcLocalAudioTrack.
> > > - It assigns that track object as ExtraData to WebMediaStreamTrack (by way
> of
> > > setExtraData())
> > > - setExtraData() assigns MediaStreamComponent::m_extraData.
> > > 
> > > That latter MediaStreamComponent keeps the MediaStreamSource alive by way
of
> > all
> > > of these abstractions (which bottom out in a Persistent<> ref.) Hence,
> > > MediaStreamComponent will be finalized first and will have all its audio
> > > consumers removed (and observers on next GC), hence no need for altering
the
> > > eagerness of its finalization.
> > > 
> > > So, assuming that this MediaStreamComponent isn't dually kept alive from
the
> > > Persistent<MediaStreamSource> reference, this is all fine.
> > 
> > Thank you for this verification!
> > This shows the relations very clearly.
> 
> Thanks sigbjorn for the clarification, and I now understand what you were
> concerned.
> 
> I think it's better to make MediaStreamComponent eagerly-finalized even if
it's
> not needed for some complicated reason.

done

[peria, 2015-10-02 03:20:00]

On 2015/10/02 02:26:34, haraken wrote:
> On 2015/10/02 02:23:34, peria wrote:
> > On 2015/10/01 18:54:06, sof wrote:
> > > from my point of view, looks good & ready now.
> > > 
> > >
> >
>
https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:99:
> > > DECLARE_TRACE();
> > > On 2015/10/01 09:11:53, peria wrote:
> > > > On 2015/09/30 09:27:34, sof wrote:
> > > > > Observers will take care of eagerly removing themselves when being
swept
> > > out,
> > > > > but what about m_audioConsumers members? Do we assume that the
embedder
> > will
> > > > > call removeAudioConsumer() for all before this MediaStreamSource* is
let
> > go
> > > > of.
> > > > > 
> > > > > i.e., have you considered how this (now heap) object behaves in the
face
> > of
> > > > lazy
> > > > > sweeping?
> > > > 
> > > > Yes, we can assume removeAudioConsure() calls happens while
> > MediaStreamSource
> > > > is alive.
> > > > removeAudioConsure() is called only in
> > > > WebMediaStreamSource::removeAudioConsumer(),
> > > > and the pointer |m_private| for MediaStreamSource is checked to be
> non-null.
> > > 
> > > To verify, when chasing around abstractions I know little of.. :), I
found:
> > > 
> > > - WebMediaStreamSource keeps a Persistent<MediaStreamSource>
> > > - WebAudioCapturerSource keeps a WebMediaStreamSource.
> > > - It calls removeAudioConsumer() on destruction/stopping.
> > > - WebRtcLocalAudioTrack keeps a scoped_refptr<WebAudioCapturerSource>.
> > > - PeerConnectionDependencyFactory::CreateLocalAudioTrack() creates a
> > > WebRtcLocalAudioTrack.
> > > - It assigns that track object as ExtraData to WebMediaStreamTrack (by way
> of
> > > setExtraData())
> > > - setExtraData() assigns MediaStreamComponent::m_extraData.
> > > 
> > > That latter MediaStreamComponent keeps the MediaStreamSource alive by way
of
> > all
> > > of these abstractions (which bottom out in a Persistent<> ref.) Hence,
> > > MediaStreamComponent will be finalized first and will have all its audio
> > > consumers removed (and observers on next GC), hence no need for altering
the
> > > eagerness of its finalization.
> > > 
> > > So, assuming that this MediaStreamComponent isn't dually kept alive from
the
> > > Persistent<MediaStreamSource> reference, this is all fine.
> > 
> > Thank you for this verification!
> > This shows the relations very clearly.
> 
> Thanks sigbjorn for the clarification, and I now understand what you were
> concerned.
> 
> I think it's better to make MediaStreamComponent eagerly-finalized even if
it's
> not needed for some complicated reason.

done

[peria, 2015-10-02 03:20:50]

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

[peria, 2015-10-02 03:20:51]

Kent-san,
could you take a look for third_party/WebKit/public \?

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:111:
HeapVector<Member<Observer>> m_observers;
On 2015/10/02 02:27:15, haraken wrote:
> 
> Can we make this HeapHashSet<WeakMember<Observer>>? Then we don't need to
> unregister the observers explicitly.

Done.

[tkent, 2015-10-02 04:02:14]

lgtm

https://codereview.chromium.org/1363143003/diff/30025/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebMediaStreamSource.cpp
(right):

https://codereview.chromium.org/1363143003/diff/30025/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebMediaStreamSource.cpp:73:
:m_private(mediaStreamSource)
Please do not remove a space.

https://codereview.chromium.org/1363143003/diff/30025/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/30025/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaStreamSource.h:122: explicit
BLINK_PLATFORM_EXPORT WebMediaStreamSource(MediaStreamSource*);
Please don't add |explicit|.  We allow implicit conversions for Web types.

[peria, 2015-10-02 04:20:37]

https://codereview.chromium.org/1363143003/diff/30025/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebMediaStreamSource.cpp
(right):

https://codereview.chromium.org/1363143003/diff/30025/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebMediaStreamSource.cpp:73:
:m_private(mediaStreamSource)
On 2015/10/02 04:02:14, tkent wrote:
> Please do not remove a space.

Done.

https://codereview.chromium.org/1363143003/diff/30025/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/30025/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaStreamSource.h:122: explicit
BLINK_PLATFORM_EXPORT WebMediaStreamSource(MediaStreamSource*);
On 2015/10/02 04:02:14, tkent wrote:
> Please don't add |explicit|.  We allow implicit conversions for Web types.
> 

Done.

[haraken, 2015-10-02 04:35:10]

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:157:
m_descriptor->setClient(0);

You must not touch m_descriptor in the destructor. As commented below, you need
to make MediaStreamDescriptorClient a GCMixin and use Member for
MediaStreamDescriptor::m_client. Then you don't need to call setClient(0) here.

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamRegistry.h
(right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamRegistry.h:39: class
MODULES_EXPORT MediaStreamRegistry final : public URLRegistry {

Would you move the URLRegistry's hierarchy to the heap in a follow-up?

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamComponent.h
(right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamComponent.h:74:
EAGERLY_FINALIZE();

Add a comment and mention that this needs to be eagerly-finalized because the
ExtraData needs to be promptly finalized (although it is actually safe  to not
eagerly-finalize it for some complicated reason).

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h
(right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h:92:
DECLARE_TRACE();

It would be better to mark this as eagerly-finalize as well since the
MediaStreamDescriptor holds an ExtraData. It is safe to finalize the ExtraData
promptly.

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h:101:
MediaStreamDescriptorClient* m_client;

You need to make MediaStreamDescriptorClient a GCMixin and use a Member here.

yutak@: It would be great if we could have a verification about this error...

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:98:
DECLARE_TRACE();

Ditto.

[peria, 2015-10-02 05:48:55]

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:157:
m_descriptor->setClient(0);
On 2015/10/02 04:35:09, haraken wrote:
> 
> You must not touch m_descriptor in the destructor. As commented below, you
need
> to make MediaStreamDescriptorClient a GCMixin and use Member for
> MediaStreamDescriptor::m_client. Then you don't need to call setClient(0)
here.

Done.

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamRegistry.h
(right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamRegistry.h:39: class
MODULES_EXPORT MediaStreamRegistry final : public URLRegistry {
On 2015/10/02 04:35:09, haraken wrote:
> 
> Would you move the URLRegistry's hierarchy to the heap in a follow-up?

Acknowledged.

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamComponent.h
(right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamComponent.h:74:
EAGERLY_FINALIZE();
On 2015/10/02 04:35:09, haraken wrote:
> 
> Add a comment and mention that this needs to be eagerly-finalized because the
> ExtraData needs to be promptly finalized (although it is actually safe  to not
> eagerly-finalize it for some complicated reason).

Done.

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h
(right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h:92:
DECLARE_TRACE();
On 2015/10/02 04:35:09, haraken wrote:
> 
> It would be better to mark this as eagerly-finalize as well since the
> MediaStreamDescriptor holds an ExtraData. It is safe to finalize the ExtraData
> promptly.
> 

Done.

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamDescriptor.h:101:
MediaStreamDescriptorClient* m_client;
On 2015/10/02 04:35:09, haraken wrote:
> 
> You need to make MediaStreamDescriptorClient a GCMixin and use a Member here.
> 
> yutak@: It would be great if we could have a verification about this error...

Done.

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:98:
DECLARE_TRACE();
On 2015/10/02 04:35:09, haraken wrote:
> 
> Ditto.

MediaStreamComponent holds a Member<MediaStreamSource>
so we cannot make both of them to be EAGARLY_FINALIZE.

[sof, 2015-10-02 05:55:19]

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:157:
m_descriptor->setClient(0);
On 2015/10/02 05:48:54, peria wrote:
> On 2015/10/02 04:35:09, haraken wrote:
> > 
> > You must not touch m_descriptor in the destructor. As commented below, you
> need
> > to make MediaStreamDescriptorClient a GCMixin and use Member for
> > MediaStreamDescriptor::m_client. Then you don't need to call setClient(0)
> here.
> 
> Done.

MediaStream is eagerly finalized, so I'm not sure that restriction holds. But if
it can be avoided (the explicit clearing _and_ the need for eager finalization),
so much the better.

[peria, 2015-10-02 06:25:35]

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:157:
m_descriptor->setClient(0);
On 2015/10/02 05:55:19, sof wrote:
> On 2015/10/02 05:48:54, peria wrote:
> > On 2015/10/02 04:35:09, haraken wrote:
> > > 
> > > You must not touch m_descriptor in the destructor. As commented below, you
> > need
> > > to make MediaStreamDescriptorClient a GCMixin and use Member for
> > > MediaStreamDescriptor::m_client. Then you don't need to call setClient(0)
> > here.
> > 
> > Done.
> 
> MediaStream is eagerly finalized, so I'm not sure that restriction holds. But
if
> it can be avoided (the explicit clearing _and_ the need for eager
finalization),
> so much the better.

We can do it with using a pre-finalizer with eagerly-finalize, right?

[haraken, 2015-10-02 06:40:28]

On 2015/10/02 06:25:35, peria wrote:
>
https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp (right):
> 
>
https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:157:
> m_descriptor->setClient(0);
> On 2015/10/02 05:55:19, sof wrote:
> > On 2015/10/02 05:48:54, peria wrote:
> > > On 2015/10/02 04:35:09, haraken wrote:
> > > > 
> > > > You must not touch m_descriptor in the destructor. As commented below,
you
> > > need
> > > > to make MediaStreamDescriptorClient a GCMixin and use Member for
> > > > MediaStreamDescriptor::m_client. Then you don't need to call
setClient(0)
> > > here.
> > > 
> > > Done.
> > 
> > MediaStream is eagerly finalized, so I'm not sure that restriction holds.
But
> if
> > it can be avoided (the explicit clearing _and_ the need for eager
> finalization),
> > so much the better.
> 
> We can do it with using a pre-finalizer with eagerly-finalize, right?

We can, but it is better to just make MediaStreamDescriptorClient a GCMixin
(because pre-finalizers add more overhead to a GC; eager-finalize is a somewhat
nasty notion).

[peria, 2015-10-02 06:48:05]

On 2015/10/02 06:40:28, haraken wrote:
> On 2015/10/02 06:25:35, peria wrote:
> >
>
https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:157:
> > m_descriptor->setClient(0);
> > On 2015/10/02 05:55:19, sof wrote:
> > > On 2015/10/02 05:48:54, peria wrote:
> > > > On 2015/10/02 04:35:09, haraken wrote:
> > > > > 
> > > > > You must not touch m_descriptor in the destructor. As commented below,
> you
> > > > need
> > > > > to make MediaStreamDescriptorClient a GCMixin and use Member for
> > > > > MediaStreamDescriptor::m_client. Then you don't need to call
> setClient(0)
> > > > here.
> > > > 
> > > > Done.
> > > 
> > > MediaStream is eagerly finalized, so I'm not sure that restriction holds.
> But
> > if
> > > it can be avoided (the explicit clearing _and_ the need for eager
> > finalization),
> > > so much the better.
> > 
> > We can do it with using a pre-finalizer with eagerly-finalize, right?
> 
> We can, but it is better to just make MediaStreamDescriptorClient a GCMixin
> (because pre-finalizers add more overhead to a GC; eager-finalize is a
somewhat
> nasty notion).

Then, how do you feel on PS#7?
If it's OK, I'll update to be same with PS7.

[haraken, 2015-10-02 06:49:29]

On 2015/10/02 06:48:05, peria wrote:
> On 2015/10/02 06:40:28, haraken wrote:
> > On 2015/10/02 06:25:35, peria wrote:
> > >
> >
>
https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:157:
> > > m_descriptor->setClient(0);
> > > On 2015/10/02 05:55:19, sof wrote:
> > > > On 2015/10/02 05:48:54, peria wrote:
> > > > > On 2015/10/02 04:35:09, haraken wrote:
> > > > > > 
> > > > > > You must not touch m_descriptor in the destructor. As commented
below,
> > you
> > > > > need
> > > > > > to make MediaStreamDescriptorClient a GCMixin and use Member for
> > > > > > MediaStreamDescriptor::m_client. Then you don't need to call
> > setClient(0)
> > > > > here.
> > > > > 
> > > > > Done.
> > > > 
> > > > MediaStream is eagerly finalized, so I'm not sure that restriction
holds.
> > But
> > > if
> > > > it can be avoided (the explicit clearing _and_ the need for eager
> > > finalization),
> > > > so much the better.
> > > 
> > > We can do it with using a pre-finalizer with eagerly-finalize, right?
> > 
> > We can, but it is better to just make MediaStreamDescriptorClient a GCMixin
> > (because pre-finalizers add more overhead to a GC; eager-finalize is a
> somewhat
> > nasty notion).
> 
> Then, how do you feel on PS#7?
> If it's OK, I'll update to be same with PS7.

PS7 LGTM

[peria, 2015-10-02 06:54:20]

On 2015/10/02 06:49:29, haraken wrote:
> On 2015/10/02 06:48:05, peria wrote:
> > On 2015/10/02 06:40:28, haraken wrote:
> > > On 2015/10/02 06:25:35, peria wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/modules/mediastream/MediaStream.cpp
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1363143003/diff/90001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/modules/mediastream/MediaStream.cpp:157:
> > > > m_descriptor->setClient(0);
> > > > On 2015/10/02 05:55:19, sof wrote:
> > > > > On 2015/10/02 05:48:54, peria wrote:
> > > > > > On 2015/10/02 04:35:09, haraken wrote:
> > > > > > > 
> > > > > > > You must not touch m_descriptor in the destructor. As commented
> below,
> > > you
> > > > > > need
> > > > > > > to make MediaStreamDescriptorClient a GCMixin and use Member for
> > > > > > > MediaStreamDescriptor::m_client. Then you don't need to call
> > > setClient(0)
> > > > > > here.
> > > > > > 
> > > > > > Done.
> > > > > 
> > > > > MediaStream is eagerly finalized, so I'm not sure that restriction
> holds.
> > > But
> > > > if
> > > > > it can be avoided (the explicit clearing _and_ the need for eager
> > > > finalization),
> > > > > so much the better.
> > > > 
> > > > We can do it with using a pre-finalizer with eagerly-finalize, right?
> > > 
> > > We can, but it is better to just make MediaStreamDescriptorClient a
GCMixin
> > > (because pre-finalizers add more overhead to a GC; eager-finalize is a
> > somewhat
> > > nasty notion).
> > 
> > Then, how do you feel on PS#7?
> > If it's OK, I'll update to be same with PS7.
> 
> PS7 LGTM

updated.
Thanks.

[peria, 2015-10-02 07:58:07]

The CQ bit was checked by peria@chromium.org

[peria, 2015-10-02 07:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1363143003/#ps140001
(title: "Revert changes in PS8")

[commit-bot: I haz the power, 2015-10-02 07:58:25]

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

[commit-bot: I haz the power, 2015-10-02 10:59:57]

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

[commit-bot: I haz the power, 2015-10-02 10:59:58]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[peria, 2015-10-02 11:07:20]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-10-02 11:07:59]

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

[commit-bot: I haz the power, 2015-10-02 12:35:00]

Committed patchset #9 (id:140001)

[commit-bot: I haz the power, 2015-10-02 12:35:54]

Patchset 9 (id:??) landed as
https://crrev.com/622d290d58d54d6c1410d19ecb88b5c2dd2d135d
Cr-Commit-Position: refs/heads/master@{#352008}

[Finnur, 2015-10-02 13:31:15]

Looks like this might be causing multiple test failures on the bots. Example:
https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/9048

[Finnur, 2015-10-02 13:34:08]

A revert of this CL (patchset #9 id:140001) has been created in
https://codereview.chromium.org/1375773004/ by finnur@chromium.org.

The reason for reverting is: Suspecting it broke at least for bots on the
waterfall, e.g.:
https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/9048.

[Finnur, 2015-10-02 15:37:39]

Yup, seems like the Linux failures have stopped.