Skip to content

Commit

Permalink
Add debug exception reporting to GURL#ensureNativeInitializedForGURL
Browse files Browse the repository at this point in the history
Calls to GURL#ensureNativeInitializedForGURL are much more common than
anticipated (~7% of startups according to UMA), but I'm unable to hit
this during startup locally, so I'd like to collect stacks from Canary
to see where users are hitting this.

Bug: 783819
Change-Id: If9b3681c9b6c786be77c5f1e82c8a7c757fdecf0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2120916
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#753329}
  • Loading branch information
Michael Thiessen authored and Commit Bot committed Mar 25, 2020
1 parent ebf04de commit 8f77959
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@
import org.chromium.chrome.browser.vr.VrModuleProvider;
import org.chromium.components.embedder_support.application.FontPreloadingWorkaround;
import org.chromium.components.module_installer.util.ModuleUtil;
import org.chromium.components.version_info.Channel;
import org.chromium.components.version_info.VersionConstants;
import org.chromium.ui.base.ResourceBundle;
import org.chromium.url.GURL;

/**
* Basic application functionality that should be shared among all browser applications that use
Expand Down Expand Up @@ -124,6 +127,11 @@ protected void attachBaseContext(Context context) {

// Set Chrome factory for mapping BackgroundTask classes to TaskIds.
ChromeBackgroundTaskFactory.setAsDefault();

if (VersionConstants.CHANNEL == Channel.CANARY) {
GURL.setReportDebugThrowableCallback(
PureJavaExceptionReporter::reportJavaException);
}
}

// Write installed modules to crash keys. This needs to be done as early as possible so that
Expand All @@ -141,6 +149,7 @@ protected void attachBaseContext(Context context) {
PureJavaExceptionReporter::reportJavaException);
}
}

AsyncTask.takeOverAndroidThreadPool();
JNIUtils.setClassLoader(getClassLoader());
ResourceBundle.setAvailablePakLocales(
Expand Down
30 changes: 30 additions & 0 deletions url/android/java/src/org/chromium/url/GURL.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
import org.chromium.base.annotations.NativeMethods;
import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.task.PostTask;
import org.chromium.base.task.TaskTraits;

import java.util.Random;

/**
* A Java wrapper for GURL, Chromium's URL parsing library.
Expand All @@ -35,6 +39,15 @@ public class GURL {
/* package */ static final int SERIALIZER_VERSION = 1;
/* package */ static final char SERIALIZER_DELIMITER = '\0';

@FunctionalInterface
public interface ReportDebugThrowableCallback {
void run(Throwable throwable);
}

// Right now this is only collecting reports on Canary which has a relatively small population.
private static final int DEBUG_REPORT_PERCENTAGE = 10;
private static ReportDebugThrowableCallback sReportCallback;

// TODO(https://crbug.com/1039841): Right now we return a new String with each request for a
// GURL component other than the spec itself. Should we cache return Strings (as
// WeakReference?) so that callers can share String memory?
Expand Down Expand Up @@ -67,6 +80,16 @@ public GURL(String uri) {
@CalledByNative
protected GURL() {}

/**
* Enables debug stack trace gathering for GURL.
*
* TODO(https://crbug.com/783819): Remove this when the the fraction of users hitting this
* drops.
*/
public static void setReportDebugThrowableCallback(ReportDebugThrowableCallback callback) {
sReportCallback = callback;
}

/**
* Ensures that the native library is sufficiently loaded for GURL usage.
*
Expand All @@ -75,6 +98,13 @@ protected GURL() {}
*/
public static void ensureNativeInitializedForGURL() {
if (LibraryLoader.getInstance().isInitialized()) return;
if (sReportCallback != null && new Random().nextInt(100) < DEBUG_REPORT_PERCENTAGE) {
final Throwable throwable = new Throwable("This is not a crash, please ignore.");
// This isn't an assert, because by design this is possible, but this path is getting
// hit much more than expected and getting stack traces from the wild will help debug.
PostTask.postTask(
TaskTraits.BEST_EFFORT_MAY_BLOCK, () -> { sReportCallback.run(throwable); });
}
long time = SystemClock.elapsedRealtime();
LibraryLoader.getInstance().ensureMainDexInitialized();
RecordHistogram.recordTimesHistogram("Startup.Android.GURLEnsureMainDexInitialized",
Expand Down

0 comments on commit 8f77959

Please sign in to comment.