Skip to content

Commit

Permalink
PartnerBrowserCustomizations#initializeAsync one at a time
Browse files Browse the repository at this point in the history
Prevent starting another #initializeAsync when one is still in progress. This CL 1) fixed cases where any of the mInitializeAsyncCallbacks tries to read #isInitialized, and 2) make those case more resource efficient so we are not running same tasks within short period of time.

To understand the impact, the described situation can happen when receiving a VIEW intent.

(cherry picked from commit 9d31994)

Bug: 1453433
Change-Id: I837518232970b0d0cda4d910f1fb47e6102ee402
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4602628
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Donn Denman <donnd@chromium.org>
Commit-Queue: Wenyu Fu <wenyufu@chromium.org>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1155671}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4610072
Reviewed-by: Krishna Govind <govind@chromium.org>
Owners-Override: Krishna Govind <govind@chromium.org>
Commit-Queue: Theresa Sullivan <twellington@chromium.org>
Commit-Queue: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#714}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
fwy423 authored and Chromium LUCI CQ committed Jun 13, 2023
1 parent b691d36 commit e108ff2
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 17 deletions.
12 changes: 11 additions & 1 deletion chrome/browser/partnercustomizations/BUILD.gn
Expand Up @@ -97,16 +97,26 @@ android_library("unit_device_javatests") {
}

robolectric_library("junit") {
sources = [ "java/src/org/chromium/chrome/browser/partnercustomizations/PartnerCustomizationsUmaUnitTest.java" ]
sources = [
"java/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizationsRoboUnitTest.java",
"java/src/org/chromium/chrome/browser/partnercustomizations/PartnerCustomizationsUmaUnitTest.java",
]

deps = [
":delegate_public_impl_java",
":java",
":uma_java",
"//base:base_java",
"//base:base_java_test_support",
"//base:base_junit_test_support",
"//base/test:test_support_java",
"//chrome/browser/flags:java",
"//chrome/test/android:chrome_java_unit_test_support",
"//third_party/androidx:androidx_annotation_annotation_java",
"//third_party/junit:junit",
"//third_party/mockito:mockito_java",
"//url:gurl_java",
"//url:gurl_junit_shadows",
"//url:gurl_junit_test_support",
]
}
Expand Up @@ -8,6 +8,7 @@
import android.content.pm.ApplicationInfo;
import android.os.SystemClock;

import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;

import org.chromium.base.CommandLine;
Expand Down Expand Up @@ -58,7 +59,7 @@ public class PartnerBrowserCustomizations {
private volatile GURL mHomepage;
private volatile boolean mIncognitoModeDisabled;
private volatile boolean mBookmarksEditingDisabled;
private boolean mIsInitialized;
private @Nullable Boolean mIsInitialized;

private final List<Runnable> mInitializeAsyncCallbacks;
private PartnerHomepageListener mListener;
Expand Down Expand Up @@ -165,7 +166,7 @@ public boolean isBookmarksEditingDisabled() {
* to read provider is also considered initialization.
*/
public boolean isInitialized() {
return mIsInitialized;
return mIsInitialized != null && mIsInitialized;
}

/**
Expand All @@ -185,6 +186,11 @@ public void initializeAsync(final Context context) {
*/
@VisibleForTesting
void initializeAsync(final Context context, long timeoutMs) {
if (mIsInitialized != null && !mIsInitialized) {
Log.w(TAG, "Another initializeAsync is already in progress.");
return;
}

mIsInitialized = false;
// Setup an initializing async task.
final AsyncTask<Void> initializeAsyncTask = new AsyncTask<Void>() {
Expand Down Expand Up @@ -235,13 +241,13 @@ protected void onCancelled(Void result) {
}

private void onFinalized() {
boolean isFirstFinalized = !mIsInitialized;
assert mIsInitialized != null;
assert !mIsInitialized;

mIsInitialized = true;
if (isFirstFinalized) {
RecordHistogram.recordTimesHistogram(
"Android.PartnerBrowserCustomizationInitDuration",
SystemClock.elapsedRealtime() - mStartTime);
}
RecordHistogram.recordTimesHistogram(
"Android.PartnerBrowserCustomizationInitDuration",
SystemClock.elapsedRealtime() - mStartTime);

for (Runnable callback : mInitializeAsyncCallbacks) {
callback.run();
Expand All @@ -251,11 +257,9 @@ private void onFinalized() {
if (mHomepageUriChanged && mListener != null) {
mListener.onHomepageUpdate();
}
if (isFirstFinalized) {
RecordHistogram.recordTimesHistogram(
"Android.PartnerBrowserCustomizationInitDuration.WithCallbacks",
SystemClock.elapsedRealtime() - mStartTime);
}
RecordHistogram.recordTimesHistogram(
"Android.PartnerBrowserCustomizationInitDuration.WithCallbacks",
SystemClock.elapsedRealtime() - mStartTime);
}
};

Expand Down Expand Up @@ -326,7 +330,7 @@ void refreshBookmarksEditingDisabled(CustomizationProviderDelegate delegate) {
* @param callback This is called when the initialization is done.
*/
public void setOnInitializeAsyncFinished(final Runnable callback) {
if (mIsInitialized) {
if (isInitialized()) {
PostTask.postTask(TaskTraits.UI_DEFAULT, callback);
} else {
mInitializeAsyncCallbacks.add(callback);
Expand All @@ -345,12 +349,12 @@ public void setOnInitializeAsyncFinished(final Runnable callback, long timeoutMs

PostTask.postDelayedTask(TaskTraits.UI_DEFAULT, () -> {
if (mInitializeAsyncCallbacks.remove(callback)) {
if (!mIsInitialized) {
if (!isInitialized()) {
Log.w(TAG, "mInitializeAsyncCallbacks executed as timeout expired.");
}
callback.run();
}
}, mIsInitialized ? 0 : timeoutMs);
}, isInitialized() ? 0 : timeoutMs);
}

public static void destroy() {
Expand Down
@@ -0,0 +1,119 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package org.chromium.chrome.browser.partnercustomizations;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import android.os.Handler;
import android.os.Looper;

import androidx.annotation.Nullable;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.annotation.Config;
import org.robolectric.annotation.Implementation;
import org.robolectric.annotation.Implements;
import org.robolectric.shadows.ShadowLooper;

import org.chromium.base.ContextUtils;
import org.chromium.base.task.TaskTraits;
import org.chromium.base.task.test.ShadowPostTask;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.chrome.browser.partnercustomizations.PartnerBrowserCustomizationsRoboUnitTest.ShadowCustomizationProviderDelegate;
import org.chromium.url.JUnitTestGURLs;
import org.chromium.url.ShadowGURL;

/**
* Unit tests for {@link PartnerBrowserCustomizations}.
*/
@RunWith(BaseRobolectricTestRunner.class)
@Config(shadows = {ShadowPostTask.class, ShadowGURL.class,
ShadowCustomizationProviderDelegate.class})
public class PartnerBrowserCustomizationsRoboUnitTest {
@Before
public void setup() {
ShadowCustomizationProviderDelegate.sHomepage = JUnitTestGURLs.EXAMPLE_URL;
ShadowPostTask.setTestImpl(new ShadowPostTask.TestImpl() {
final Handler mHandler = new Handler(Looper.getMainLooper());
@Override
public void postDelayedTask(@TaskTraits int taskTraits, Runnable task, long delay) {
mHandler.post(task);
}
});
}

@After
public void tearDown() {
PartnerBrowserCustomizations.destroy();
}

@Test
public void initializeAsyncOneAtATime() {
PartnerBrowserCustomizations.getInstance().initializeAsync(
ContextUtils.getApplicationContext());
// Run one task, so AsyncTask#doInBackground is run, but not #onFinalized.
ShadowLooper.runMainLooperOneTask();
assertFalse("The homepage refreshed, but result is not yet posted on UI thread.",
PartnerBrowserCustomizations.getInstance().isInitialized());

// Assuming homepage is changed during 1st #initializeAsync, and another #initializeAsync is
// triggered. The 2nd #initializeAsync is ignored since there's one already in the process.
ShadowCustomizationProviderDelegate.sHomepage = JUnitTestGURLs.GOOGLE_URL;
PartnerBrowserCustomizations.getInstance().initializeAsync(
ContextUtils.getApplicationContext());
assertFalse("#initializeAsync should be in progress.",
PartnerBrowserCustomizations.getInstance().isInitialized());

ShadowLooper.idleMainLooper();
assertTrue("#initializeAsync should be done.",
PartnerBrowserCustomizations.getInstance().isInitialized());
assertEquals("Homepage should be set via 1st initializeAsync.", JUnitTestGURLs.EXAMPLE_URL,
PartnerBrowserCustomizations.getInstance().getHomePageUrl().getSpec());

PartnerBrowserCustomizations.getInstance().initializeAsync(
ContextUtils.getApplicationContext());
assertFalse("3rd #initializeAsync should be in progress.",
PartnerBrowserCustomizations.getInstance().isInitialized());

ShadowLooper.idleMainLooper();
assertTrue("3rd #initializeAsync should be done.",
PartnerBrowserCustomizations.getInstance().isInitialized());
assertEquals("Homepage should refreshed by 3rd #initializeAsync.",
JUnitTestGURLs.GOOGLE_URL,
PartnerBrowserCustomizations.getInstance().getHomePageUrl().getSpec());
}

/**
* Convenient shadow class to set homepage provided by partner during robo tests.
*/
@Implements(CustomizationProviderDelegateUpstreamImpl.class)
public static class ShadowCustomizationProviderDelegate {
static String sHomepage;

public ShadowCustomizationProviderDelegate() {}

@Implementation
@Nullable
/** Returns the homepage string or null if none is available. */
protected String getHomepage() {
return sHomepage;
}
@Implementation
/** Returns whether incognito mode is disabled. */
protected boolean isIncognitoModeDisabled() {
return false;
}
@Implementation
/** Returns whether bookmark editing is disabled. */
protected boolean isBookmarksEditingDisabled() {
return false;
}
}
}

0 comments on commit e108ff2

Please sign in to comment.