Commit bce6119f authored by tedchoc's avatar tedchoc Committed by Commit bot

[Android] Fix timing issue in enabling/disabling printing.

There are a couple timing issues in the previous
implementation of the code that enables/disables the print
activity that is accessible via share.

1.) Multiple calls to enablePrintShareOption could result
    in the second call having the callback run immediately
    without the component first being enabled (since it
    would have been triggered in an AsyncTask in the first
    call).
2.) Back to back enable/disable calls could result in
    unpredictable execution ordering as they run their
    tasks on the thread pool instead of in serial.  Because
    the initial UI is only shown after the first callback is
    run, this would require a show, followed by a hide, followed
    by a show where the task enqueued by the hide is run after
    the enabling call in the second show.

To work around this (and due to the expected usage pattern of
this class), just wait for the task to be run in either of these
states to avoid this being possible.  It would also be possible
to use the SERIAL executor, but that might end up slowing down
the default share flow where we don't expect this interleaving
issue.

BUG=649453,664486

Review-Url: https://codereview.chromium.org/2523873003
Cr-Commit-Position: refs/heads/master@{#434676}
parent d7c801bb
......@@ -10,11 +10,13 @@ import android.content.Intent;
import android.content.pm.PackageManager;
import android.os.AsyncTask;
import android.os.Bundle;
import android.os.StrictMode;
import android.support.v7.app.AppCompatActivity;
import org.chromium.base.ActivityState;
import org.chromium.base.ApplicationStatus;
import org.chromium.base.ApplicationStatus.ActivityStateListener;
import org.chromium.base.Log;
import org.chromium.base.ThreadUtils;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ChromeActivity;
......@@ -26,15 +28,19 @@ import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ExecutionException;
/**
* A simple activity that allows Chrome to expose print as an option in the share menu.
*/
public class PrintShareActivity extends AppCompatActivity {
private static final String TAG = "cr_printing";
private static Set<Activity> sPendingShareActivities =
Collections.synchronizedSet(new HashSet<Activity>());
private static ActivityStateListener sStateListener;
private static AsyncTask<Void, Void, Void> sStateChangeTask;
/**
* Enable the print sharing option.
......@@ -61,8 +67,10 @@ public class PrintShareActivity extends AppCompatActivity {
ApplicationStatus.registerStateListenerForAllActivities(sStateListener);
boolean wasEmpty = sPendingShareActivities.isEmpty();
sPendingShareActivities.add(activity);
waitForPendingStateChangeTask();
if (wasEmpty) {
new AsyncTask<Void, Void, Void>() {
sStateChangeTask = new AsyncTask<Void, Void, Void>() {
@Override
protected Void doInBackground(Void... params) {
if (sPendingShareActivities.isEmpty()) return null;
......@@ -76,6 +84,11 @@ public class PrintShareActivity extends AppCompatActivity {
@Override
protected void onPostExecute(Void result) {
if (sStateChangeTask == this) {
sStateChangeTask = null;
} else {
waitForPendingStateChangeTask();
}
callback.run();
}
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
......@@ -85,11 +98,14 @@ public class PrintShareActivity extends AppCompatActivity {
}
private static void unregisterActivity(final Activity activity) {
ThreadUtils.assertOnUiThread();
sPendingShareActivities.remove(activity);
if (!sPendingShareActivities.isEmpty()) return;
ApplicationStatus.unregisterActivityStateListener(sStateListener);
new AsyncTask<Void, Void, Void>() {
waitForPendingStateChangeTask();
sStateChangeTask = new AsyncTask<Void, Void, Void>() {
@Override
protected Void doInBackground(Void... params) {
if (!sPendingShareActivities.isEmpty()) return null;
......@@ -100,9 +116,34 @@ public class PrintShareActivity extends AppCompatActivity {
PackageManager.DONT_KILL_APP);
return null;
}
@Override
protected void onPostExecute(Void result) {
if (sStateChangeTask == this) sStateChangeTask = null;
}
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
}
/**
* Waits for any pending state change operations to be completed.
*
* This will avoid timing issues described here: crbug.com/649453.
*/
private static void waitForPendingStateChangeTask() {
ThreadUtils.assertOnUiThread();
if (sStateChangeTask == null) return;
StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads();
try {
sStateChangeTask.get();
sStateChangeTask = null;
} catch (InterruptedException | ExecutionException e) {
Log.e(TAG, "Print state change task did not complete as expected");
} finally {
StrictMode.setThreadPolicy(oldPolicy);
}
}
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment