|
|
Created:
8 years, 7 months ago by enal Modified:
8 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionFix race condition caused by r114084. It added base::unretained(this) when scheduling
a task, as a result AudioOutputController can be deleted before task run, and it would
use junk instead of AudioSource.
BUG=124991
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Messages
Total messages: 9 (0 generated)
Tommi, can you please take a look?
https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_outp... File media/audio/audio_output_controller.cc (right): https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_outp... media/audio/audio_output_controller.cc:52: this, I don't think it's a good idea to do this in this particular case. You're running in the destructor, the reference count is 0. If you addref and release again, you will attempt double delete. https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_outp... media/audio/audio_output_controller.cc:84: &AudioOutputController::DoPlay, this)); The original reason for using Unretained() here was to avoid shifting references to the audiomanager's thread. If the final reference is released on that thread, then there were problems. If memory serves, the problem was that the AudioManager itself could subsequently have its last reference released, and hang since it would try to shut down the audio thread from the audio thread itself. However, I did make a change a while ago to make the manager non-ref counted, so I wonder if that problem has gone away? I'm not sure. Alternatively, could you use weak_this_.GetWeakPtr() here instead?
On 2012/05/02 07:56:18, tommi wrote: > https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_outp... > File media/audio/audio_output_controller.cc (right): > > https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_outp... > media/audio/audio_output_controller.cc:52: this, > I don't think it's a good idea to do this in this particular case. > You're running in the destructor, the reference count is 0. If you addref and > release again, you will attempt double delete. > > https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_outp... > media/audio/audio_output_controller.cc:84: &AudioOutputController::DoPlay, > this)); > The original reason for using Unretained() here was to avoid shifting references > to the audiomanager's thread. If the final reference is released on that > thread, then there were problems. > > If memory serves, the problem was that the AudioManager itself could > subsequently have its last reference released, and hang since it would try to > shut down the audio thread from the audio thread itself. > > However, I did make a change a while ago to make the manager non-ref counted, so > I wonder if that problem has gone away? I'm not sure. > > Alternatively, could you use weak_this_.GetWeakPtr() here instead? Weak pointer does not help when you have race condition involving 2 threads. Or you need some synchronization to avoid one thread deleting the object while other uses it, and rigid synchronization is hard, you cannot rely on in-object lock. Let me look at the code and think what can be done.
Just a thought but could we use PostTaskAndReply and bind the reference to the reply? Sent from my phone. Pardon brevity. On May 2, 2012 8:18 PM, <enal@google.com> wrote: > On 2012/05/02 07:56:18, tommi wrote: > > https://chromiumcodereview.**appspot.com/10261027/diff/1/** > media/audio/audio_output_**controller.cc<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc> > >> File media/audio/audio_output_**controller.cc (right): >> > > > https://chromiumcodereview.**appspot.com/10261027/diff/1/** > media/audio/audio_output_**controller.cc#newcode52<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc#newcode52> > >> media/audio/audio_output_**controller.cc:52: this, >> I don't think it's a good idea to do this in this particular case. >> You're running in the destructor, the reference count is 0. If you >> addref and >> release again, you will attempt double delete. >> > > > https://chromiumcodereview.**appspot.com/10261027/diff/1/** > media/audio/audio_output_**controller.cc#newcode84<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc#newcode84> > >> media/audio/audio_output_**controller.cc:84: &AudioOutputController::** >> DoPlay, >> this)); >> The original reason for using Unretained() here was to avoid shifting >> > references > >> to the audiomanager's thread. If the final reference is released on that >> thread, then there were problems. >> > > If memory serves, the problem was that the AudioManager itself could >> subsequently have its last reference released, and hang since it would >> try to >> shut down the audio thread from the audio thread itself. >> > > However, I did make a change a while ago to make the manager non-ref >> counted, >> > so > >> I wonder if that problem has gone away? I'm not sure. >> > > Alternatively, could you use weak_this_.GetWeakPtr() here instead? >> > > Weak pointer does not help when you have race condition involving 2 > threads. Or > you need some synchronization to avoid one thread deleting the object while > other uses it, and rigid synchronization is hard, you cannot rely on > in-object > lock. > > Let me look at the code and think what can be done. > > https://chromiumcodereview.**appspot.com/10261027/<https://chromiumcodereview... >
I've taken a closer look and I think your approach should be fine except for the one place in the destructor where I think we should just keep it as is and use base::Unretained. WDYT? On Wed, May 2, 2012 at 9:04 PM, Tommi <tommi@chromium.org> wrote: > Just a thought but could we use PostTaskAndReply and bind the reference to > the reply? > > Sent from my phone. Pardon brevity. > On May 2, 2012 8:18 PM, <enal@google.com> wrote: > >> On 2012/05/02 07:56:18, tommi wrote: >> >> https://chromiumcodereview.**appspot.com/10261027/diff/1/** >> media/audio/audio_output_**controller.cc<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc> >> >>> File media/audio/audio_output_**controller.cc (right): >>> >> >> >> https://chromiumcodereview.**appspot.com/10261027/diff/1/** >> media/audio/audio_output_**controller.cc#newcode52<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc#newcode52> >> >>> media/audio/audio_output_**controller.cc:52: this, >>> I don't think it's a good idea to do this in this particular case. >>> You're running in the destructor, the reference count is 0. If you >>> addref and >>> release again, you will attempt double delete. >>> >> >> >> https://chromiumcodereview.**appspot.com/10261027/diff/1/** >> media/audio/audio_output_**controller.cc#newcode84<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc#newcode84> >> >>> media/audio/audio_output_**controller.cc:84: &AudioOutputController::** >>> DoPlay, >>> this)); >>> The original reason for using Unretained() here was to avoid shifting >>> >> references >> >>> to the audiomanager's thread. If the final reference is released on that >>> thread, then there were problems. >>> >> >> If memory serves, the problem was that the AudioManager itself could >>> subsequently have its last reference released, and hang since it would >>> try to >>> shut down the audio thread from the audio thread itself. >>> >> >> However, I did make a change a while ago to make the manager non-ref >>> counted, >>> >> so >> >>> I wonder if that problem has gone away? I'm not sure. >>> >> >> Alternatively, could you use weak_this_.GetWeakPtr() here instead? >>> >> >> Weak pointer does not help when you have race condition involving 2 >> threads. Or >> you need some synchronization to avoid one thread deleting the object >> while >> other uses it, and rigid synchronization is hard, you cannot rely on >> in-object >> lock. >> >> Let me look at the code and think what can be done. >> >> https://chromiumcodereview.**appspot.com/10261027/<https://chromiumcodereview... >> >
Left base::Unretained() in destructor call (should figure out myself that we should not increment ref count there). On 2012/05/03 07:53:38, tommi wrote: > I've taken a closer look and I think your approach should be fine except > for the one place in the destructor where I think we should just keep it as > is and use base::Unretained. > WDYT? > > On Wed, May 2, 2012 at 9:04 PM, Tommi <mailto:tommi@chromium.org> wrote: > > > Just a thought but could we use PostTaskAndReply and bind the reference to > > the reply? > > > > Sent from my phone. Pardon brevity. > > On May 2, 2012 8:18 PM, <mailto:enal@google.com> wrote: > > > >> On 2012/05/02 07:56:18, tommi wrote: > >> > >> https://chromiumcodereview.**appspot.com/10261027/diff/1/** > >> > media/audio/audio_output_**controller.cc<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc> > >> > >>> File media/audio/audio_output_**controller.cc (right): > >>> > >> > >> > >> https://chromiumcodereview.**appspot.com/10261027/diff/1/** > >> > media/audio/audio_output_**controller.cc#newcode52<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc#newcode52> > >> > >>> media/audio/audio_output_**controller.cc:52: this, > >>> I don't think it's a good idea to do this in this particular case. > >>> You're running in the destructor, the reference count is 0. If you > >>> addref and > >>> release again, you will attempt double delete. > >>> > >> > >> > >> https://chromiumcodereview.**appspot.com/10261027/diff/1/** > >> > media/audio/audio_output_**controller.cc#newcode84<https://chromiumcodereview.appspot.com/10261027/diff/1/media/audio/audio_output_controller.cc#newcode84> > >> > >>> media/audio/audio_output_**controller.cc:84: &AudioOutputController::** > >>> DoPlay, > >>> this)); > >>> The original reason for using Unretained() here was to avoid shifting > >>> > >> references > >> > >>> to the audiomanager's thread. If the final reference is released on that > >>> thread, then there were problems. > >>> > >> > >> If memory serves, the problem was that the AudioManager itself could > >>> subsequently have its last reference released, and hang since it would > >>> try to > >>> shut down the audio thread from the audio thread itself. > >>> > >> > >> However, I did make a change a while ago to make the manager non-ref > >>> counted, > >>> > >> so > >> > >>> I wonder if that problem has gone away? I'm not sure. > >>> > >> > >> Alternatively, could you use weak_this_.GetWeakPtr() here instead? > >>> > >> > >> Weak pointer does not help when you have race condition involving 2 > >> threads. Or > >> you need some synchronization to avoid one thread deleting the object > >> while > >> other uses it, and rigid synchronization is hard, you cannot rely on > >> in-object > >> lock. > >> > >> Let me look at the code and think what can be done. > >> > >> > https://chromiumcodereview.**appspot.com/10261027/%3Chttps://chromiumcoderevi...> > >> > >
Lgtm. Thanks for fixing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enal@chromium.org/10261027/9001
Change committed as 135244 |