Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionDo 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 : #Messages
Total messages: 35 (0 generated)
PTAL
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:54: MediaStreamManager::SendMessageToNativeLog(oss.str()); I felt it is safer to disabling logging instead of calling DetachFromVM because we are unsure the performance implication.
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... 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 felt it is safer to disabling logging instead of calling DetachFromVM because > we are unsure the performance implication. This is only called for the first audio sample data, or when there is unexpectedly long delay. So for most of the times, it should be called once only. Should that make performance less a concern?
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:62: base::android::DetachFromVM(); Just so that I understand - from information in the bug, the problem occurs while calling SendMessageToNativeLog(). So, since you're adding this call _after_ SendMessageToNativeLog, does that mean that in the bug report, we were seeing a second call to SendMessageToNativeLog? If so, then that should be a very rare exception case according to jiayl. If not, then shouldn't this be done before the call to SendMessageToNativeLog?
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... 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. https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:54: MediaStreamManager::SendMessageToNativeLog(oss.str()); thanks for explanation, sounds fine to me. On 2014/05/19 23:30:20, jiayl wrote: > On 2014/05/19 23:27:01, Feng Qian wrote: > > I felt it is safer to disabling logging instead of calling DetachFromVM > because > > we are unsure the performance implication. > > This is only called for the first audio sample data, or when there is > unexpectedly long delay. So for most of the times, it should be called once > only. > Should that make performance less a concern? https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:56: // MediaStreamManager::SendMessageToNativeLog posts a task to the UI thread, Should comments be inside #if defined(OS_ANDROID)? https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:62: base::android::DetachFromVM(); tommi@, not sure if I understand your question correctly. SendMessageToNativeLog is called once. AttachCurrentThread can be called multiple times, in this case, each SendMessageToNativeLog calls it once. DetachFromVM only needs to be called once before exiting thread. Performance doesn't seem like an issue here. On 2014/05/20 12:28:29, tommi wrote: > Just so that I understand - from information in the bug, the problem occurs > while calling SendMessageToNativeLog(). So, since you're adding this call > _after_ SendMessageToNativeLog, does that mean that in the bug report, we were > seeing a second call to SendMessageToNativeLog? > > If so, then that should be a very rare exception case according to jiayl. If > not, then shouldn't this be done before the call to SendMessageToNativeLog?
https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/20001/content/browser/renderer... 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 that I understand - from information in the bug, the problem occurs > while calling SendMessageToNativeLog(). So, since you're adding this call > _after_ SendMessageToNativeLog, does that mean that in the bug report, we were > seeing a second call to SendMessageToNativeLog? > > If so, then that should be a very rare exception case according to jiayl. If > not, then shouldn't this be done before the call to SendMessageToNativeLog? The crash happens when the thread exists, not while calling SendMessageToNativeLog. SendMessageToNativeLog caused the thread attached to the VM and never detached, which caused a failure of 'threadExitCheck' in dalvik.
Ah, thanks, that explains it. Lgtm. On May 20, 2014 5:56 PM, <jiayl@chromium.org> wrote: > > 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 that I understand - from information in the bug, the problem >> > occurs > >> while calling SendMessageToNativeLog(). So, since you're adding this >> > call > >> _after_ SendMessageToNativeLog, does that mean that in the bug report, >> > we were > >> seeing a second call to SendMessageToNativeLog? >> > > If so, then that should be a very rare exception case according to >> > jiayl. If > >> not, then shouldn't this be done before the call to >> > SendMessageToNativeLog? > > > The crash happens when the thread exists, not while calling > SendMessageToNativeLog. > > SendMessageToNativeLog caused the thread attached to the VM and never > detached, which caused a failure of 'threadExitCheck' in dalvik. > > https://codereview.chromium.org/294803003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/294803003/40001
Message was sent while issue was closed.
Change committed as 271764
Message was sent while issue was closed.
The first fix was reverted due a crash on the build bot (http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...). This is a different fix by posting to the IO thread to avoid attaching the audio thread to the VM. PTAL!
Adding Ami since tommi may be offline.
https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:60: // the IO thread first to avoid attaching the audio thread to VM. I don't get it. How is it that the PostTask that MediaStreamManager::SendMessageToNativeLog does (and which is _all_ MSM::SMTNL() does) triggers an attachment of the calling thread to the JVM, while the PostTask here does not?
Feng, could you take a look at the new fix? https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:60: // the IO thread first to avoid attaching the audio thread to VM. On 2014/05/21 17:51:42, Ami Fischman wrote: > I don't get it. > How is it that the PostTask that MediaStreamManager::SendMessageToNativeLog does > (and which is _all_ MSM::SMTNL() does) triggers an attachment of the calling > thread to the JVM, while the PostTask here does not? See https://code.google.com/p/chromium/issues/detail?id=365915#c45. It seems only posting to the UI thread will cause VM attached.
https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:55: #if defined(OS_ANDROID) vrk@ points out that logging is not available on android anyway b/c of lack of extension API, so how about you just conditionalize l.65 on !defined(OS_ANDROID) and call it a day?
https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:60: // the IO thread first to avoid attaching the audio thread to VM. jiayl, can you build and test the new patch locally? It should give a definitive answer to Ami's question by testing the patch using instructions I sent you. On 2014/05/21 17:59:07, jiayl wrote: > On 2014/05/21 17:51:42, Ami Fischman wrote: > > I don't get it. > > How is it that the PostTask that MediaStreamManager::SendMessageToNativeLog > does > > (and which is _all_ MSM::SMTNL() does) triggers an attachment of the calling > > thread to the JVM, while the PostTask here does not? > > > See https://code.google.com/p/chromium/issues/detail?id=365915#c45. > It seems only posting to the UI thread will cause VM attached.
On 2014/05/21 18:03:38, Ami Fischman wrote: > https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... > File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): > > https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... > content/browser/renderer_host/media/audio_input_sync_writer.cc:55: #if > defined(OS_ANDROID) > vrk@ points out that logging is not available on android anyway b/c of lack of > extension API, so how about you just conditionalize l.65 on !defined(OS_ANDROID) > and call it a day? Haha. Done.
https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_sync_writer.cc:60: // the IO thread first to avoid attaching the audio thread to VM. On 2014/05/21 18:08:33, Feng Qian wrote: > jiayl, can you build and test the new patch locally? It should give a definitive > answer to Ami's question by testing the patch using instructions I sent you. > > On 2014/05/21 17:59:07, jiayl wrote: > > On 2014/05/21 17:51:42, Ami Fischman wrote: > > > I don't get it. > > > How is it that the PostTask that MediaStreamManager::SendMessageToNativeLog > > does > > > (and which is _all_ MSM::SMTNL() does) triggers an attachment of the calling > > > thread to the JVM, while the PostTask here does not? > > > > > > See https://code.google.com/p/chromium/issues/detail?id=365915#c45. > > It seems only posting to the UI thread will cause VM attached. > Code removed for Android now.
https://codereview.chromium.org/294803003/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/100001/content/browser/rendere... 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/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:55: if (!oss.str().empty()) { remove {}
https://codereview.chromium.org/294803003/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/100001/content/browser/rendere... 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: > This is unnecessary now. Done. https://codereview.chromium.org/294803003/diff/100001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:55: if (!oss.str().empty()) { On 2014/05/21 18:23:30, Feng Qian wrote: > remove {} Done.
LGTM https://codereview.chromium.org/294803003/diff/110001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/294803003/diff/110001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:39: static const uint32 kLogDelayThreadholdMs = 500; FWIW now that this is a function-static instead of a file-static you could static const base::TimeDelta kLogDelayThreshold = base::TimeDelta::FromMilliseconds(500); and avoid the need for InMilliseconds at l.49.
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/294803003/130001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/294803003/130001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/294803003/130001
Message was sent while issue was closed.
Change committed as 272309 |
