Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740103005:
Fix premature access on m_fifo in AudioDestination. (Closed)
DescriptionFix premature access on m_fifo in AudioDestination.
In the AudioDestination construction process, WebAudioDevice can start
the thread even before m_fifo is ready. This may result a crash when
render() method is called by AudioDeviceThread.
This CL implements a speculative fix of the incorrect order of
initialization.
BUG=692423
TEST=(none)
Review-Url: https://codereview.chromium.org/2740103005
Cr-Commit-Position: refs/heads/master@{#456204}
Committed: https://chromium.googlesource.com/chromium/src/+/9d2ce527f901bc5c2175c783082ca8e8b964f1ad
Patch Set 1 : Initial commit #
Total comments: 8
Patch Set 2 : Addressing feedback #Patch Set 3 : Reverting changes in PushPullFIFO #Messages
Total messages: 22 (12 generated)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:51: #else Why change this now? No one has ever complained on Android that they weren't hearing anything. https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:80: CHECK(m_fifo); How can m_fifo not be null?
https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:51: #else On 2017/03/10 19:34:55, Raymond Toy wrote: > Why change this now? No one has ever complained on Android that they weren't > hearing anything. Okay. I am reverting this. https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:80: CHECK(m_fifo); On 2017/03/10 19:34:55, Raymond Toy wrote: > How can m_fifo not be null? Do you want me to remove this check then? I can certainly do that.
https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:80: CHECK(m_fifo); On 2017/03/10 20:57:36, hongchan wrote: > On 2017/03/10 19:34:55, Raymond Toy wrote: > > How can m_fifo not be null? > > Do you want me to remove this check then? I can certainly do that. AFAICT, the only way this can happen is if you run out of memory and that would cause a crash instead of returning null. https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (left): https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:112: #endif Why get rid of all of these prints? They might not show up in the crash reports but they seem useful if we could ever see the problem locally.
https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right): https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/AudioDestination.cpp:80: CHECK(m_fifo); On 2017/03/10 21:11:09, Raymond Toy wrote: > On 2017/03/10 20:57:36, hongchan wrote: > > On 2017/03/10 19:34:55, Raymond Toy wrote: > > > How can m_fifo not be null? > > > > Do you want me to remove this check then? I can certainly do that. > > AFAICT, the only way this can happen is if you run out of memory and that would > cause a crash instead of returning null. Removed. https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp (left): https://codereview.chromium.org/2740103005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/audio/PushPullFIFO.cpp:112: #endif On 2017/03/10 21:11:09, Raymond Toy wrote: > Why get rid of all of these prints? They might not show up in the crash reports > but they seem useful if we could ever see the problem locally. Hmm. I thought we agreed upon deleting these for the fix. I will put them back after M58 branching.
Description was changed from ========== Fix premature access on m_fifo in AudioDestination. In the AudioDestination construction process, WebAudioDevice can start the thread even before m_fifo is ready. This may result a crash when render() method is called by AudioDeviceThread. This CL implements a speculative fix of the incorrect order of initialization. BUG=692423 TEST=(none) ========== to ========== Fix premature access on m_fifo in AudioDestination. In the AudioDestination construction process, WebAudioDevice can start the thread even before m_fifo is ready. This may result a crash when render() method is called by AudioDeviceThread. This CL implements a speculative fix of the incorrect order of initialization. BUG=692423 TEST=(none) ==========
hongchan@chromium.org changed reviewers: + amineer@chromium.org
amineer@ Could you take a look at this CL and merge if okay?
The CQ bit was checked by hongchan@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.
The CQ bit was checked by hongchan@chromium.org
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": 1489186088895420,
"parent_rev": "b21b7a3b7323da005040915a6a2498b4bb08f6e0", "commit_rev":
"9d2ce527f901bc5c2175c783082ca8e8b964f1ad"}
Message was sent while issue was closed.
Description was changed from ========== Fix premature access on m_fifo in AudioDestination. In the AudioDestination construction process, WebAudioDevice can start the thread even before m_fifo is ready. This may result a crash when render() method is called by AudioDeviceThread. This CL implements a speculative fix of the incorrect order of initialization. BUG=692423 TEST=(none) ========== to ========== Fix premature access on m_fifo in AudioDestination. In the AudioDestination construction process, WebAudioDevice can start the thread even before m_fifo is ready. This may result a crash when render() method is called by AudioDeviceThread. This CL implements a speculative fix of the incorrect order of initialization. BUG=692423 TEST=(none) Review-Url: https://codereview.chromium.org/2740103005 Cr-Commit-Position: refs/heads/master@{#456204} Committed: https://chromium.googlesource.com/chromium/src/+/9d2ce527f901bc5c2175c783082c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9d2ce527f901bc5c2175c783082c... |
