New issue
Advanced search Search tips

Issue 719129 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: ----



Sign in to add a comment

[Crash Reporting] Potential race between uploading minidumps and appending logcat data.

Project Member Reported by isherman@chromium.org, May 6 2017

Issue description

Currently, a file named "foo.dmp" might simultaneously be scheduled for logcat data to be appended to it, and for upload to the server.  It's a fairly unlikely race, but it could in theory result in duplicated uploads or additional crashes (depending on whether the file is deleted out from under a thread trying to read it, or whether it happens to be successfully read by both threads).
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0

commit 6a32f0233caef0ad160bb4c3adcc25bb091ab4d0
Author: isherman <isherman@chromium.org>
Date: Thu May 18 00:48:28 2017

[Crash Reporting] Implement a more direct bridge for extracting logcat output.

When a child process crashes, Chrome attempts to attach recent logcat
output to the minidump for the crash. Previously, this was implemented
via a FileObserver on the crash reports directory: When the file observer
detected a file moved into the directory, and saw that the filename
matched the appropriate regex pattern, it would attempt to append logcat
output. This has two drawbacks:
  (1) It's spooky action at a distance, which makes the code harder to
      reason about.
  (2) There's a super subtle constraint encoded into the "moved to" check:
      When the browser process crashes, a crash dump matching the same
      file pattern appears in the observed directory, but it is *created*
      rather than *moved*. Thus, this check is intentionally excluding
      browser crashes, for which the code might otherwise have a race
      between trying to attach logcat output and tearing down the main
      Java process.

This CL replaces the FileObserver with a direct call from the C++ code
that processes the minidump into the Java code that extracts the logcat.
(Well, there's some indirection to get through the Chrome layering onion,
but the flow is still much more linear.)

BUG= 719129 
TEST=CrashDumpManagerTest.*
R=gsennton@chromium.org

Review-Url: https://codereview.chromium.org/2878193002
Cr-Commit-Position: refs/heads/master@{#472613}

[modify] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/chrome/android/BUILD.gn
[modify] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/chrome/android/java/DEPS
[delete] https://crrev.com/dede6efa1ae80ebb11885c649a610548485d3979/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java
[modify] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
[modify] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/chrome/android/java_sources.gni
[add] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/components/crash/android/BUILD.gn
[add] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/components/crash/android/DEPS
[add] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/components/crash/android/OWNERS
[add] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
[add] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/components/crash/android/javatests/src/org/chromium/components/crash/browser/CrashDumpManagerTest.java
[modify] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/components/crash/content/browser/BUILD.gn
[modify] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/components/crash/content/browser/DEPS
[modify] https://crrev.com/6a32f0233caef0ad160bb4c3adcc25bb091ab4d0/components/crash/content/browser/crash_dump_manager_android.cc

Project Member

Comment 2 by bugdroid1@chromium.org, May 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4b0825f3059a63da4210b9e8fa1d53271d60b0d

commit d4b0825f3059a63da4210b9e8fa1d53271d60b0d
Author: isherman <isherman@chromium.org>
Date: Fri May 19 21:16:11 2017

[Crash Reporting] Fix a race between uploading and appending logcat data.

Prior to this CL, files ending with ".dmp" could potentially be simulatenously
scheduled for appending logcat data and scheduled for upload. This CL resolves
the race by requiring a ".try0" suffix to be present prior to attempting an
upload.

BUG= 719129 
R=gsennton@chromium.org

Review-Url: https://codereview.chromium.org/2862353003
Cr-Commit-Position: refs/heads/master@{#473319}

[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/android_webview/java/src/org/chromium/android_webview/crash/AwMinidumpUploaderDelegate.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/chrome/android/java/src/org/chromium/chrome/browser/crash/ChromeMinidumpUploaderDelegate.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/chrome/android/javatests/src/org/chromium/chrome/browser/crash/MinidumpUploadServiceTest.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderDelegate.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashFileManagerTest.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploaderImplTest.java
[modify] https://crrev.com/d4b0825f3059a63da4210b9e8fa1d53271d60b0d/components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/TestMinidumpUploaderDelegate.java

http://crrev.com/2878193002 created a new Java dependency (//components/crash/android:java) for //components/crash/content/browser but only added the dependency to //chrome/android:chrome_java. There are other Android targets that depend on //components/crash/content/browser, for example, //chromecast/browser.

It is difficult to determine exactly which targets it needs to be added to since the android_library and source_set (or whatever C++ target) are not directly related.
Status: Fixed (was: Started)

Sign in to add a comment