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

Issue 294803003: Do not call SendMessageToNativeLog on Android in AudioInputSyncWriter (Closed)

Created:
6 years, 7 months ago by jiayl
Modified:
6 years, 7 months ago
CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@temp
Visibility:
Public.

Description

Do not call SendMessageToNativeLog on Android in AudioInputSyncWriter MediaStreamManager::SendMessageToNativeLog posts a task to the UI thread, which will attach the audio thread to the Android java VM. Unlike chrome created threads, the audio thread is owned by the OS and does not detach itself from the VM on exit, causing a crash. The fix is to not call SendMessageToNativeLog on Android because webrtc logging is never enabled on Android due to the lack of extension. BUG=365915 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271764 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272309

Patch Set 1 : #

Total comments: 8

Patch Set 2 : #

Patch Set 3 : try another fix #

Total comments: 5

Patch Set 4 : #

Total comments: 4

Patch Set 5 : #

Total comments: 1

Patch Set 6 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -4 lines) Patch
M content/browser/renderer_host/media/audio_input_sync_writer.cc View 1 2 3 4 5 2 chunks +8 lines, -4 lines 0 comments Download

Messages

Total messages: 35 (0 generated)
jiayl
PTAL
6 years, 7 months ago (2014-05-19 23:14:15 UTC) #1
Feng Qian
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode54 content/browser/renderer_host/media/audio_input_sync_writer.cc:54: MediaStreamManager::SendMessageToNativeLog(oss.str()); I felt it is safer to disabling logging ...
6 years, 7 months ago (2014-05-19 23:27:00 UTC) #2
jiayl
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode54 content/browser/renderer_host/media/audio_input_sync_writer.cc:54: MediaStreamManager::SendMessageToNativeLog(oss.str()); On 2014/05/19 23:27:01, Feng Qian wrote: > I ...
6 years, 7 months ago (2014-05-19 23:30:19 UTC) #3
tommi (sloooow) - chröme
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode62 content/browser/renderer_host/media/audio_input_sync_writer.cc:62: base::android::DetachFromVM(); Just so that I understand - from information ...
6 years, 7 months ago (2014-05-20 12:28:29 UTC) #4
Feng Qian
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode8 content/browser/renderer_host/media/audio_input_sync_writer.cc:8: #if defined(OS_ANDROID) #include "base/android/jni_android.h" #endif Otherwise it won't compile. ...
6 years, 7 months ago (2014-05-20 15:53:11 UTC) #5
jiayl
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode62 content/browser/renderer_host/media/audio_input_sync_writer.cc:62: base::android::DetachFromVM(); On 2014/05/20 12:28:29, tommi wrote: > Just so ...
6 years, 7 months ago (2014-05-20 15:56:13 UTC) #6
tommi (sloooow) - chröme
Ah, thanks, that explains it. Lgtm. On May 20, 2014 5:56 PM, <jiayl@chromium.org> wrote: > ...
6 years, 7 months ago (2014-05-20 16:10:51 UTC) #7
tommi (sloooow) - chröme
lgtm
6 years, 7 months ago (2014-05-20 16:11:15 UTC) #8
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 7 months ago (2014-05-20 17:34:48 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/294803003/40001
6 years, 7 months ago (2014-05-20 17:36:10 UTC) #10
commit-bot: I haz the power
Change committed as 271764
6 years, 7 months ago (2014-05-20 21:43:25 UTC) #11
jiayl
The first fix was reverted due a crash on the build bot (http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/20509/steps/androidwebview_instrumentation_tests/logs/stdio). This is ...
6 years, 7 months ago (2014-05-21 16:18:05 UTC) #12
jiayl
Adding Ami since tommi may be offline.
6 years, 7 months ago (2014-05-21 17:40:20 UTC) #13
Ami GONE FROM CHROMIUM
https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode60 content/browser/renderer_host/media/audio_input_sync_writer.cc:60: // the IO thread first to avoid attaching the ...
6 years, 7 months ago (2014-05-21 17:51:41 UTC) #14
jiayl
Feng, could you take a look at the new fix? https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode60 ...
6 years, 7 months ago (2014-05-21 17:59:07 UTC) #15
Ami GONE FROM CHROMIUM
https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode55 content/browser/renderer_host/media/audio_input_sync_writer.cc:55: #if defined(OS_ANDROID) vrk@ points out that logging is not ...
6 years, 7 months ago (2014-05-21 18:03:38 UTC) #16
Feng Qian
https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode60 content/browser/renderer_host/media/audio_input_sync_writer.cc:60: // the IO thread first to avoid attaching the ...
6 years, 7 months ago (2014-05-21 18:08:33 UTC) #17
jiayl
On 2014/05/21 18:03:38, Ami Fischman wrote: > https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc > File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): > > https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode55 ...
6 years, 7 months ago (2014-05-21 18:13:05 UTC) #18
jiayl
https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode60 content/browser/renderer_host/media/audio_input_sync_writer.cc:60: // the IO thread first to avoid attaching the ...
6 years, 7 months ago (2014-05-21 18:13:58 UTC) #19
Feng Qian
https://codereview.chromium.org/294803003/diff/100001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/100001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode11 content/browser/renderer_host/media/audio_input_sync_writer.cc:11: #include "content/public/browser/browser_thread.h" This is unnecessary now. https://codereview.chromium.org/294803003/diff/100001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode55 content/browser/renderer_host/media/audio_input_sync_writer.cc:55: if ...
6 years, 7 months ago (2014-05-21 18:23:29 UTC) #20
jiayl
https://codereview.chromium.org/294803003/diff/100001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/100001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode11 content/browser/renderer_host/media/audio_input_sync_writer.cc:11: #include "content/public/browser/browser_thread.h" On 2014/05/21 18:23:30, Feng Qian wrote: > ...
6 years, 7 months ago (2014-05-21 18:29:21 UTC) #21
Ami GONE FROM CHROMIUM
LGTM https://codereview.chromium.org/294803003/diff/110001/content/browser/renderer_host/media/audio_input_sync_writer.cc File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/110001/content/browser/renderer_host/media/audio_input_sync_writer.cc#newcode39 content/browser/renderer_host/media/audio_input_sync_writer.cc:39: static const uint32 kLogDelayThreadholdMs = 500; FWIW now ...
6 years, 7 months ago (2014-05-21 18:38:38 UTC) #22
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 7 months ago (2014-05-21 18:47:58 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/294803003/130001
6 years, 7 months ago (2014-05-21 19:56:55 UTC) #24
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are ...
6 years, 7 months ago (2014-05-22 08:41:40 UTC) #25
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-05-22 08:45:20 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered_tests/builds/9689)
6 years, 7 months ago (2014-05-22 08:45:21 UTC) #27
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 7 months ago (2014-05-22 15:52:31 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/294803003/130001
6 years, 7 months ago (2014-05-22 15:53:59 UTC) #29
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are ...
6 years, 7 months ago (2014-05-22 15:57:36 UTC) #30
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-05-22 15:59:51 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered_tests/builds/9759)
6 years, 7 months ago (2014-05-22 15:59:52 UTC) #32
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 7 months ago (2014-05-22 20:37:51 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/294803003/130001
6 years, 7 months ago (2014-05-22 20:39:45 UTC) #34
commit-bot: I haz the power
6 years, 7 months ago (2014-05-22 20:41:48 UTC) #35
Message was sent while issue was closed.
Change committed as 272309

Powered by Google App Engine
This is Rietveld 408576698