Skip to content

Commit

Permalink
CronetLogger: ensure logging calls always fail gracefully
Browse files Browse the repository at this point in the history
To ensure logging calls can never result in a user crash, wrap every
logging call into a try-catch(Exception) statement.

Bug: b:226554121
Change-Id: Id434af3dd94d23c37118c3e3fa7ff4a89aa82bc9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3773496
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Stefano Duo <stefanoduo@google.com>
Cr-Commit-Position: refs/heads/main@{#1028258}
  • Loading branch information
StefanoDuo authored and Chromium LUCI CQ committed Jul 26, 2022
1 parent 7df64bf commit 29359f1
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,14 @@ private CronetTrafficInfo buildCronetTrafficInfo() {
private void maybeReportMetrics() {
if (mMetrics != null) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
mLogger.logCronetTrafficInfo(mCronetEngineId, buildCronetTrafficInfo());
try {
mLogger.logCronetTrafficInfo(mCronetEngineId, buildCronetTrafficInfo());
} catch (RuntimeException e) {
// Handle any issue gracefully, we should never crash due failures while
// logging.
Log.e(CronetUrlRequestContext.LOG_TAG,
"Error while trying to log CronetTrafficInfo: ", e);
}
}

final RequestFinishedInfo requestInfo = new RequestFinishedInfoImpl(mInitialUrl,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,14 +204,13 @@ public CronetUrlRequestContext(final CronetEngineBuilderImpl builder) {

mLogger = CronetLoggerFactory.createLogger(builder.getContext(), getCronetSource());

// getVersionString()'s output looks like "Cronet/w.x.y.z@hash". CronetVersion only cares
// about the "w.x.y.z" bit.
String version = getVersionString();
version = version.split("/")[1];
version = version.split("@")[0];

mLogger.logCronetEngineCreation(getCronetEngineId(), new CronetEngineBuilderInfo(builder),
new CronetVersion(version), getCronetSource());
try {
mLogger.logCronetEngineCreation(getCronetEngineId(),
new CronetEngineBuilderInfo(builder), buildCronetVersion(), getCronetSource());
} catch (RuntimeException e) {
// Handle any issue gracefully, we should never crash due failures while logging.
Log.e(LOG_TAG, "Error while trying to log CronetEngine creation: ", e);
}

// Init native Chromium URLRequestContext on init thread.
CronetLibraryLoader.postToInitThread(new Runnable() {
Expand Down Expand Up @@ -310,6 +309,15 @@ public String getVersionString() {
return "Cronet/" + ImplVersion.getCronetVersionWithLastChange();
}

private CronetVersion buildCronetVersion() {
String version = getVersionString();
// getVersionString()'s output looks like "Cronet/w.x.y.z@hash". CronetVersion only cares
// about the "w.x.y.z" bit.
version = version.split("/")[1];
version = version.split("@")[0];
return new CronetVersion(version);
}

@Override
public void shutdown() {
if (mInUseStoragePath != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import static android.os.Process.THREAD_PRIORITY_BACKGROUND;
import static android.os.Process.THREAD_PRIORITY_MORE_FAVORABLE;

import android.util.Log;

import org.chromium.net.BidirectionalStream;
import org.chromium.net.ExperimentalBidirectionalStream;
import org.chromium.net.NetworkQualityRttListener;
Expand Down Expand Up @@ -39,6 +41,8 @@
* <p>Does not support netlogs, transferred data measurement, bidistream, cache, or priority.
*/
public final class JavaCronetEngine extends CronetEngineBase {
private static final String TAG = JavaCronetEngine.class.getSimpleName();

private final String mUserAgent;
private final ExecutorService mExecutorService;
private final int mCronetEngineId;
Expand Down Expand Up @@ -71,13 +75,13 @@ public void run() {
});
mLogger = CronetLoggerFactory.createLogger(
builder.getContext(), CronetSource.CRONET_SOURCE_FALLBACK);
// getVersionString()'s output looks like "Cronet/w.x.y.z@hash". CronetVersion only cares
// about the "w.x.y.z" bit.
String version = getVersionString();
version = version.split("/")[1];
version = version.split("@")[0];
mLogger.logCronetEngineCreation(mCronetEngineId, new CronetEngineBuilderInfo(builder),
new CronetVersion(version), CronetSource.CRONET_SOURCE_FALLBACK);
try {
mLogger.logCronetEngineCreation(mCronetEngineId, new CronetEngineBuilderInfo(builder),
buildCronetVersion(), CronetSource.CRONET_SOURCE_FALLBACK);
} catch (RuntimeException e) {
// Handle any issue gracefully, we should never crash due failures while logging.
Log.e(TAG, "Error while trying to log JavaCronetEngine creation: ", e);
}
}

int getCronetEngineId() {
Expand Down Expand Up @@ -129,6 +133,15 @@ public String getVersionString() {
return "CronetHttpURLConnection/" + ImplVersion.getCronetVersionWithLastChange();
}

private CronetVersion buildCronetVersion() {
String version = getVersionString();
// getVersionString()'s output looks like "Cronet/w.x.y.z@hash". CronetVersion only cares
// about the "w.x.y.z" bit.
version = version.split("/")[1];
version = version.split("@")[0];
return new CronetVersion(version);
}

@Override
public void shutdown() {
mExecutorService.shutdown();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -964,7 +964,13 @@ private CronetTrafficInfo buildCronetTrafficInfo() {
// after Callback's onSucceeded, onFailed and onCanceled.
private void maybeReportMetrics() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
mLogger.logCronetTrafficInfo(mCronetEngineId, buildCronetTrafficInfo());
try {
mLogger.logCronetTrafficInfo(mCronetEngineId, buildCronetTrafficInfo());
} catch (RuntimeException e) {
// Handle any issue gracefully, we should never crash due failures while
// logging.
Log.e(TAG, "Error while trying to log CronetTrafficInfo: ", e);
}
}
}

Expand Down

0 comments on commit 29359f1

Please sign in to comment.