Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570773002:
Fix crash when pipeline is stopped while video restart is pending (Closed)
DescriptionFix crash when pipeline is stopped while video restart is pending
Media renderer might get destroyed (due to pipeline being stopped or
suspended) while a video track restart is pending. When that happens
the VideoRendererImpl has a pending flush_cb_ that will get called from
video renderer destructor invoking RendererImpl::RestartVideoRenderer,
so RestartVideoRenderer needs to actually try restarting the video
renderer only if we are still in the playing state.
BUG=668963
Committed: https://crrev.com/a5c72b04df68fd0721447e9de3867bb7ccaa743d
Cr-Commit-Position: refs/heads/master@{#441457}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Added comments + addressed CR feedback #
Total comments: 2
Patch Set 3 : Invalidate weak RendererImpl pointers, instead of introducing a new renderer state #
Messages
Total messages: 45 (24 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix crash when pipeline is stopped while video restart is pending Media renderer might get destroyed (due to pipeline being stopped or suspended) while a video track restart is pending. When that happens the VideoRendererImpl has a pending flush_cb_ that will get called from video renderer destructor invoking RendererImpl::RestartVideoRenderer, so RestartVideoRenderer needs to actually try restarting the video renderer only if we are still in the playing state. BUG=668963 ========== to ========== Fix crash when pipeline is stopped while video restart is pending Media renderer might get destroyed (due to pipeline being stopped or suspended) while a video track restart is pending. When that happens the VideoRendererImpl has a pending flush_cb_ that will get called from video renderer destructor invoking RendererImpl::RestartVideoRenderer, so RestartVideoRenderer needs to actually try restarting the video renderer only if we are still in the playing state. BUG=668963 ==========
servolk@chromium.org changed reviewers: + avayvod@chromium.org, dalecurtis@chromium.org, xhwang@chromium.org
non-OWNER review with a kind of architectural question https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:124: base::ResetAndReturn(&flush_cb_).Run(); Hm, I'd rather have the callbacks owner know somehow the object is destroyed and not call its methods at all. Otherwise it seems like all the current and new public methods of RendererImpl should at least DCHECK for STATE_SHUTDOWN or actively take it into account like you do below to avoid similar crashes in the future. https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:251: if (state_ == STATE_PLAYING) { nit: maybe you should rather check for STATE_SHUTDOWN before the state_ DCHECK and exit if it is?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:124: base::ResetAndReturn(&flush_cb_).Run(); On 2016/12/13 05:08:41, whywhat wrote: > Hm, I'd rather have the callbacks owner know somehow the object is destroyed and > not call its methods at all. Otherwise it seems like all the current and new > public methods of RendererImpl should at least DCHECK for STATE_SHUTDOWN or > actively take it into account like you do below to avoid similar crashes in the > future. Wait, are you talking about RendererImpl::flush_cb_ here? But I'm not changing any logic related to RendererImpl::flush_cb_ at all in this CL. And yes, every callback owner of an async operation (flush_cb_ in this case) might need to check that the object is still in valid state, but again that's not changing with my CL. As you can see from the implementation of ~RendererImpl both video_renderer_ and audio_renderer_ will be null if the final flush_cb_ was called due to async Flush pending while the RendererImpl was destroyed. I'm not aware of any crashes happening in the methods bound to flush_cb_ here, so I guess this means callback owners are already sufficiently careful and take all of this into account. https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:251: if (state_ == STATE_PLAYING) { On 2016/12/13 05:08:41, whywhat wrote: > nit: maybe you should rather check for STATE_SHUTDOWN before the state_ DCHECK > and exit if it is? Yes, I've considered doing that, but I've decided that doing it this way is better. Initially I assumed that RestartVideoRenderer will be called only in the STATE_PLAYING. But now that I've been proven wrong I've thought a bit more about this and realized that we only need to call video_renderer_->StartPlayingFrom if we are in STATE_PLAYING. In all other states this can be a no-op. Since if we are in a STATE_SHUTDOWN, we don't need to restart the video renderer. And if RendererImpl::RestartVideoRenderer was somehow called in some other state, e.g. STATE_INITIALIZING or STATE_FLUSHING, then we don't need to call video_renderer_->StartPlayingFrom here, since it will be called later from RendererImpl::StartPlayingFrom.
https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:124: base::ResetAndReturn(&flush_cb_).Run(); On 2016/12/13 at 19:39:47, servolk wrote: > On 2016/12/13 05:08:41, whywhat wrote: > > Hm, I'd rather have the callbacks owner know somehow the object is destroyed and > > not call its methods at all. Otherwise it seems like all the current and new > > public methods of RendererImpl should at least DCHECK for STATE_SHUTDOWN or > > actively take it into account like you do below to avoid similar crashes in the > > future. > > Wait, are you talking about RendererImpl::flush_cb_ here? But I'm not changing any logic related to RendererImpl::flush_cb_ at all in this CL. And yes, every callback owner of an async operation (flush_cb_ in this case) might need to check that the object is still in valid state, but again that's not changing with my CL. As you can see from the implementation of ~RendererImpl both video_renderer_ and audio_renderer_ will be null if the final flush_cb_ was called due to async Flush pending while the RendererImpl was destroyed. I'm not aware of any crashes happening in the methods bound to flush_cb_ here, so I guess this means callback owners are already sufficiently careful and take all of this into account. Wait, aren't you fixing a crash where flush_cb_ is RestartVideoRenderer and called from the dtor and crashes because it uses video_renderer_ (so it's a counter example to your assumption in the last sentence)? https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:251: if (state_ == STATE_PLAYING) { On 2016/12/13 at 19:39:47, servolk wrote: > On 2016/12/13 05:08:41, whywhat wrote: > > nit: maybe you should rather check for STATE_SHUTDOWN before the state_ DCHECK > > and exit if it is? > > Yes, I've considered doing that, but I've decided that doing it this way is better. Initially I assumed that RestartVideoRenderer will be called only in the STATE_PLAYING. But now that I've been proven wrong I've thought a bit more about this and realized that we only need to call video_renderer_->StartPlayingFrom if we are in STATE_PLAYING. In all other states this can be a no-op. Since if we are in a STATE_SHUTDOWN, we don't need to restart the video renderer. And if RendererImpl::RestartVideoRenderer was somehow called in some other state, e.g. STATE_INITIALIZING or STATE_FLUSHING, then we don't need to call video_renderer_->StartPlayingFrom here, since it will be called later from RendererImpl::StartPlayingFrom. Calling this method in any other state than PLAYING (and now SHUTDOWN) was supposed to be a logical mistake, hence the DCHECK. Now this method can be called in any state and will just do nothing if the state is wrong. Meaning that if it's called by mistake in the wrong state, noone will notice.
https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:124: base::ResetAndReturn(&flush_cb_).Run(); On 2016/12/13 20:10:54, whywhat wrote: > On 2016/12/13 at 19:39:47, servolk wrote: > > On 2016/12/13 05:08:41, whywhat wrote: > > > Hm, I'd rather have the callbacks owner know somehow the object is destroyed > and > > > not call its methods at all. Otherwise it seems like all the current and new > > > public methods of RendererImpl should at least DCHECK for STATE_SHUTDOWN or > > > actively take it into account like you do below to avoid similar crashes in > the > > > future. > > > > Wait, are you talking about RendererImpl::flush_cb_ here? But I'm not changing > any logic related to RendererImpl::flush_cb_ at all in this CL. And yes, every > callback owner of an async operation (flush_cb_ in this case) might need to > check that the object is still in valid state, but again that's not changing > with my CL. As you can see from the implementation of ~RendererImpl both > video_renderer_ and audio_renderer_ will be null if the final flush_cb_ was > called due to async Flush pending while the RendererImpl was destroyed. I'm not > aware of any crashes happening in the methods bound to flush_cb_ here, so I > guess this means callback owners are already sufficiently careful and take all > of this into account. > > Wait, aren't you fixing a crash where flush_cb_ is RestartVideoRenderer and > called from the dtor and crashes because it uses video_renderer_ (so it's a > counter example to your assumption in the last sentence)? No, the flush_cb_ in RendererImpl is probably null, the RestartVideoRenderer is invoked via VideoRendererImpl::flush_cb_. I agree that the pattern is very similar (Flush is an async operation in both cases, and the flush_cb_ is invoked from the destructor in both cases if the object is destroyed while the Flush is pending). But I think that flush_cb_ is called from the destructor intentionally, so that it's easier for clients to reason about async Flush (this guarantees that Flush callback will be always called eventually, even if the object is destroyed). And yes, this means that clients need to be more careful, but it's probably a better tradeoff than needing to be careful about whether the Flush callback will get invoked or not. https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:251: if (state_ == STATE_PLAYING) { On 2016/12/13 20:10:54, whywhat wrote: > On 2016/12/13 at 19:39:47, servolk wrote: > > On 2016/12/13 05:08:41, whywhat wrote: > > > nit: maybe you should rather check for STATE_SHUTDOWN before the state_ > DCHECK > > > and exit if it is? > > > > Yes, I've considered doing that, but I've decided that doing it this way is > better. Initially I assumed that RestartVideoRenderer will be called only in the > STATE_PLAYING. But now that I've been proven wrong I've thought a bit more about > this and realized that we only need to call video_renderer_->StartPlayingFrom if > we are in STATE_PLAYING. In all other states this can be a no-op. Since if we > are in a STATE_SHUTDOWN, we don't need to restart the video renderer. And if > RendererImpl::RestartVideoRenderer was somehow called in some other state, e.g. > STATE_INITIALIZING or STATE_FLUSHING, then we don't need to call > video_renderer_->StartPlayingFrom here, since it will be called later from > RendererImpl::StartPlayingFrom. > > Calling this method in any other state than PLAYING (and now SHUTDOWN) was > supposed to be a logical mistake, hence the DCHECK. Now this method can be > called in any state and will just do nothing if the state is wrong. Meaning that > if it's called by mistake in the wrong state, noone will notice. I want to make two notes here: 1. Calling this method in any other state beside PLAYING doesn't have to be a logical mistake. After this CL I believe it would be valid to call it in any state. 2. What's the alternative? The RestartVideoRenderer is used as a flush callback in RestartStreamPlayback. Flush is an async operation, meaning RendererImpl or VideoRendererImpl might get destroyed while an async operation is pending. As I've explained in the comment above I believe this is an intentional design choice to guarantee that the Flush callback will always get called, even if the object is destroyed. I'm not changing that design choice in this CL, since it potentially might have other destabilizing consequences (we'd need to go and ensure that all code that used Flush API is going to clean up resources correctly if a renderer gets destroyed while an async flush is pending). So I've decided to do what I've done with this CL - just make RestartVideo/AudioRenderer more careful and allow them to be called in any object state. I believe this is a better and a more robust solution than other alternatives.
https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:124: base::ResetAndReturn(&flush_cb_).Run(); On 2016/12/13 at 20:57:10, servolk wrote: > On 2016/12/13 20:10:54, whywhat wrote: > > On 2016/12/13 at 19:39:47, servolk wrote: > > > On 2016/12/13 05:08:41, whywhat wrote: > > > > Hm, I'd rather have the callbacks owner know somehow the object is destroyed > > and > > > > not call its methods at all. Otherwise it seems like all the current and new > > > > public methods of RendererImpl should at least DCHECK for STATE_SHUTDOWN or > > > > actively take it into account like you do below to avoid similar crashes in > > the > > > > future. > > > > > > Wait, are you talking about RendererImpl::flush_cb_ here? But I'm not changing > > any logic related to RendererImpl::flush_cb_ at all in this CL. And yes, every > > callback owner of an async operation (flush_cb_ in this case) might need to > > check that the object is still in valid state, but again that's not changing > > with my CL. As you can see from the implementation of ~RendererImpl both > > video_renderer_ and audio_renderer_ will be null if the final flush_cb_ was > > called due to async Flush pending while the RendererImpl was destroyed. I'm not > > aware of any crashes happening in the methods bound to flush_cb_ here, so I > > guess this means callback owners are already sufficiently careful and take all > > of this into account. > > > > Wait, aren't you fixing a crash where flush_cb_ is RestartVideoRenderer and > > called from the dtor and crashes because it uses video_renderer_ (so it's a > > counter example to your assumption in the last sentence)? > > No, the flush_cb_ in RendererImpl is probably null, the RestartVideoRenderer is invoked via VideoRendererImpl::flush_cb_. I agree that the pattern is very similar (Flush is an async operation in both cases, and the flush_cb_ is invoked from the destructor in both cases if the object is destroyed while the Flush is pending). But I think that flush_cb_ is called from the destructor intentionally, so that it's easier for clients to reason about async Flush (this guarantees that Flush callback will be always called eventually, even if the object is destroyed). And yes, this means that clients need to be more careful, but it's probably a better tradeoff than needing to be careful about whether the Flush callback will get invoked or not. Hm, the dtor checks if flush_cb_ is null before invoking it. The crash happens on the line invoking video_renderer_ so I'd suspect video_renderer_ is the null pointer that causes SIGSEGV: https://chromium.googlesource.com/chromium/src/+/57.0.2931.0/media/renderers/... https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:251: if (state_ == STATE_PLAYING) { On 2016/12/13 at 20:57:10, servolk wrote: > On 2016/12/13 20:10:54, whywhat wrote: > > On 2016/12/13 at 19:39:47, servolk wrote: > > > On 2016/12/13 05:08:41, whywhat wrote: > > > > nit: maybe you should rather check for STATE_SHUTDOWN before the state_ > > DCHECK > > > > and exit if it is? > > > > > > Yes, I've considered doing that, but I've decided that doing it this way is > > better. Initially I assumed that RestartVideoRenderer will be called only in the > > STATE_PLAYING. But now that I've been proven wrong I've thought a bit more about > > this and realized that we only need to call video_renderer_->StartPlayingFrom if > > we are in STATE_PLAYING. In all other states this can be a no-op. Since if we > > are in a STATE_SHUTDOWN, we don't need to restart the video renderer. And if > > RendererImpl::RestartVideoRenderer was somehow called in some other state, e.g. > > STATE_INITIALIZING or STATE_FLUSHING, then we don't need to call > > video_renderer_->StartPlayingFrom here, since it will be called later from > > RendererImpl::StartPlayingFrom. > > > > Calling this method in any other state than PLAYING (and now SHUTDOWN) was > > supposed to be a logical mistake, hence the DCHECK. Now this method can be > > called in any state and will just do nothing if the state is wrong. Meaning that > > if it's called by mistake in the wrong state, noone will notice. > > I want to make two notes here: > 1. Calling this method in any other state beside PLAYING doesn't have to be a logical mistake. After this CL I believe it would be valid to call it in any state. > 2. What's the alternative? The RestartVideoRenderer is used as a flush callback in RestartStreamPlayback. Flush is an async operation, meaning RendererImpl or VideoRendererImpl might get destroyed while an async operation is pending. As I've explained in the comment above I believe this is an intentional design choice to guarantee that the Flush callback will always get called, even if the object is destroyed. I'm not changing that design choice in this CL, since it potentially might have other destabilizing consequences (we'd need to go and ensure that all code that used Flush API is going to clean up resources correctly if a renderer gets destroyed while an async flush is pending). So I've decided to do what I've done with this CL - just make RestartVideo/AudioRenderer more careful and allow them to be called in any object state. I believe this is a better and a more robust solution than other alternatives. I agree that fixing this specific crash by making RestartVideoDecoder tolerable to being called from dtor via flash_cb_ is fine. I argue about two points: - if flush_cb_ wasn't called as is from the dtor, it would prevent this crash as well as any future attempts to use the renderer object while it's being destroyed; it's probably not worth fixing in this CL; at least Flush() method in the header could have a comment saying that flush_cb_ needs to check the state and exit early if it's SHUTDOWN - you don't have to allow calling RestartVideoRenderer in any state, only SHUTDOWN and PLAYING, to fix the crash; do you expect flush_cb_ being called from other states? again, a comment saying that RVR does nothing unless the object is in PLAYING state would be helpful https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:261: if (state_ == STATE_PLAYING) { nit: you could still do if (state_ != STATE_PLAYING) return; reducing the indentation of the code :)
https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:124: base::ResetAndReturn(&flush_cb_).Run(); On 2016/12/13 21:43:07, whywhat wrote: > On 2016/12/13 at 20:57:10, servolk wrote: > > On 2016/12/13 20:10:54, whywhat wrote: > > > On 2016/12/13 at 19:39:47, servolk wrote: > > > > On 2016/12/13 05:08:41, whywhat wrote: > > > > > Hm, I'd rather have the callbacks owner know somehow the object is > destroyed > > > and > > > > > not call its methods at all. Otherwise it seems like all the current and > new > > > > > public methods of RendererImpl should at least DCHECK for STATE_SHUTDOWN > or > > > > > actively take it into account like you do below to avoid similar crashes > in > > > the > > > > > future. > > > > > > > > Wait, are you talking about RendererImpl::flush_cb_ here? But I'm not > changing > > > any logic related to RendererImpl::flush_cb_ at all in this CL. And yes, > every > > > callback owner of an async operation (flush_cb_ in this case) might need to > > > check that the object is still in valid state, but again that's not changing > > > with my CL. As you can see from the implementation of ~RendererImpl both > > > video_renderer_ and audio_renderer_ will be null if the final flush_cb_ was > > > called due to async Flush pending while the RendererImpl was destroyed. I'm > not > > > aware of any crashes happening in the methods bound to flush_cb_ here, so I > > > guess this means callback owners are already sufficiently careful and take > all > > > of this into account. > > > > > > Wait, aren't you fixing a crash where flush_cb_ is RestartVideoRenderer and > > > called from the dtor and crashes because it uses video_renderer_ (so it's a > > > counter example to your assumption in the last sentence)? > > > > No, the flush_cb_ in RendererImpl is probably null, the RestartVideoRenderer > is invoked via VideoRendererImpl::flush_cb_. I agree that the pattern is very > similar (Flush is an async operation in both cases, and the flush_cb_ is invoked > from the destructor in both cases if the object is destroyed while the Flush is > pending). But I think that flush_cb_ is called from the destructor > intentionally, so that it's easier for clients to reason about async Flush (this > guarantees that Flush callback will be always called eventually, even if the > object is destroyed). And yes, this means that clients need to be more careful, > but it's probably a better tradeoff than needing to be careful about whether the > Flush callback will get invoked or not. > > Hm, the dtor checks if flush_cb_ is null before invoking it. The crash happens > on the line invoking video_renderer_ so I'd suspect video_renderer_ is the null > pointer that causes SIGSEGV: > https://chromium.googlesource.com/chromium/src/+/57.0.2931.0/media/renderers/... Yes, that's correct and doesn't contradict what I've said above. Again: RendererImpl::flush_cb_ is null. VideoRendererImpl::flush_cb_ is non-null and is used to invoke RendererImpl::RestartVideoPlayback. Note that VideoRendererImpl is not RendererImpl. The crash indeed happens because RendererImpl::video_renderer_ is null, while the video_renderer_.reset (line 118 above) is executing. The full crash call stack can be seen here, hopefully this will clarify things: https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.ptype%3D%... https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:251: if (state_ == STATE_PLAYING) { On 2016/12/13 21:43:07, whywhat wrote: > On 2016/12/13 at 20:57:10, servolk wrote: > > On 2016/12/13 20:10:54, whywhat wrote: > > > On 2016/12/13 at 19:39:47, servolk wrote: > > > > On 2016/12/13 05:08:41, whywhat wrote: > > > > > nit: maybe you should rather check for STATE_SHUTDOWN before the state_ > > > DCHECK > > > > > and exit if it is? > > > > > > > > Yes, I've considered doing that, but I've decided that doing it this way > is > > > better. Initially I assumed that RestartVideoRenderer will be called only in > the > > > STATE_PLAYING. But now that I've been proven wrong I've thought a bit more > about > > > this and realized that we only need to call > video_renderer_->StartPlayingFrom if > > > we are in STATE_PLAYING. In all other states this can be a no-op. Since if > we > > > are in a STATE_SHUTDOWN, we don't need to restart the video renderer. And if > > > RendererImpl::RestartVideoRenderer was somehow called in some other state, > e.g. > > > STATE_INITIALIZING or STATE_FLUSHING, then we don't need to call > > > video_renderer_->StartPlayingFrom here, since it will be called later from > > > RendererImpl::StartPlayingFrom. > > > > > > Calling this method in any other state than PLAYING (and now SHUTDOWN) was > > > supposed to be a logical mistake, hence the DCHECK. Now this method can be > > > called in any state and will just do nothing if the state is wrong. Meaning > that > > > if it's called by mistake in the wrong state, noone will notice. > > > > I want to make two notes here: > > 1. Calling this method in any other state beside PLAYING doesn't have to be a > logical mistake. After this CL I believe it would be valid to call it in any > state. > > 2. What's the alternative? The RestartVideoRenderer is used as a flush > callback in RestartStreamPlayback. Flush is an async operation, meaning > RendererImpl or VideoRendererImpl might get destroyed while an async operation > is pending. As I've explained in the comment above I believe this is an > intentional design choice to guarantee that the Flush callback will always get > called, even if the object is destroyed. I'm not changing that design choice in > this CL, since it potentially might have other destabilizing consequences (we'd > need to go and ensure that all code that used Flush API is going to clean up > resources correctly if a renderer gets destroyed while an async flush is > pending). So I've decided to do what I've done with this CL - just make > RestartVideo/AudioRenderer more careful and allow them to be called in any > object state. I believe this is a better and a more robust solution than other > alternatives. > > I agree that fixing this specific crash by making RestartVideoDecoder tolerable > to being called from dtor via flash_cb_ is fine. > > I argue about two points: > - if flush_cb_ wasn't called as is from the dtor, it would prevent this crash as > well as any future attempts to use the renderer object while it's being > destroyed; it's probably not worth fixing in this CL; at least Flush() method in > the header could have a comment saying that flush_cb_ needs to check the state > and exit early if it's SHUTDOWN > - you don't have to allow calling RestartVideoRenderer in any state, only > SHUTDOWN and PLAYING, to fix the crash; do you expect flush_cb_ being called > from other states? again, a comment saying that RVR does nothing unless the > object is in PLAYING state would be helpful I see your points. But my counterarguments are: 1. If we didn't call flush_cb_ from dtor then indeed we would avoid this crash, but we would also break the Flush contract by not guaranteeing the flush_cb is going to be always invoked no matter what. I'm arguing that it's much easier to introduce some new bug via changing the Flush contract like that. I believe that the fix proposed in this CL is safer. I'm also curious to hear Dale/Xiaohan opinion on this, hopefully they'll chime in soon. 2. We don't have to allow calling RestartVideoRenderer in any state besides PLAYING or SHUTDOWN for now. But it's easy for us to do, so why not? I don't expect flush_cb_ to be called from other states yet, but I don't also see any good reasons to prohibit RVR from being called in states other than PLAYING or SHUTDOWN. And yes, I agree, adding a comment in RVR explaining why nothing need to be done in state other than PLAYING is reasonable, I'll do that. https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:261: if (state_ == STATE_PLAYING) { On 2016/12/13 21:43:07, whywhat wrote: > nit: you could still do > > if (state_ != STATE_PLAYING) > return; > > reducing the indentation of the code :) Done.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/13 at 22:18:33, servolk wrote: > https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... > File media/renderers/renderer_impl.cc (right): > > https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... > media/renderers/renderer_impl.cc:124: base::ResetAndReturn(&flush_cb_).Run(); > On 2016/12/13 21:43:07, whywhat wrote: > > On 2016/12/13 at 20:57:10, servolk wrote: > > > On 2016/12/13 20:10:54, whywhat wrote: > > > > On 2016/12/13 at 19:39:47, servolk wrote: > > > > > On 2016/12/13 05:08:41, whywhat wrote: > > > > > > Hm, I'd rather have the callbacks owner know somehow the object is > > destroyed > > > > and > > > > > > not call its methods at all. Otherwise it seems like all the current and > > new > > > > > > public methods of RendererImpl should at least DCHECK for STATE_SHUTDOWN > > or > > > > > > actively take it into account like you do below to avoid similar crashes > > in > > > > the > > > > > > future. > > > > > > > > > > Wait, are you talking about RendererImpl::flush_cb_ here? But I'm not > > changing > > > > any logic related to RendererImpl::flush_cb_ at all in this CL. And yes, > > every > > > > callback owner of an async operation (flush_cb_ in this case) might need to > > > > check that the object is still in valid state, but again that's not changing > > > > with my CL. As you can see from the implementation of ~RendererImpl both > > > > video_renderer_ and audio_renderer_ will be null if the final flush_cb_ was > > > > called due to async Flush pending while the RendererImpl was destroyed. I'm > > not > > > > aware of any crashes happening in the methods bound to flush_cb_ here, so I > > > > guess this means callback owners are already sufficiently careful and take > > all > > > > of this into account. > > > > > > > > Wait, aren't you fixing a crash where flush_cb_ is RestartVideoRenderer and > > > > called from the dtor and crashes because it uses video_renderer_ (so it's a > > > > counter example to your assumption in the last sentence)? > > > > > > No, the flush_cb_ in RendererImpl is probably null, the RestartVideoRenderer > > is invoked via VideoRendererImpl::flush_cb_. I agree that the pattern is very > > similar (Flush is an async operation in both cases, and the flush_cb_ is invoked > > from the destructor in both cases if the object is destroyed while the Flush is > > pending). But I think that flush_cb_ is called from the destructor > > intentionally, so that it's easier for clients to reason about async Flush (this > > guarantees that Flush callback will be always called eventually, even if the > > object is destroyed). And yes, this means that clients need to be more careful, > > but it's probably a better tradeoff than needing to be careful about whether the > > Flush callback will get invoked or not. > > > > Hm, the dtor checks if flush_cb_ is null before invoking it. The crash happens > > on the line invoking video_renderer_ so I'd suspect video_renderer_ is the null > > pointer that causes SIGSEGV: > > https://chromium.googlesource.com/chromium/src/+/57.0.2931.0/media/renderers/... > > Yes, that's correct and doesn't contradict what I've said above. Again: RendererImpl::flush_cb_ is null. VideoRendererImpl::flush_cb_ is non-null and is used to invoke RendererImpl::RestartVideoPlayback. Note that VideoRendererImpl is not RendererImpl. > The crash indeed happens because RendererImpl::video_renderer_ is null, while the video_renderer_.reset (line 118 above) is executing. > > The full crash call stack can be seen here, hopefully this will clarify things: > https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.ptype%3D%... Yes, that makes it's somewhat clearer now, thanks for the link. There're too many flash_cb_ out there :) Do you think SHUTDOWN state is needed or the callback could just check the video_renderer for null and exit? > > https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... > media/renderers/renderer_impl.cc:251: if (state_ == STATE_PLAYING) { > On 2016/12/13 21:43:07, whywhat wrote: > > On 2016/12/13 at 20:57:10, servolk wrote: > > > On 2016/12/13 20:10:54, whywhat wrote: > > > > On 2016/12/13 at 19:39:47, servolk wrote: > > > > > On 2016/12/13 05:08:41, whywhat wrote: > > > > > > nit: maybe you should rather check for STATE_SHUTDOWN before the state_ > > > > DCHECK > > > > > > and exit if it is? > > > > > > > > > > Yes, I've considered doing that, but I've decided that doing it this way > > is > > > > better. Initially I assumed that RestartVideoRenderer will be called only in > > the > > > > STATE_PLAYING. But now that I've been proven wrong I've thought a bit more > > about > > > > this and realized that we only need to call > > video_renderer_->StartPlayingFrom if > > > > we are in STATE_PLAYING. In all other states this can be a no-op. Since if > > we > > > > are in a STATE_SHUTDOWN, we don't need to restart the video renderer. And if > > > > RendererImpl::RestartVideoRenderer was somehow called in some other state, > > e.g. > > > > STATE_INITIALIZING or STATE_FLUSHING, then we don't need to call > > > > video_renderer_->StartPlayingFrom here, since it will be called later from > > > > RendererImpl::StartPlayingFrom. > > > > > > > > Calling this method in any other state than PLAYING (and now SHUTDOWN) was > > > > supposed to be a logical mistake, hence the DCHECK. Now this method can be > > > > called in any state and will just do nothing if the state is wrong. Meaning > > that > > > > if it's called by mistake in the wrong state, noone will notice. > > > > > > I want to make two notes here: > > > 1. Calling this method in any other state beside PLAYING doesn't have to be a > > logical mistake. After this CL I believe it would be valid to call it in any > > state. > > > 2. What's the alternative? The RestartVideoRenderer is used as a flush > > callback in RestartStreamPlayback. Flush is an async operation, meaning > > RendererImpl or VideoRendererImpl might get destroyed while an async operation > > is pending. As I've explained in the comment above I believe this is an > > intentional design choice to guarantee that the Flush callback will always get > > called, even if the object is destroyed. I'm not changing that design choice in > > this CL, since it potentially might have other destabilizing consequences (we'd > > need to go and ensure that all code that used Flush API is going to clean up > > resources correctly if a renderer gets destroyed while an async flush is > > pending). So I've decided to do what I've done with this CL - just make > > RestartVideo/AudioRenderer more careful and allow them to be called in any > > object state. I believe this is a better and a more robust solution than other > > alternatives. > > > > I agree that fixing this specific crash by making RestartVideoDecoder tolerable > > to being called from dtor via flash_cb_ is fine. > > > > I argue about two points: > > - if flush_cb_ wasn't called as is from the dtor, it would prevent this crash as > > well as any future attempts to use the renderer object while it's being > > destroyed; it's probably not worth fixing in this CL; at least Flush() method in > > the header could have a comment saying that flush_cb_ needs to check the state > > and exit early if it's SHUTDOWN > > - you don't have to allow calling RestartVideoRenderer in any state, only > > SHUTDOWN and PLAYING, to fix the crash; do you expect flush_cb_ being called > > from other states? again, a comment saying that RVR does nothing unless the > > object is in PLAYING state would be helpful > > I see your points. But my counterarguments are: > 1. If we didn't call flush_cb_ from dtor then indeed we would avoid this crash, but we would also break the Flush contract by not guaranteeing the flush_cb is going to be always invoked no matter what. I'm arguing that it's much easier to introduce some new bug via changing the Flush contract like that. I believe that the fix proposed in this CL is safer. > I'm also curious to hear Dale/Xiaohan opinion on this, hopefully they'll chime in soon. But Flush doesn't do anything in that case though and now it have to account for being called in that special context so I don't think it's too bad to simply not call it in dtor and specify that in the contract. I don't know all the uses of flush_cb_ though (seems like the mojo renderer uses them with some classes) so maybe always calling it is justified. > > 2. We don't have to allow calling RestartVideoRenderer in any state besides PLAYING or SHUTDOWN for now. But it's easy for us to do, so why not? I don't expect flush_cb_ to be called from other states yet, but I don't also see any good reasons to prohibit RVR from being called in states other than PLAYING or SHUTDOWN. And yes, I agree, adding a comment in RVR explaining why nothing need to be done in state other than PLAYING is reasonable, I'll do that. My view is until it's not needed, don't allow it. But I don't feel too strongly at this point. > > https://codereview.chromium.org/2570773002/diff/1/media/renderers/renderer_im... > media/renderers/renderer_impl.cc:261: if (state_ == STATE_PLAYING) { > On 2016/12/13 21:43:07, whywhat wrote: > > nit: you could still do > > > > if (state_ != STATE_PLAYING) > > return; > > > > reducing the indentation of the code :) > > Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
dalecurtis@chromium.org changed required reviewers: + xhwang@chromium.org
defer to xhwang@ since he's our rendererimpl expert.
On 2016/12/14 00:45:08, DaleCurtis_OOO_Dec14_Jan6 wrote: > defer to xhwang@ since he's our rendererimpl expert. Post-holiday ping
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sorry for the delay, looking
https://codereview.chromium.org/2570773002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2570773002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:114: state_ = STATE_SHUTDOWN; Instead of adding a new state, does it make sense to invalid all weak pointers so that RendererImpl::RestartVideoRenderer() (the callback) will never be called during destruction? Note that all callbacks are bound using weak pointers, e.g. https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=0&l...
https://codereview.chromium.org/2570773002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2570773002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:114: state_ = STATE_SHUTDOWN; On 2017/01/04 18:29:09, xhwang wrote: > Instead of adding a new state, does it make sense to invalid all weak pointers > so that RendererImpl::RestartVideoRenderer() (the callback) will never be called > during destruction? > > Note that all callbacks are bound using weak pointers, e.g. > https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=0&l... Yes, I guess we could try invalidating RendererImpl weak pointers at the beginning of ~RendererImpl destruction to ensure RestartA/VRenderer don't get invoked, I'll give it a try. Although I still think that removing the DCHECK_EQ(state_, STATE_PLAYING); in the Restart* methods is a good idea too, since we might get track status notifications even before playback is started. But I guess I can do that as a separate change with a separate test case.
On 2017/01/04 19:00:52, servolk wrote: > https://codereview.chromium.org/2570773002/diff/20001/media/renderers/rendere... > File media/renderers/renderer_impl.cc (right): > > https://codereview.chromium.org/2570773002/diff/20001/media/renderers/rendere... > media/renderers/renderer_impl.cc:114: state_ = STATE_SHUTDOWN; > On 2017/01/04 18:29:09, xhwang wrote: > > Instead of adding a new state, does it make sense to invalid all weak pointers > > so that RendererImpl::RestartVideoRenderer() (the callback) will never be > called > > during destruction? > > > > Note that all callbacks are bound using weak pointers, e.g. > > > https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=0&l... > > Yes, I guess we could try invalidating RendererImpl weak pointers at the > beginning of ~RendererImpl destruction to ensure RestartA/VRenderer don't get > invoked, I'll give it a try. > Although I still think that removing the DCHECK_EQ(state_, STATE_PLAYING); in > the Restart* methods is a good idea too, since we might get track status > notifications even before playback is started. But I guess I can do that as a > separate change with a separate test case. The check is fine. Since we'll never call Restart*() in the dtor (due to invalided weak ptrs) the DCHECK should be valid.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/04 19:02:57, xhwang wrote: > On 2017/01/04 19:00:52, servolk wrote: > > > https://codereview.chromium.org/2570773002/diff/20001/media/renderers/rendere... > > File media/renderers/renderer_impl.cc (right): > > > > > https://codereview.chromium.org/2570773002/diff/20001/media/renderers/rendere... > > media/renderers/renderer_impl.cc:114: state_ = STATE_SHUTDOWN; > > On 2017/01/04 18:29:09, xhwang wrote: > > > Instead of adding a new state, does it make sense to invalid all weak > pointers > > > so that RendererImpl::RestartVideoRenderer() (the callback) will never be > > called > > > during destruction? > > > > > > Note that all callbacks are bound using weak pointers, e.g. > > > > > > https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=0&l... > > > > Yes, I guess we could try invalidating RendererImpl weak pointers at the > > beginning of ~RendererImpl destruction to ensure RestartA/VRenderer don't get > > invoked, I'll give it a try. > > Although I still think that removing the DCHECK_EQ(state_, STATE_PLAYING); in > > the Restart* methods is a good idea too, since we might get track status > > notifications even before playback is started. But I guess I can do that as a > > separate change with a separate test case. > > The check is fine. Since we'll never call Restart*() in the dtor (due to > invalided weak ptrs) the DCHECK should be valid. It's fine for now, but I wonder if I'll be able to find a way for Restart*() to be called while the renderer is still in some different state, e.g. STATE_INITIALIZING or STATE_FLUSHING. I'll look at that later and will create a separate CL if necessary. PTAL at the latest patchset, it should be good now.
Just double check, did you check that the test fails without the fix? Otherwise, LGTM! Thanks for the fix!
On 2017/01/04 19:37:52, xhwang wrote: > Just double check, did you check that the test fails without the fix? > > Otherwise, LGTM! Thanks for the fix! Thanks for the review! Yes, I did check that without the fix PipelineStoppedWhileVideoRestartPending test case crashes. The PipelineStoppedWhileAudioRestartPending test case passes with or without this change, because unlike video renderer the ~AudioRendererImpl doesn't actually trigger the pending AudioRendererImpl::flush_cb_. But I still think that the audio test case is good to have for symmetry and to ensure that we have test coverage for this situation if we decide to call AudioRendererImpl::flush_cb_ from audio renderer destructor in the future.
The CQ bit was unchecked by servolk@chromium.org
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2570773002/#ps40001 (title: "Invalidate weak RendererImpl pointers, instead of introducing a new renderer state")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483559166665370,
"parent_rev": "9d9cf36ed7905fa519fb55bd5cea02579b244148", "commit_rev":
"6811925daada62e232c096381e55e60dab9ce38a"}
Message was sent while issue was closed.
Description was changed from ========== Fix crash when pipeline is stopped while video restart is pending Media renderer might get destroyed (due to pipeline being stopped or suspended) while a video track restart is pending. When that happens the VideoRendererImpl has a pending flush_cb_ that will get called from video renderer destructor invoking RendererImpl::RestartVideoRenderer, so RestartVideoRenderer needs to actually try restarting the video renderer only if we are still in the playing state. BUG=668963 ========== to ========== Fix crash when pipeline is stopped while video restart is pending Media renderer might get destroyed (due to pipeline being stopped or suspended) while a video track restart is pending. When that happens the VideoRendererImpl has a pending flush_cb_ that will get called from video renderer destructor invoking RendererImpl::RestartVideoRenderer, so RestartVideoRenderer needs to actually try restarting the video renderer only if we are still in the playing state. BUG=668963 Review-Url: https://codereview.chromium.org/2570773002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix crash when pipeline is stopped while video restart is pending Media renderer might get destroyed (due to pipeline being stopped or suspended) while a video track restart is pending. When that happens the VideoRendererImpl has a pending flush_cb_ that will get called from video renderer destructor invoking RendererImpl::RestartVideoRenderer, so RestartVideoRenderer needs to actually try restarting the video renderer only if we are still in the playing state. BUG=668963 Review-Url: https://codereview.chromium.org/2570773002 ========== to ========== Fix crash when pipeline is stopped while video restart is pending Media renderer might get destroyed (due to pipeline being stopped or suspended) while a video track restart is pending. When that happens the VideoRendererImpl has a pending flush_cb_ that will get called from video renderer destructor invoking RendererImpl::RestartVideoRenderer, so RestartVideoRenderer needs to actually try restarting the video renderer only if we are still in the playing state. BUG=668963 Committed: https://crrev.com/a5c72b04df68fd0721447e9de3867bb7ccaa743d Cr-Commit-Position: refs/heads/master@{#441457} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a5c72b04df68fd0721447e9de3867bb7ccaa743d Cr-Commit-Position: refs/heads/master@{#441457} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
