Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1671033002:
Do not let go of MediaStreamTracks too early. (Closed)
DescriptionDo not let go of MediaStreamTracks too early.
Add missing hasPendingActivity() predicate.
R=haraken
BUG=583264
Committed: https://crrev.com/6c27a5cbd705e2d052efbb2acd50af30b0557f01
Cr-Commit-Position: refs/heads/master@{#373864}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 21 (5 generated)
sigbjornf@opera.com changed reviewers: + haraken@chromium.org, tommi@chromium.org
please take a look.
LGTM
Description was changed from ========== Do not let go of MediaStreamTracks too early. Add missing hasPendingActivity() predicate. R= BUG=583264 ========== to ========== Do not let go of MediaStreamTracks too early. Add missing hasPendingActivity() predicate. R=haraken BUG=583264 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1671033002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1671033002/1
Message was sent while issue was closed.
Description was changed from ========== Do not let go of MediaStreamTracks too early. Add missing hasPendingActivity() predicate. R=haraken BUG=583264 ========== to ========== Do not let go of MediaStreamTracks too early. Add missing hasPendingActivity() predicate. R=haraken BUG=583264 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Do not let go of MediaStreamTracks too early. Add missing hasPendingActivity() predicate. R=haraken BUG=583264 ========== to ========== Do not let go of MediaStreamTracks too early. Add missing hasPendingActivity() predicate. R=haraken BUG=583264 Committed: https://crrev.com/6c27a5cbd705e2d052efbb2acd50af30b0557f01 Cr-Commit-Position: refs/heads/master@{#373864} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6c27a5cbd705e2d052efbb2acd50af30b0557f01 Cr-Commit-Position: refs/heads/master@{#373864}
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:215: return !m_stopped && hasEventListeners(); I wonder if something effectively like this needs to be done for all ActiveDOMObjects? It isn't done for several.
Message was sent while issue was closed.
https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:215: return !m_stopped && hasEventListeners(); On 2016/02/07 06:54:27, sof wrote: > I wonder if something effectively like this needs to be done for all > ActiveDOMObjects? It isn't done for several. I notice MajorGCWrapperVisitor has special handling for Nodes with event listeners, but not ActiveDOMObjects.
Message was sent while issue was closed.
https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:215: return !m_stopped && hasEventListeners(); On 2016/02/07 07:04:36, sof wrote: > On 2016/02/07 06:54:27, sof wrote: > > I wonder if something effectively like this needs to be done for all > > ActiveDOMObjects? It isn't done for several. > > I notice MajorGCWrapperVisitor has special handling for Nodes with event > listeners, but not ActiveDOMObjects. Yeah, without having the check, we'll end up with keeping ActiveDOMObjects alive longer than needed. So we should. That said, we can't simply check hasEventListeners in V8GCController because each ActiveDOMObject has its own semantics to keep the object alive. So I think we need to manually add the hasEventListeners check to all hasPendingActivity()'s that need the check.
Message was sent while issue was closed.
On 2016/02/07 07:21:31, haraken wrote: > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:215: return > !m_stopped && hasEventListeners(); > On 2016/02/07 07:04:36, sof wrote: > > On 2016/02/07 06:54:27, sof wrote: > > > I wonder if something effectively like this needs to be done for all > > > ActiveDOMObjects? It isn't done for several. > > > > I notice MajorGCWrapperVisitor has special handling for Nodes with event > > listeners, but not ActiveDOMObjects. > > Yeah, without having the check, we'll end up with keeping ActiveDOMObjects alive > longer than needed. So we should. Sorry, I'm confused. Without having the check, we end up with collecting the ActiveDOMObjects too early, right? (That is problematic.)
Message was sent while issue was closed.
On 2016/02/07 07:22:50, haraken wrote: > On 2016/02/07 07:21:31, haraken wrote: > > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp > (right): > > > > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:215: return > > !m_stopped && hasEventListeners(); > > On 2016/02/07 07:04:36, sof wrote: > > > On 2016/02/07 06:54:27, sof wrote: > > > > I wonder if something effectively like this needs to be done for all > > > > ActiveDOMObjects? It isn't done for several. > > > > > > I notice MajorGCWrapperVisitor has special handling for Nodes with event > > > listeners, but not ActiveDOMObjects. > > > > Yeah, without having the check, we'll end up with keeping ActiveDOMObjects > alive > > longer than needed. So we should. > > Sorry, I'm confused. Without having the check, we end up with collecting the > ActiveDOMObjects too early, right? (That is problematic.) Yes, I was confused too :) That's a separate issue, (not) creating an implicit dependency to the listener wrappers.
Message was sent while issue was closed.
On 2016/02/07 07:53:52, sof wrote: > On 2016/02/07 07:22:50, haraken wrote: > > On 2016/02/07 07:21:31, haraken wrote: > > > > > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > > > File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp > > (right): > > > > > > > > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:215: > return > > > !m_stopped && hasEventListeners(); > > > On 2016/02/07 07:04:36, sof wrote: > > > > On 2016/02/07 06:54:27, sof wrote: > > > > > I wonder if something effectively like this needs to be done for all > > > > > ActiveDOMObjects? It isn't done for several. > > > > > > > > I notice MajorGCWrapperVisitor has special handling for Nodes with event > > > > listeners, but not ActiveDOMObjects. > > > > > > Yeah, without having the check, we'll end up with keeping ActiveDOMObjects > > alive > > > longer than needed. So we should. > > > > Sorry, I'm confused. Without having the check, we end up with collecting the > > ActiveDOMObjects too early, right? (That is problematic.) > > Yes, I was confused too :) That's a separate issue, (not) creating an implicit > dependency to the listener wrappers. Let's recap..to enumerate the two issues: - should there be implicit references set up from the ActiveDOMObject wrapper to its listener wrappers. - if hasPendingActivity() returns false if it has event listeners, we end up GCing the ActiveDOMObject, but the wrapper isn't GCed. Next GC will then fail. I think they are independent issues. But solving the 2nd like was done here, looks questionable now.
Message was sent while issue was closed.
On 2016/02/07 08:03:26, sof wrote: > On 2016/02/07 07:53:52, sof wrote: > > On 2016/02/07 07:22:50, haraken wrote: > > > On 2016/02/07 07:21:31, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > > > > File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > > > > third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:215: > > return > > > > !m_stopped && hasEventListeners(); > > > > On 2016/02/07 07:04:36, sof wrote: > > > > > On 2016/02/07 06:54:27, sof wrote: > > > > > > I wonder if something effectively like this needs to be done for all > > > > > > ActiveDOMObjects? It isn't done for several. > > > > > > > > > > I notice MajorGCWrapperVisitor has special handling for Nodes with event > > > > > listeners, but not ActiveDOMObjects. > > > > > > > > Yeah, without having the check, we'll end up with keeping ActiveDOMObjects > > > alive > > > > longer than needed. So we should. > > > > > > Sorry, I'm confused. Without having the check, we end up with collecting the > > > ActiveDOMObjects too early, right? (That is problematic.) > > > > Yes, I was confused too :) That's a separate issue, (not) creating an implicit > > dependency to the listener wrappers. > > Let's recap..to enumerate the two issues: > > - should there be implicit references set up from the ActiveDOMObject wrapper > to its listener wrappers. > - if hasPendingActivity() returns false if it has event listeners, we end up > GCing the ActiveDOMObject, but the wrapper isn't GCed. Next GC will then fail. > > I think they are independent issues. But solving the 2nd like was done here, > looks questionable now. And a third issue is if hasPendingActivity() doesn't always return true if hasEventListeners() holds, then is it safe for MinorGCUnmodifiedWrapperVisitor not to check for that directly, like what is done for Nodes. (No minor GC involved for the testcase here, but I need to study its GC behavior more regardless.)
Message was sent while issue was closed.
On 2016/02/07 08:50:00, sof wrote: > ... > > (No minor GC involved for the testcase here, but I need to study its GC behavior > more regardless.) Interesting case; MediaStreamTrack is notified as having been 'ended' when its associated MediaStreamComponent is (pre)finalized after a v8 major GC. It then dispatches the 'ended' event. Which creates an Event object referring back to the MediaStreamTrack wrapper... So, if MediaStreamTrack isn't otherwise kept alive before that happens (it wasn't prior to this CL), it will be unreachable and swept out by Oilpan. But only for prefinalizer to revive a reference to it (its wrapper). Makes sense & sounds plausible? Assuming so, the change here is too blunt -- the hasPendingActivity() predicate should be equal to ended(). And, along with looking into these details, the original question here "does hasEventListeners() have to be used by all (EventTarget + ActiveDOMObject) objects hasPendingActivity overrides" is then understood and not an issue. Good.
Message was sent while issue was closed.
The assert in https://codereview.chromium.org/1673073003/ would have caught out the problem here.
Message was sent while issue was closed.
On 2016/02/07 08:50:00, sof wrote: > On 2016/02/07 08:03:26, sof wrote: > > On 2016/02/07 07:53:52, sof wrote: > > > On 2016/02/07 07:22:50, haraken wrote: > > > > On 2016/02/07 07:21:31, haraken wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > > > > > File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1671033002/diff/1/third_party/WebKit/Source/m... > > > > > third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:215: > > > return > > > > > !m_stopped && hasEventListeners(); > > > > > On 2016/02/07 07:04:36, sof wrote: > > > > > > On 2016/02/07 06:54:27, sof wrote: > > > > > > > I wonder if something effectively like this needs to be done for all > > > > > > > ActiveDOMObjects? It isn't done for several. > > > > > > > > > > > > I notice MajorGCWrapperVisitor has special handling for Nodes with > event > > > > > > listeners, but not ActiveDOMObjects. > > > > > > > > > > Yeah, without having the check, we'll end up with keeping > ActiveDOMObjects > > > > alive > > > > > longer than needed. So we should. > > > > > > > > Sorry, I'm confused. Without having the check, we end up with collecting > the > > > > ActiveDOMObjects too early, right? (That is problematic.) > > > > > > Yes, I was confused too :) That's a separate issue, (not) creating an > implicit > > > dependency to the listener wrappers. > > > > Let's recap..to enumerate the two issues: > > > > - should there be implicit references set up from the ActiveDOMObject wrapper > > to its listener wrappers. > > - if hasPendingActivity() returns false if it has event listeners, we end up > > GCing the ActiveDOMObject, but the wrapper isn't GCed. Next GC will then fail. > > > > I think they are independent issues. But solving the 2nd like was done here, > > looks questionable now. > > And a third issue is if hasPendingActivity() doesn't always return true if > hasEventListeners() holds, then is it safe for MinorGCUnmodifiedWrapperVisitor > not to check for that directly, like what is done for Nodes. > > (No minor GC involved for the testcase here, but I need to study its GC behavior > more regardless.) I'm still confused :) > > - should there be implicit references set up from the ActiveDOMObject wrapper > > to its listener wrappers. I think the answer is yes. If the ActiveDOMObject wrapper doesn't have an implicit reference to its listener wrappers, you end up with collecting the listener wrappers prematurely. > > - if hasPendingActivity() returns false if it has event listeners, we end up > > GCing the ActiveDOMObject, but the wrapper isn't GCed. Next GC will then fail. I don't quite understand this. The ActiveDOMObject wrapper has an Oilpan's persistent handle to the ActiveDOMObject. So the ActiveDOMObject should not be collected by Oilpan's GC until the wrapper is collected by V8's GC. > And a third issue is if hasPendingActivity() doesn't always return true if > hasEventListeners() holds, then is it safe for MinorGCUnmodifiedWrapperVisitor > not to check for that directly, like what is done for Nodes. This is working because the current minor GC simply ignores all wrappers of non-Nodes (i.e., the current minor GC cannot collect wrappers of non-Nodes). https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
