Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(153)

Issue 562663002: Revert 181702 "Oilpan: Move MediaStreamSource, MediaStreamCompon..." (Closed)

Created:
6 years, 3 months ago by haraken
Modified:
6 years, 3 months ago
Reviewers:
haraken
CC:
blink-reviews
Visibility:
Public.

Description

Revert 181702 "Oilpan: Move MediaStreamSource, MediaStreamCompon..." This CL caused memory leaks in content_unittests. http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%282%29/builds/7259 > 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. > > - Introduced MediaStreamComponentDisposer to delay the destruction of > MediaStreamComponent::m_extraData. The ExtraData is exposed to the web > and some Chromium objects inherit from the ExtraData. If we clear the > m_extraData in the MediaStreamComponent's destructor, those Chromium-side > objects are also destructed in the MediaStreamComponent's destructor. > This is problematic because the destructors of the Chromium-side classes > can touch other on-heap objects in the Blink side. To avoid the issue, > we need to delay the destruction of the ExtraData to thread-specific > weak processing. The disposer pattern actually makes the deletion of > the extra data happen earlier and not later. The disposer makes sure > that the extra data is destructed in weak processing which is run before > sweeping and therefore all the objects are still alive and can be touched. > > > - The same problem arises for MediaStreamDescriptor and MediaStreamSource. > This CL adds MediaStreamDescriptorDisposer and MediaStreamSourceDisposer. > > - This CL depends on https://codereview.chromium.org/543603003/ and https://codereview.chromium.org/549153002. > > BUG=340522 > > Review URL: https://codereview.chromium.org/552653005 TBR=haraken@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181730

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+131 lines, -208 lines) Patch
M trunk/Source/modules/mediastream/MediaStream.h View 3 chunks +4 lines, -4 lines 0 comments Download
M trunk/Source/modules/mediastream/MediaStream.cpp View 3 chunks +3 lines, -4 lines 0 comments Download
M trunk/Source/modules/mediastream/MediaStreamRegistry.h View 2 chunks +1 line, -2 lines 0 comments Download
M trunk/Source/modules/mediastream/MediaStreamTrack.h View 2 chunks +2 lines, -2 lines 0 comments Download
M trunk/Source/modules/mediastream/MediaStreamTrack.cpp View 3 chunks +4 lines, -5 lines 0 comments Download
M trunk/Source/modules/mediastream/RTCStatsRequestImpl.h View 1 chunk +1 line, -1 line 0 comments Download
M trunk/Source/modules/mediastream/RTCStatsRequestImpl.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M trunk/Source/modules/mediastream/UserMediaRequest.h View 1 chunk +1 line, -1 line 0 comments Download
M trunk/Source/modules/mediastream/UserMediaRequest.cpp View 1 chunk +1 line, -1 line 0 comments Download
M trunk/Source/modules/webaudio/MediaStreamAudioDestinationNode.h View 1 chunk +1 line, -1 line 0 comments Download
M trunk/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp View 1 chunk +0 lines, -1 line 0 comments Download
M trunk/Source/platform/exported/WebMediaStream.cpp View 2 chunks +11 lines, -1 line 0 comments Download
M trunk/Source/platform/exported/WebMediaStreamSource.cpp View 2 chunks +6 lines, -1 line 0 comments Download
M trunk/Source/platform/exported/WebMediaStreamTrack.cpp View 3 chunks +26 lines, -0 lines 0 comments Download
M trunk/Source/platform/mediastream/MediaStreamComponent.h View 4 chunks +7 lines, -10 lines 0 comments Download
M trunk/Source/platform/mediastream/MediaStreamComponent.cpp View 2 chunks +5 lines, -43 lines 0 comments Download
M trunk/Source/platform/mediastream/MediaStreamDescriptor.h View 4 chunks +13 lines, -15 lines 0 comments Download
M trunk/Source/platform/mediastream/MediaStreamDescriptor.cpp View 5 chunks +10 lines, -50 lines 0 comments Download
M trunk/Source/platform/mediastream/MediaStreamSource.h View 4 chunks +7 lines, -11 lines 0 comments Download
M trunk/Source/platform/mediastream/MediaStreamSource.cpp View 3 chunks +11 lines, -45 lines 0 comments Download
M trunk/public/platform/WebMediaStream.h View 1 chunk +3 lines, -1 line 0 comments Download
M trunk/public/platform/WebMediaStreamSource.h View 3 chunks +2 lines, -7 lines 0 comments Download
M trunk/public/platform/WebMediaStreamTrack.h View 2 chunks +12 lines, -1 line 0 comments Download

Messages

Total messages: 2 (0 generated)
haraken
6 years, 3 months ago (2014-09-10 13:28:33 UTC) #1
haraken
6 years, 3 months ago (2014-09-10 13:29:00 UTC) #2
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as r181730.

Powered by Google App Engine
This is Rietveld 408576698