Oilpan: Move MediaStreamSource, MediaStreamComponent and MediaStreamDescriptor to oilpan's heap

Oilpan: Move MediaStreamSource, MediaStreamComponent and MediaStreamDescriptor to oilpan's heap

- MediaStreamSource, MediaStreamComponent and MediaStreamDescriptor must be moved to oilpan's heap in one go because their lifetime are tightly coupled.

- Removed m_descriptor->setClient(0) from MediaStream's destructor. This is OK because MediaStreamDescriptor has a strong Member back to the Client (i.e., MediaStream).

- Removed m_component->source()->removeObserver(this) from MediaStreamTrack's destructor. This is OK because this CL made the observers weak (i.e., MediaStreamSource::m_observers is a hash set of weak members to MediaStreamTrack objects).

- Removed WebMediaStreamTrack::ExtraData::m_owner because it's unused.


BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181204

[haraken, 2014-08-27 12:29:51]

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

[haraken, 2014-08-27 12:33:56]

PTAL

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
File public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
public/platform/WebMediaStreamSource.h:66: MediaStreamSource* m_owner;

I'm not sure if I'm doing a right thing here.

- Because ExtraData is a web-exposed class, we cannot make the ExtraData derive
GarbageCollected.

- We cannot use Persistent<MediaStreamSource>, because it can cause a cycle (we
cannot assume nothing about the lifetime of ExtraData objects in the embedder
side).

- If we don't add INSIDE_BLINK, it causes a lot of compile errors.

- Given the above, I used a raw pointer for m_owner.

[zerny-chromium, 2014-08-27 13:21:53]

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

[zerny-chromium, 2014-08-27 13:21:53]

lgtm

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
File public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
public/platform/WebMediaStreamSource.h:66: MediaStreamSource* m_owner;
On 2014/08/27 12:33:56, haraken wrote:
> 
> I'm not sure if I'm doing a right thing here.

This raw pointer looks very unsafe, but is no more unsafe than in the non-oilpan
build (given that ExtaData is referenced by an OwnPtr from MediaStreamSource).

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
File public/platform/WebMediaStreamTrack.h (left):

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
public/platform/WebMediaStreamTrack.h:54: MediaStreamComponent* m_owner;
It's not clear to me why we can remove the owner-api here but not for the
MediaStreamSource case.

[haraken, 2014-08-27 13:24:55]

Thanks for review. I'll wait for tkent-san's comment on
WebMediaStreamSource::m_owner.

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
File public/platform/WebMediaStreamTrack.h (left):

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
public/platform/WebMediaStreamTrack.h:54: MediaStreamComponent* m_owner;
On 2014/08/27 13:21:53, zerny-chromium wrote:
> It's not clear to me why we can remove the owner-api here but not for the
> MediaStreamSource case.

It's just because WebMediaStreamSource::m_owner is used in the Chromium side but
WebMediaStreamTrack::m_owner is not used in the Chromium side.

[tkent, 2014-09-01 06:59:47]

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
File public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
public/platform/WebMediaStreamSource.h:66: MediaStreamSource* m_owner;
On 2014/08/27 13:21:53, zerny-chromium wrote:
> On 2014/08/27 12:33:56, haraken wrote:
> > 
> > I'm not sure if I'm doing a right thing here.
> 
> This raw pointer looks very unsafe, but is no more unsafe than in the
non-oilpan
> build (given that ExtaData is referenced by an OwnPtr from MediaStreamSource).

Resolving this raw pointer needs a lot of work, and I don't think it's worth to
do it now. Let's keep the raw pointer.

[haraken, 2014-09-01 07:10:44]

PTAL

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
File public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/509933002/diff/1/public/platform/WebMediaStre...
public/platform/WebMediaStreamSource.h:66: MediaStreamSource* m_owner;
On 2014/09/01 06:59:46, tkent wrote:
> On 2014/08/27 13:21:53, zerny-chromium wrote:
> > On 2014/08/27 12:33:56, haraken wrote:
> > > 
> > > I'm not sure if I'm doing a right thing here.
> > 
> > This raw pointer looks very unsafe, but is no more unsafe than in the
> non-oilpan
> > build (given that ExtaData is referenced by an OwnPtr from
MediaStreamSource).
> 
> Resolving this raw pointer needs a lot of work, and I don't think it's worth
to
> do it now. Let's keep the raw pointer.

Thanks, I filed a bug and added the bug number to GC_PLUGIN_IGNORE.

[tkent, 2014-09-01 07:56:24]

lgtm

[tkent, 2014-09-01 07:56:28]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-09-01 07:56:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/509933002/20001

[commit-bot: I haz the power, 2014-09-01 09:02:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/25053)

[commit-bot: I haz the power, 2014-09-01 09:10:29]

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

[commit-bot: I haz the power, 2014-09-01 09:10:31]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/25065)

[haraken, 2014-09-01 09:20:13]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-01 09:21:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/509933002/20001

[commit-bot: I haz the power, 2014-09-01 09:21:33]

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

[commit-bot: I haz the power, 2014-09-01 09:21:33]

Failed to apply patch for Source/modules/mediastream/RTCStatsRequestImpl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/mediastream/RTCStatsRequestImpl.h
  Hunk #1 FAILED at 58.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/mediastream/RTCStatsRequestImpl.h.rej

Patch:       Source/modules/mediastream/RTCStatsRequestImpl.h
Index: Source/modules/mediastream/RTCStatsRequestImpl.h
diff --git a/Source/modules/mediastream/RTCStatsRequestImpl.h
b/Source/modules/mediastream/RTCStatsRequestImpl.h
index
270ae3906ab554e2ee89cd94478a7568db3f0929..1dbf6e4cd12273b8fdfbd9901f8e879c14a06190
100644
--- a/Source/modules/mediastream/RTCStatsRequestImpl.h
+++ b/Source/modules/mediastream/RTCStatsRequestImpl.h
@@ -58,7 +58,7 @@ private:
     void clear();
 
     OwnPtr<RTCStatsCallback> m_successCallback;
-    RefPtr<MediaStreamComponent> m_component;
+    Persistent<MediaStreamComponent> m_component;
 
     Persistent<RTCPeerConnection> m_requester;
 };

[haraken, 2014-09-01 10:33:10]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-01 10:34:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/509933002/40001

[commit-bot: I haz the power, 2014-09-01 11:42:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/21769)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48762)

[commit-bot: I haz the power, 2014-09-01 11:51:03]

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

[commit-bot: I haz the power, 2014-09-01 11:51:04]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48783)

[haraken, 2014-09-01 11:51:51]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-01 11:52:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/509933002/40001

[commit-bot: I haz the power, 2014-09-01 11:52:52]

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

[commit-bot: I haz the power, 2014-09-01 11:52:53]

Failed to apply patch for Source/modules/mediastream/RTCStatsRequestImpl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/mediastream/RTCStatsRequestImpl.h
  Hunk #1 FAILED at 60.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/mediastream/RTCStatsRequestImpl.h.rej

Patch:       Source/modules/mediastream/RTCStatsRequestImpl.h
Index: Source/modules/mediastream/RTCStatsRequestImpl.h
diff --git a/Source/modules/mediastream/RTCStatsRequestImpl.h
b/Source/modules/mediastream/RTCStatsRequestImpl.h
index
d50c0774f3cae977016849aa9718c0ad454c15de..858ac3fade484cc7fac78a1719d34c5a89428c48
100644
--- a/Source/modules/mediastream/RTCStatsRequestImpl.h
+++ b/Source/modules/mediastream/RTCStatsRequestImpl.h
@@ -60,7 +60,7 @@ private:
     void clear();
 
     OwnPtrWillBePersistent<RTCStatsCallback> m_successCallback;
-    RefPtr<MediaStreamComponent> m_component;
+    Member<MediaStreamComponent> m_component;
 
     Member<RTCPeerConnection> m_requester;
 };

[haraken, 2014-09-01 12:22:46]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-01 12:23:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/509933002/60001

[commit-bot: I haz the power, 2014-09-01 13:42:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/21785)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/25100)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48793)

[commit-bot: I haz the power, 2014-09-01 13:53:03]

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

[commit-bot: I haz the power, 2014-09-01 13:53:04]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48804)

[haraken, 2014-09-02 00:50:35]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-02 00:50:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/509933002/80001

[commit-bot: I haz the power, 2014-09-02 01:52:44]

Committed patchset #5 (id:80001) as 181204

[tkent, 2014-09-02 02:32:48]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/534473002/ by tkent@chromium.org.

The reason for reverting is: Broke multiple tests in content_unittests.
e.g.
https://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/39151
.

[haraken, 2014-09-04 11:27:51]

Now I understand why this crashes several tests in content_unittests.

The first problem is that the unittest needs to explicitly call
Heap::collectGarbage to test what it wants to test. For that goal, we need to
expose Heap::collectGarbage to the web layer. This is just work. I'll do that.

https://codereview.chromium.org/509933002/diff/100001/Source/platform/mediast...
File Source/platform/mediastream/MediaStreamComponent.cpp (right):

https://codereview.chromium.org/509933002/diff/100001/Source/platform/mediast...
Source/platform/mediastream/MediaStreamComponent.cpp:53: class
MediaStreamComponentDisposer {

We need to implement the disposer not only for MediaStreamComponent but also for
all platform objects that hold OwnPtr<ExtraData>s, because we have to assume
that the ExtraData's destructor can touch on-heap objects in the embedder side.

It's not nice to add the disposer for each class, we need to implement some
abstract class for the disposer.

https://codereview.chromium.org/509933002/diff/100001/Source/platform/mediast...
File Source/platform/mediastream/MediaStreamComponent.h (right):

https://codereview.chromium.org/509933002/diff/100001/Source/platform/mediast...
Source/platform/mediastream/MediaStreamComponent.h:109: OwnPtr<ExtraData>
m_extraData;

The second problem is this OwnPtr. If we explicitly call Heap::collectGarbage()
from media_stream_impl_unittest, it crashes for the following reason:

- Heap::collectGarbage() destructs MediaStreamSource and MediaStreamComponent.

- MediaStreamComponent's destructor destructs the OwnPtr<ExtraData>.

- Since the ExtraData is inherited in the embedder side, whatever can happen in
the ExtraData's destructor. In this particular case, the ExtraData's destructor
touches the MediaStreamSource that dies in the same GC round, and crashes.

To address the issue, we need to delay the destruction of OwnPtr<ExtraData> to
the thread-specific weak processing. I implemented the logic in
MediaStreamComponent.cpp.