Fix a crash in the event tracing shutdown path

Fix a crash in the event tracing shutdown path

This CL fixes a crash that could happen when JSON event tracing is
shutting down. The cause of the crash was the fact that the logger
thread function was returning 'true', causing the platform thread to run
it repeatedly even though that wasn't the intention.

Usually the EventLogger::Stop() function would set the event requesting
the logging thread to clean up and close the file, and then immediately
call PlatformThread::Stop() which would stop the outer loop. The Log()
function would only run once and everything behaves as expected.

However, if a context switch happens between the shutdown_event_.Set()
and logging_thread_.Stop() calls in EventLogger::Stop(), the logger
thread function would close the file and exit the Log() method, while
PlatformThread will rerun it again. So the Log() function runs twice,
and the second time output_file_ is NULL which either causes the DCHECK
to fail (in debug builds) or the fprintf() to crash with SIGSEGV (in
release builds).

The fix simply changes the return value of the thread function to false
so it never runs twice.

R=pthatcher@webrtc.org

Committed: https://crrev.com/843b6f503f151753842d447b2d2c33ca548e01aa
Cr-Commit-Position: refs/heads/master@{#13510}

[skvlad, 2016-07-22 00:09:30]

skvlad@webrtc.org changed reviewers:
+ henrika@webrtc.org, pthatcher@webrtc.org

[pthatcher1, 2016-07-22 08:41:42]

lgtm

[skvlad, 2016-07-23 03:35:04]

The CQ bit was checked by skvlad@webrtc.org

[commit-bot: I haz the power, 2016-07-23 03:35:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-23 03:45:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-23 03:45:45]

Try jobs failed on following builders:
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[skvlad, 2016-07-23 04:45:50]

Description was changed from

==========
Fix a crash in the event tracing shutdown path

This CL fixes a crash that could happen when JSON event tracing is
shutting down. The cause of the crash was the fact that the logger
thread function was returning 'true', causing the platform thread to run
it repeatedly even though that wasn't the intention.

Usually the EventLogger::Stop() function would set the event requesting
the logging thread to clean up and close the file, and then immediately
call PlatformThread::Stop() which would stop the outer loop. The Log()
function would only run once and everything behaves as expected.

However, if a context switch happens between the shutdown_event_.Set()
and logging_thread_.Stop() calls in EventLogger::Stop(), the logger
thread function would close the file and exit the Log() method, while
PlatformThread will rerun it again. So the Log() function runs twice,
and the second time output_file_ is NULL which either causes the DCHECK
to fail (in debug builds) or the fprintf() to crash with SIGSEGV (in
release builds).

The fix simply changes the return value of the thread function to false
so it never runs twice.
==========

to

==========
Fix a crash in the event tracing shutdown path

This CL fixes a crash that could happen when JSON event tracing is
shutting down. The cause of the crash was the fact that the logger
thread function was returning 'true', causing the platform thread to run
it repeatedly even though that wasn't the intention.

Usually the EventLogger::Stop() function would set the event requesting
the logging thread to clean up and close the file, and then immediately
call PlatformThread::Stop() which would stop the outer loop. The Log()
function would only run once and everything behaves as expected.

However, if a context switch happens between the shutdown_event_.Set()
and logging_thread_.Stop() calls in EventLogger::Stop(), the logger
thread function would close the file and exit the Log() method, while
PlatformThread will rerun it again. So the Log() function runs twice,
and the second time output_file_ is NULL which either causes the DCHECK
to fail (in debug builds) or the fprintf() to crash with SIGSEGV (in
release builds).

The fix simply changes the return value of the thread function to false
so it never runs twice.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/843b6f503f151753842d447b2...
==========

[skvlad, 2016-07-23 04:45:52]

Committed patchset #1 (id:1) manually as
843b6f503f151753842d447b2d2c33ca548e01aa (presubmit successful).

[commit-bot: I haz the power, 2016-07-23 04:45:53]

Description was changed from

==========
Fix a crash in the event tracing shutdown path

This CL fixes a crash that could happen when JSON event tracing is
shutting down. The cause of the crash was the fact that the logger
thread function was returning 'true', causing the platform thread to run
it repeatedly even though that wasn't the intention.

Usually the EventLogger::Stop() function would set the event requesting
the logging thread to clean up and close the file, and then immediately
call PlatformThread::Stop() which would stop the outer loop. The Log()
function would only run once and everything behaves as expected.

However, if a context switch happens between the shutdown_event_.Set()
and logging_thread_.Stop() calls in EventLogger::Stop(), the logger
thread function would close the file and exit the Log() method, while
PlatformThread will rerun it again. So the Log() function runs twice,
and the second time output_file_ is NULL which either causes the DCHECK
to fail (in debug builds) or the fprintf() to crash with SIGSEGV (in
release builds).

The fix simply changes the return value of the thread function to false
so it never runs twice.

R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/843b6f503f151753842d447b2...
==========

to

==========
Fix a crash in the event tracing shutdown path

This CL fixes a crash that could happen when JSON event tracing is
shutting down. The cause of the crash was the fact that the logger
thread function was returning 'true', causing the platform thread to run
it repeatedly even though that wasn't the intention.

Usually the EventLogger::Stop() function would set the event requesting
the logging thread to clean up and close the file, and then immediately
call PlatformThread::Stop() which would stop the outer loop. The Log()
function would only run once and everything behaves as expected.

However, if a context switch happens between the shutdown_event_.Set()
and logging_thread_.Stop() calls in EventLogger::Stop(), the logger
thread function would close the file and exit the Log() method, while
PlatformThread will rerun it again. So the Log() function runs twice,
and the second time output_file_ is NULL which either causes the DCHECK
to fail (in debug builds) or the fprintf() to crash with SIGSEGV (in
release builds).

The fix simply changes the return value of the thread function to false
so it never runs twice.

R=pthatcher@webrtc.org

Committed: https://crrev.com/843b6f503f151753842d447b2d2c33ca548e01aa
Cr-Commit-Position: refs/heads/master@{#13510}
==========

[henrika_webrtc, 2016-07-25 11:46:48]

Don't know the detail well here. LGTM.

[skvlad, 2016-09-26 23:50:11]

Patchset #2 (id:20001) has been deleted