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

Issue 10261027: Fix race condition caused by r114084. (Closed)

Created:
8 years, 7 months ago by enal
Modified:
8 years, 7 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Fix 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 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -6 lines) Patch
M media/audio/audio_output_controller.cc View 1 2 chunks +6 lines, -6 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
enal1
Tommi, can you please take a look?
8 years, 7 months ago (2012-05-01 17:22:37 UTC) #1
tommi (sloooow) - chröme
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 media/audio/audio_output_controller.cc:52: this, I don't think it's a good idea to ...
8 years, 7 months ago (2012-05-02 07:56:18 UTC) #2
enal1
On 2012/05/02 07:56:18, tommi wrote: > 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 > ...
8 years, 7 months ago (2012-05-02 18:18:27 UTC) #3
tommi (sloooow) - chröme
Just a thought but could we use PostTaskAndReply and bind the reference to the reply? ...
8 years, 7 months ago (2012-05-02 19:04:46 UTC) #4
tommi (sloooow) - chröme
I've taken a closer look and I think your approach should be fine except for ...
8 years, 7 months ago (2012-05-03 07:53:38 UTC) #5
enal1
Left base::Unretained() in destructor call (should figure out myself that we should not increment ref ...
8 years, 7 months ago (2012-05-03 16:11:53 UTC) #6
tommi (sloooow) - chröme
Lgtm. Thanks for fixing.
8 years, 7 months ago (2012-05-03 20:16:41 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enal@chromium.org/10261027/9001
8 years, 7 months ago (2012-05-03 22:11:52 UTC) #8
commit-bot: I haz the power
8 years, 7 months ago (2012-05-03 23:57:13 UTC) #9
Change committed as 135244

Powered by Google App Engine
This is Rietveld 408576698