Do not call SendMessageToNativeLog on Android in AudioInputSyncWriter

content/browser/renderer_host/media/audio_input_sync_writer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/media/audio_input_sync_writer.h"
 
 #include <algorithm>
 

[Feng Qian, 2014/05/20 15:53:12]

#if defined(OS_ANDROID)
#include "base/android/jni_android.h"
#endif

Otherwise it won't compile.

 #include "base/memory/shared_memory.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 
 static const uint32 kLogDelayThreadholdMs = 500;
 
 namespace content {
 
 AudioInputSyncWriter::AudioInputSyncWriter(
     base::SharedMemory* shared_memory,
     int shared_memory_segment_count)
@@ skipping 24 matching lines @@
     base::TimeDelta interval = base::Time::Now() - creation_time_;
     oss << "Audio input data received for the first time: delay = "
         << interval.InMilliseconds() << "ms.";
   } else {
     base::TimeDelta interval = base::Time::Now() - last_write_time_;
     if (interval.InMilliseconds() > kLogDelayThreadholdMs) {
       oss << "Audio input data delay unexpectedly long: delay = "
          << interval.InMilliseconds() << "ms.";
     }
   }
-  if (!oss.str().empty())
+  if (!oss.str().empty()) {
     MediaStreamManager::SendMessageToNativeLog(oss.str());

[Feng Qian, 2014/05/19 23:27:01]

I felt it is safer to disabling logging instead of calling DetachFromVM because
we are unsure the performance implication.

[jiayl, 2014/05/19 23:30:20]

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?

[Feng Qian, 2014/05/20 15:53:12]

thanks for explanation, sounds fine to me.

On 2014/05/19 23:30:20, jiayl wrote:

 
+    // MediaStreamManager::SendMessageToNativeLog posts a task to the UI thread,

[Feng Qian, 2014/05/20 15:53:12]

Should comments be inside #if defined(OS_ANDROID)?

+    // 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 (crbug/365915). So we detach
+    // here to make sure the thread exits clean.
+#if defined(OS_ANDROID)
+    base::android::DetachFromVM();

[tommi (sloooow) - chröme, 2014/05/20 12:28:29]

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?

[Feng Qian, 2014/05/20 15:53:12]

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:

[jiayl, 2014/05/20 15:56:14]

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.

+#endif
+  }
+
   last_write_time_ = base::Time::Now();
 
   uint8* ptr = static_cast<uint8*>(shared_memory_->memory());
   ptr += current_segment_id_ * shared_memory_segment_size_;
   media::AudioInputBuffer* buffer =
       reinterpret_cast<media::AudioInputBuffer*>(ptr);
   buffer->params.volume = volume;
   buffer->params.size = size;
   buffer->params.key_pressed = key_pressed;
   memcpy(buffer->audio, data, size);
@@ skipping 32 matching lines @@
     base::ProcessHandle process_handle,
     base::FileDescriptor* foreign_handle) {
   foreign_handle->fd = foreign_socket_->handle();
   foreign_handle->auto_close = false;
   return (foreign_handle->fd != -1);
 }
 
 #endif
 
 }  // namespace content