New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 843745 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 27
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 794637
issue 542151
issue 839637
issue 844809



Sign in to add a comment

Add Chrome-specific AsyncTask implementation

Project Member Reported by dskiba@chromium.org, May 16 2018

Issue description

So, my tasks on this are:

1) Switch all usages over to use our third_party copy
2) Bytecode check to see if android.os.AsyncTask is being used - Add error to enforce usage
3) Investigate prebuilt library usage of AsyncTask (consider bytecode rewrite)
4) Investigate framework usage of AsyncTask (see if we can call shutdown() to prevent it from using threads - not sure if we care)
5) See what's running for crashes (modifying SerialExecutor to give more information on queue full RejectedExecutionException)
6) Delete unused methods
7) Delete get() (convert existing usages to callbacks/promises)
8) Add tracing
9) Ensure exceptions can be seen from doInBackground()
10) Remove SerialExecutor and force usages to create own SerialExecutor, and have it run each task's onPostExecute before the next one's onPreExecute.


This will provide a way to deprecate features on AsyncTask and pave a way towards something task-schedulery (see issue 825947).

One alternative to cloning AsyncTask.java is to just use build rules to ban using its executors and also ban uses of .execute(), and .get(). The main advantage for cloning it that I see:
 * The logic of AsyncTask has changed over time, this will avoid quirks with variations in the OS's version.
 * It allows for more flexibility in terms of changing the api (if we want to)
 

Comment 1 by dskiba@chromium.org, May 16 2018

I've started an implementation here: crrev.com/c/1062604
Blocking: 542151
Note: We have a copy of android's AsyncTask checked in at 
//third_party/android_async_task/java/src/org/chromium/third_party/android/os/AsyncTask.java

I don't really care much whether we fork & edit vs. write our own entirely, but thought it'd be worth pointing out.
Blocking: 839637
Cc: -mariakho...@chromium.org
Owner: mariakho...@chromium.org
Status: Assigned (was: Available)

Comment 6 by dskiba@chromium.org, May 17 2018

BTW, it seems that Chrome doesn't use 'Progress' AsyncTask arguments - it might be a good idea to remove the argument and all progress-related function in a follow-up CL.

At least when I search cs/ for "extends AsyncTask<\w+,\s+[^V]" in p:clankium I get zero results (actually there are 2 results, but they are unrelated).
Blocking: 844809

Comment 8 by wnwen@chromium.org, May 28 2018

Cc: nyquist@chromium.org wnwen@chromium.org torne@chromium.org
Cc: estevenson@chromium.org
Blocking: 794637

Comment 11 by smaier@google.com, Jun 13 2018

Owner: smaier@chromium.org
Planning on picking this up now
Thanks, Sam! I made some progress on it, but have been pretty busy with other things and didn't finish. (Notice there's a CL in #1 linked above with TraceEvent wrapping stuff that Dmitry did, it's a nice start).

Some things that I found myself considering:
1) In addition to async tasks, there's a bunch of code that calls AsyncTask.THREAD_POOL_EXECUTOR.execute() directly with a runnable instead of async task. These need logging in some way too because those could be some of the tasks that are blocking the queue and leading to the RejectedExecutionException.

2) In release builds all the names are proguarded / obfuscated so consider how you're going to use the info. Andrew suggested that we just log them obfuscated and deobfuscate by hand. I also had to try various funky ways to get useful info about where the async task is posted from (I found that getting a stack trace works best, but not sure if that's performant).




Cc: mariakho...@chromium.org
Just as a summary there are two things that we need to fix:

1) We don't know what takes so long that we fill up the queue with tasks and get RejectedExecutionException. So we need to know what's currently running on all the threads in the queue when that exception occurs. That'll fix a bunch of the attached crashes.

2) Once we get rid of the issue, we should think about how to prevent it from happening in the future. Maybe add monitoring to the task execution lengths and alert when someone starts scheduling really long tasks?

3) When doing performance work, these tasks don't show up on the traces, but can contribute to latency, etc. So having trace events with the tasks is really nice.

Comment 14 by boliu@chromium.org, Jun 13 2018

Do people here prefer the AsyncTask interface or the simpler Handler.post/Executor.execute interface?

I always thought AsyncTask interface is kind of unweildy. Executor.execute is also closer to TaskScheduler in native code, if we want to one day merge them. So maybe we only need to implement our own Executor for now, and then convert existing AsyncTask usage to the simpler thing. Executor is also closer to base::TaskScheduler in native code.

Comment 15 by torne@chromium.org, Jun 14 2018

The AsyncTask interface is specifically useful for the case where you are doing an operation on a background thread that is expected to then return a result back to the UI thread (asynchronously), optionally also with incremental progress updates to the UI thread (also asynchronously). For that specific case it's pretty nice, but we have almost no uses of this in our codebase: virtually all AsyncTasks we use return void.

So, yeah, thinking about this now I think we should use Handler/Executor style interfaces where possible, and largely avoid AsyncTask entirely. That doesn't mean we shouldn't have a Chrome-specific AsyncTask still, for the cases where we *do* use those capabilities of AsyncTask, but it woudl be better to convert all the void-returning tasks to be simple Runnables and use some kind of Handler/Executor shaped interface as Bo says.

Comment 16 by wnwen@chromium.org, Jun 14 2018

Agreed with Torne. Another benefit of using Handler/Executor for all our use cases that doesn't require posting back to the UI thread is that AsyncTask's THREAD_POOL_EXECUTOR will be free to just focus on these UI-specific needs and be *much* less likely to block.
Today, tasks that are just runnables are generally scheduled on THREAD_POOL_EXECUTOR anyways (just directly instead of through the API). Are we proposing creating a separate thread pool? I would be wary of creating too many threads.

Comment 18 by torne@chromium.org, Jun 14 2018

What I think we'd like here is Chrome-specific APIs that don't explicitly use THREAD_POOL_EXECUTOR but probably also don't explicitly use a different executor either. For WebView it's very desirable to *not* share THREAD_POOL_EXECUTOR with the app because this can lead to deadlocks, but for Chrome the overhead of creating additional threads may not be worth it, so it'd be good to have a central place to make this decision instead of each individual callsite being responsible.

Comment 19 by boliu@chromium.org, Jun 14 2018

> Are we proposing creating a separate thread pool?

THREAD_POOL_EXECUTOR was causing all the problems for chrome too, right? It's the thing that has weird rejection rules. It's not clear if things are allowed to block or not.

On webview, it's shared with the app, which means we can't even enforce any rules, so we definitely don't want to use the same thread pool.

As for additional threads, *hopefully* once chrome stops using AsyncTask altogether, then the threads in THREAD_POOL_EXECUTOR don't need to spin up at all, but probably need to double check that's how the implementation works. And if not, then maybe this project by itself won't be good enough to ship, but once the new thing is combined with TaskScheduler in native, then there won't be more threads.

For webview, I think we'd happily pay the costs of more threads.

Comment 20 by torne@chromium.org, Jun 14 2018

I don't think there is any way to prevent the AsyncTask threads from being created - I believe this happens in the framework before app code is even running.

Comment 21 by torne@chromium.org, Jun 14 2018

(also there's likely other code we're including, from gmscore client library and similar, which may use asynctask internally in ways we can't easily avoid)
@Bo, yes rejection rules are weird, but something is also definitely off about our code if we have 9 threads running and 128 tasks queued already. We have lots of bug reports that show this state, but not at all clear what all these tasks are. In local testing I couldn't get even a 3rd of this number when starting up / loading a page.
Cc: skyos...@chromium.org
Glad to see this is moving forward. FYI, something we wanted to look into in Q3 is using base::TaskScheduler to back AsyncTasks so that do don't have multiple independent thread pools in the same process. This would also give us a way to prioritize some AsyncTasks over others. At that point we might want to consider changing the API too to something that fits better.
So, my tasks on this are:

1) Switch all usages over to use our third_party copy
2) Bytecode check to see if android.os.AsyncTask is being used - Add error to enforce usage
3) Investigate prebuilt library usage of AsyncTask (consider bytecode rewrite)
4) Investigate framework usage of AsyncTask (see if we can call shutdown() to prevent it from using threads - not sure if we care)
5) See what's running for crashes (modifying SerialExecutor to give more information on queue full RejectedExecutionException)
6) Delete unused methods
7) Delete get() (convert existing usages to callbacks/promises)
8) Add tracing

This should be independent of the scheduling work skyostil@ is planning, but it would be great if AsyncTask's API could be improved.

Comment 26 by boliu@chromium.org, Jun 18 2018

Should we also look into dealing with task rejection exceptions in this bug? eg crbug.com/844809 in the blocking list of this bug.

> This should be independent of the scheduling work skyostil@ is planning, but it would be great if AsyncTask's API could be improved.

I guess this excludes everyone's pet projects (including mine), but that's probably a good idea. Anyone want to throw out more ideas in that area probably should go to crbug.com/825947
Description: Show this description
FWIW, I think figuring out "what's running for crashes" bit should be higher priority than some of the other bits since we're continuing to crash for users in production.
Description: Show this description
Status: Started (was: Assigned)

Comment 31 by agrieve@google.com, Jun 28 2018

Description: Show this description
If we're going to clone, why not put it into base namespace and replace all references? This will also make it easy to prohibit usage of standard AsyncTask - just write simple presubmit check.
That's the plan. Initial CL here:
https://chromium-review.googlesource.com/c/chromium/src/+/1117637
I've come up with a list of android.os.AsyncTask usages that are shipped in our binary but not immediately obvious if/how I could switch them over to use our own implementation: https://docs.google.com/document/d/11sfusVCZbzkWq8eBnYa6qlXiBxqeCXDwzNzAGY769ak/edit?usp=sharing
Did a bunch more digging into this, and found that if we downcast AsyncTask.THREAD_POOL_EXECUTOR to ThreadPoolExecutor, then we can achieve some of the goals here without our own AsyncTask.

To log pending tasks when it fills up:
* TheadPoolExecutor.setRejectedExecutionHandler() to catch the event
* TheadPoolExecutor.getQueue() to inspect all tasks

To hijack all tasks and put them on our own threadpool:
* TheadPoolExecutor.shutdown() to cause all runnables to be rejected
* TheadPoolExecutor.setRejectedExecutionHandler() to receive all runnables
We could add our own tracing using this

To surface exceptions from doInBackground():
* Add try/catch around hijacked runnables.
* Or maybe: add Thread.setUncaughtExceptionHandler() via ThreadFactory.
* Or maybe even: Thread.setDefaultUncaughtExceptionHandler()

Problem with logging tasks:
* All AsyncTasks will show up as AsyncTask$1 (AsyncTask does not implement runnable)
* Can check if className.startsWith(AsyncTask$1), or use .getDeclaringClass() == AsyncTask.class
* And then use reflection on the "this$1" field to access it and call .getClass() on it.

So why create our own AsyncTask rather than use ErrorProne to prevent get():
* Older implementations do not call Binder.flushPendingCommands()
* Forces thread priority to BACKGROUND, and there's nothing we can do other than set it back again in our doInBackground for each task.

I'm a but uncertain as to whether these last two points are compelling enough to be creating our own AsyncTask. I'm leaning towards yes, as I think changing thread priority should be the job of the scheduler, not the task. Does anyone else have an opinion on this?
Thanks for digging into this. One reason I think our own AsyncTask would be warranted is when we want to extend the interface. For instance base::TaskScheduler can use task priorities for scheduling. To take advantage of that, we'd need a new Java API for priority annotation. Of course that doesn't have to be AsyncTask, but if that's the API folks are already using it seems like a natural choice.
The original reason to do this, which is still probably the most important, is so that WebView can use a separate thread pool than the embedding application to avoid deadlocks. We can't change the global behaviour of AsyncTask.THREAD_POOL_EXECUTOR in WebView - that would break apps.
For webview - we still could use the system's AsyncTask, so long as we change all of our call sites to use myTask.executeOnExecutor(SomeBaseClass.BASE_EXECUTOR).
Right, but the original idea here was to introduce our own version of AsyncTask that didn't have an executeOnExecutor() method, and just had an execute() that used the "right" executor (which may be different for webview vs chrome)
Project Member

Comment 40 by bugdroid1@chromium.org, Jul 3

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

commit a8bccdc9228f6b15279cb1564a3b1af0558406ab
Author: Sam Maier <smaier@chromium.org>
Date: Tue Jul 03 20:10:21 2018

Updating our version of AsyncTask

We are moving to Oreo's implementation, at commit
1488a3a19d4681a41fb45570c15e14d99db1cb66

TBR=agrieve  # Mechanical change to cronet

Bug:  843745 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ic612b7767007993a1b7761418061dbe0346afa7e
Reviewed-on: https://chromium-review.googlesource.com/1117637
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572331}
[modify] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/BUILD.gn
[modify] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/base/BUILD.gn
[add] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/base/test/android/junit/src/org/chromium/base/test/asynctask/BackgroundShadowAsyncTask.java
[add] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/base/test/android/junit/src/org/chromium/base/test/asynctask/CustomShadowAsyncTask.java
[modify] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/components/cronet/android/BUILD.gn
[delete] https://crrev.com/7110fa011e7621ced9b3aa2d3f88ebd27c0f1123/third_party/android_async_task/BUILD.gn
[modify] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/third_party/android_async_task/README.chromium
[rename] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/third_party/android_async_task/java/src/org/chromium/base/AsyncTask.java
[modify] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/third_party/robolectric/README.chromium
[add] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/third_party/robolectric/custom_asynctask/java/src/org/chromium/base/test/asynctask/ShadowAsyncTask.java
[add] https://crrev.com/a8bccdc9228f6b15279cb1564a3b1af0558406ab/third_party/robolectric/custom_asynctask/java/src/org/chromium/base/test/asynctask/ShadowAsyncTaskBridge.java

Cc: alexclarke@chromium.org
Project Member

Comment 42 by bugdroid1@chromium.org, Jul 5

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

commit bcccaef0db0a46ccae2eb0864fd9d56cab51c623
Author: Sam Maier <smaier@chromium.org>
Date: Thu Jul 05 15:40:54 2018

Switching usage of AsyncTask in chrome/ to use our own

Bug:  843745 
Change-Id: I93d52af0a70fc58255559d0d719e30fc23bf3e08
Reviewed-on: https://chromium-review.googlesource.com/1126160
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Sam Maier <smaier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572803}
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabModelSelector.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/cookies/CookiesFetcher.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/crypto/CipherFactory.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/customtabs/RequestThrottler.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/home/glue/FileDeletionQueue.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/home/list/CalendarFactory.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/ui/SpaceDisplay.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/download/ui/StorageSummary.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/feedback/AsyncFeedbackSourceAdapter.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/installedapp/InstalledAppProviderImpl.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaService.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksReader.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizations.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerTask.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/services/AccountsChangedReceiver.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/InvalidationGcmUpstreamSender.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninFragmentBase.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileRenderer.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/survey/ChromeSurveyController.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/StorageDelegate.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappAuthenticator.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/java/src/org/chromium/chrome/browser/widget/ThumbnailDiskStorage.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicyTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/javatests/src/org/chromium/chrome/browser/util/ChromeFileProviderTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/init/AsyncInitTaskRunnerTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/installedapp/InstalledAppProviderTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTrackerTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerUnitTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDirectoryManagerTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/webapk/libs/client/BUILD.gn
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/webapk/libs/client/DEPS
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManagerTest.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/components/signin/core/browser/android/java/src/org/chromium/components/signin/AccountManagerFacade.java
[modify] https://crrev.com/bcccaef0db0a46ccae2eb0864fd9d56cab51c623/components/signin/core/browser/android/junit/src/org/chromium/components/signin/test/AccountManagerFacadeTest.java

Horray, great to see some real progress on our own AsyncTask (after ~2-3 years of mulling it over on various bugs). Thanks for picking this up Sam!
I investigated upping the work queue size from 128 to something larger, but still have it reporting crashes (without crashing the app) when we exceed 128 tasks in queue.

This doesn't work (without some serious effort) because the two available exception reporters aren't set up for us:

PureJavaExceptionReporter - this one would be ideal for us, but it exists in chrome/ and not base/. It appears that pulling this out of base might be a pain, since it uses some chrome specific information (namely what channel - canary, dev, etc.).

JavaExceptionReporter - requires native to be loaded. Although odds are native will be loaded when the AsyncTask queue fills up, it's not a guarantee, and I was hitting issues where native wasn't loaded in my testing.

So, the plan for now is to keep the queue size fixed at 128, and continue to crash. However, we will start to improve the visibility of these crashes to shine a light on the causes by inspecting the queue. Perhaps when we have a better understanding on how the queue fills up, we can think about upping the queue size.
Can you just have asynctask define an interface that's implemented by an upper layer? Does that work?
Project Member

Comment 47 by bugdroid1@chromium.org, Jul 10

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

commit 62ca6db2b91f053fa1c1c9b7a105bb230ebea650
Author: Sam Maier <smaier@chromium.org>
Date: Tue Jul 10 19:07:41 2018

Updating AsyncTask usage in all other places in the code base

Bug:  843745 
Cq-Include-Trybots: luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win10_chromium_x64_rel_ng;master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ica93601caaee202f17754a653fc7e954d39b5811
Merge-With: eureka-internal/184486
Reviewed-on: https://chromium-review.googlesource.com/1126171
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Simeon Anfinrud <sanfin@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org>
Commit-Queue: Sam Maier <smaier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573844}
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/base/android/java/src/org/chromium/base/PathUtils.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/AsyncTaskRunner.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/chromecast/browser/android/junit/src/org/chromium/chromecast/shell/AsyncTaskRunnerTest.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTask.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/components/sync/test/android/javatests/src/org/chromium/components/sync/test/util/MockSyncContentResolverDelegate.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/content/public/android/java/src/org/chromium/content/browser/selection/SmartSelectionProvider.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/remoting/android/java/src/org/chromium/chromoting/base/OAuthTokenFetcher.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/services/shape_detection/android/java/src/org/chromium/shape_detection/FaceDetectionImpl.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/testing/android/junit/BUILD.gn
[delete] https://crrev.com/aef833510f1c295773f2a456db59e72d210c99fd/testing/android/junit/java/src/org/chromium/testing/local/BackgroundShadowAsyncTask.java
[delete] https://crrev.com/aef833510f1c295773f2a456db59e72d210c99fd/testing/android/junit/java/src/org/chromium/testing/local/CustomShadowAsyncTask.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/ui/android/java/src/org/chromium/ui/resources/ResourceExtractor.java
[modify] https://crrev.com/62ca6db2b91f053fa1c1c9b7a105bb230ebea650/ui/android/java/src/org/chromium/ui/resources/async/AsyncPreloadResourceLoader.java

Project Member

Comment 48 by bugdroid1@chromium.org, Jul 10

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

commit 84acc05f7fe0b6716fda159d9760e4fd384e0085
Author: Sam Maier <smaier@chromium.org>
Date: Tue Jul 10 20:11:51 2018

Android: Stealing Runnables from framework's AsyncTask

We are shutting down Android's built-in AsyncTask.THREAD_POOL_EXECUTOR for all ChromeApplication processes, and having the Runnables sent there fall through to our own ThreadPoolExecutor.

Currently, this ThreadPoolExecutor just matches the behavior of the framework's ThreadPoolExecutor, but having all Runnables fall through to our own ThreadPoolExecutor gives us flexibility to implement special functionality on the thread pool.

Bug:  843745 
Change-Id: Ie6d426601abc9a3bac975c28093f5d93d048d8be
Reviewed-on: https://chromium-review.googlesource.com/1128161
Commit-Queue: Sam Maier <smaier@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573877}
[modify] https://crrev.com/84acc05f7fe0b6716fda159d9760e4fd384e0085/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
[modify] https://crrev.com/84acc05f7fe0b6716fda159d9760e4fd384e0085/third_party/android_async_task/README.chromium
[modify] https://crrev.com/84acc05f7fe0b6716fda159d9760e4fd384e0085/third_party/android_async_task/java/src/org/chromium/base/AsyncTask.java

Project Member

Comment 49 by bugdroid1@chromium.org, Jul 13

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/81d53827c005893bb70653517c9c87f44a7b4142

commit 81d53827c005893bb70653517c9c87f44a7b4142
Author: Sam Maier <smaier@chromium.org>
Date: Fri Jul 13 19:20:41 2018

Description: Show this description
Is there a way to enforce that code loaded through Qwark also ends up using our AsyncTask?
My understanding is that the AGSA team generally has banned AsyncTask because of overuse and some OS versions had only one(?!) thread per pool. We should try to understand what their current solution is and how that fits into the browser scheduler work.
IIRC GMS had a study on it and now they use their own pooled executors, resulting in lower RAM usage especially when idle.
Oh, and they've banned AsyncTasks. :)
Project Member

Comment 55 by bugdroid1@chromium.org, Jul 16

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

commit 4aba67d247c5be8798ea763027f01e2c58dab1a2
Author: Andrew Grieve <agrieve@chromium.org>
Date: Mon Jul 16 18:45:36 2018

Android: AsyncTask shows popular tasks when queue is full

We are attempting to improve visiblity into why RejectedExecutionExceptions happen. This change includes any Runnable that appears more than 50 times into the exception message.

Bug:  843745 
Change-Id: Ifa383cfc0be3b7c58298ff48399d120bd7ab45ba
Reviewed-on: https://chromium-review.googlesource.com/1133776
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575366}
[modify] https://crrev.com/4aba67d247c5be8798ea763027f01e2c58dab1a2/base/BUILD.gn
[add] https://crrev.com/4aba67d247c5be8798ea763027f01e2c58dab1a2/base/android/javatests/src/org/chromium/base/AsyncTaskTest.java
[modify] https://crrev.com/4aba67d247c5be8798ea763027f01e2c58dab1a2/third_party/android_async_task/README.chromium
[modify] https://crrev.com/4aba67d247c5be8798ea763027f01e2c58dab1a2/third_party/android_async_task/java/src/org/chromium/base/AsyncTask.java

Project Member

Comment 56 by bugdroid1@chromium.org, Jul 24

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

commit 05ddc100a3a2ab3bdd166f9e0b7fb1574142f891
Author: Sam Maier <smaier@chromium.org>
Date: Tue Jul 24 15:26:11 2018

Android: Remove all Params usages in AsyncTask

This is step 1 in an API shrinking effort for AsyncTask. There is no reason for Params to exist instead of just using constructor arguments, so we are changing the few usages over. Step 2 will be the removal of the Params argument.

Bug:  843745 
Change-Id: I36a03dd91b7af10763587fb5048e9f8d775f203e
Reviewed-on: https://chromium-review.googlesource.com/1136730
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: Sam Maier <smaier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577550}
[modify] https://crrev.com/05ddc100a3a2ab3bdd166f9e0b7fb1574142f891/chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
[modify] https://crrev.com/05ddc100a3a2ab3bdd166f9e0b7fb1574142f891/chrome/android/java/src/org/chromium/chrome/browser/feedback/AsyncFeedbackSourceAdapter.java
[modify] https://crrev.com/05ddc100a3a2ab3bdd166f9e0b7fb1574142f891/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
[modify] https://crrev.com/05ddc100a3a2ab3bdd166f9e0b7fb1574142f891/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerTask.java
[modify] https://crrev.com/05ddc100a3a2ab3bdd166f9e0b7fb1574142f891/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerBitmapViewHolder.java
[modify] https://crrev.com/05ddc100a3a2ab3bdd166f9e0b7fb1574142f891/ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java
[modify] https://crrev.com/05ddc100a3a2ab3bdd166f9e0b7fb1574142f891/ui/android/junit/src/org/chromium/ui/base/SelectFileDialogTest.java

Project Member

Comment 58 by bugdroid1@chromium.org, Jul 25

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/ff528cdb618802f6a6c4e9ed5413d5289f61abcc

commit ff528cdb618802f6a6c4e9ed5413d5289f61abcc
Author: Sam Maier <smaier@chromium.org>
Date: Wed Jul 25 16:55:57 2018

Looking at .get() calls that are causing ANRs, it appears that almost every single ANR comes from .get() blocking on the SERIAL_EXECUTOR. In fact, I have yet to find an ANR which comes from an AsyncTask queued on the THREAD_POOL_EXECUTOR (I'm sure they exist, but they are infrequent and rare).

Some use cases have legitimate reasons for using the SERIAL_EXECUTOR - TabPersistentStore, for example. However, there are many places which seemingly unknowingly use "execute()" - it sure sounds like a reasonable function name if you want to execute your function, but unfortunately it defaults to SERIAL_EXECUTOR.

I am going to start switching usages of execute() to use an explicit executor, then I plan to remove execute() entirely.
I don't think that's the desired API surface for the WebView use case. Which explicit executor are you going to switch it to? It can't be either of AsyncTask's executors - the whole goal for webview here is to not share it with the app any more.

I assumed that what we were going to do was the *opposite*: switch all the usages to *not* use an explicit executor, and instead change the *default* executor to what we wanted.
I plan on switching all usages of execute() to use THREAD_POOL_EXECUTOR where possible, and SERIAL_EXECUTOR when they are relying on execute() happening serially.

Eventually, once it is clear we are departed from the AsyncTask API, we could add back a default which goes to THREAD_POOL_EXECUTOR.

Unfortunately, we do need an explicit executeOnExecutor function (or something which is similar) in the case where we require serial execution. Our plan is to have a good replacement for SERIAL_EXECUTOR (a la native task scheduler SequencedTaskRunner).

Earlier, you mentioned wanting execute() to do some logic to figure out where to run a task (based on Chrome or WebView). I believe (and correct me if I'm mistaken) that we already cover the WebView case here. Tasks should already be run in a separate thread pool if its in chromium code, and the OS thread pool is only shut down in ChromeApplication https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java?sq=package:chromium&g=0&l=113

This means that if you are submitting tasks to org.chromium.base.AsyncTask's executors, your tasks are not running on the host application's executors.

OK, so I did not realise that our implementation *wasn't* using the existing pools. So, now I have a bunch of different questions instead:

1) is it only chrome code that needs serial executor? does it get started even if nothing uses it?
2) are the thread names/etc exactly the same as the native android ones? (looks like they are, which may not be great for debugging)
3) not a question but ChromeApplication.onCreate does actually get called in webview renderer processes, I think? We don't have app code there, so it may be fine, but.. hmm.
1) The serial executor could be used by the framework/libraries/non-chrome code as well. However, the serial executor is just a very light wrapper around the thread pool executor. It doesn't allocate any meaningful amount of resources, so I believe it gets "started" (meaning it is a static class with an implicit default constructor) whether or not it is ever used.

2) Are you referring to the names of certain public functions and static variables (esp. THREAD_POOL_EXECUTOR and SERIAL_EXECUTOR)? If so, yes. We're trying to move away from the AsyncTask API, but until we have equal (or better) options than what exists in the AsyncTask API, we don't want to confuse developers by simply renaming well known APIs. Feel free to push back here if you think differently.

3) I'm not really an expert here. I imagine that if the WebView renderer process isn't shared with the host application's code, this won't cause issues?
1) I mean, how many additional threads did this change create, in a process? Does it create a thread to run serial executor tasks on, even if nobody ever executes a serial executor task? Also, what do you mean it could be used by framework/libraries/non-chrome code? Isn't it in org.chromium?

2) No, I'm referring to the actual names of the threads that get created, which are visible in a debugger or in crash/ANR dumps.
1) THREAD_POOL_EXECUTOR will create threads when it needs them. So, if there is no usage of the THREAD_POOL_EXECUTOR, there will be no threads created. The SERIAL_EXECUTOR (right now, and also in Android's AsyncTask) is simply a wrapper which sends tasks one by one into the THREAD_POOL_EXECUTOR. So, if nobody sends a task to the SERIAL_EXEUCTOR, the THREAD_POOL_EXECUTOR doesn't have to spin up a thread for it.

I misspoke earlier. The SERIAL_EXECUTOR itself will NOT be used by non-org.chromium code. However, android.os.AsyncTask.SERIAL_EXECUTOR will send its tasks to org.chromium.base.AsyncTask.THREAD_POOL_EXECUTOR (in any process which has run ChromeApplication.attachBaseContext()), since we shutdown Android's THREAD_POOL_EXECUTOR and handle the rejected tasks ourselves. The flow is:

   a) We set org.chromium.AsyncTask to be the rejectedExecutionHandler of android.os.THREAD_POOL_EXECUTOR
   b) We shutdown android.os.THREAD_POOL_EXECUTOR in ChromeApplication.attachBaseContext()
   c) framework code submits to android.os.SERIAL_EXEUCTOR
   d) android.os.SERIAL_EXECUTOR submits a task to android.os.THREAD_POOL_EXEUCTOR
   e) android.os.THREAD_POOL_EXECUTOR has been shut down so it rejects the task
   f) android.os.THREAD_POOL_EXECUTOR sends its rejected task to its rejectedExecutionHandler
   g) org.chromium.THREAD_POOL_EXECUTOR is given the task from the handler
   h) org.chromium.THREAD_POOL_EXECUTOR executes the task submitted to android.os.SERIAL_EXECUTOR

2) Sorry, threads have the same name as the Android AsyncTask but it's trivial to change - https://chromium-review.googlesource.com/c/chromium/src/+/1158398
Project Member

Comment 66 by bugdroid1@chromium.org, Aug 1

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

commit 57519c060ecbab348b1f02aa1097d067d12846ec
Author: Sam Maier <smaier@chromium.org>
Date: Wed Aug 01 16:22:04 2018

Android: changed name of threads created in AsyncTask

This is to give better visibility of the threads which are Chrome's and
threads which are Android's.

Bug:  843745 
Change-Id: Ie7f75cb915e70b13ef8957f7320d93d890f23a18
Reviewed-on: https://chromium-review.googlesource.com/1158398
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Peter Wen <wnwen@chromium.org>
Commit-Queue: Sam Maier <smaier@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579828}
[modify] https://crrev.com/57519c060ecbab348b1f02aa1097d067d12846ec/third_party/android_async_task/java/src/org/chromium/base/AsyncTask.java

OK - thanks a lot for the detailed explanation there; I haven't been following along with the implementation of this closely and I didn't realise we had effectively done all the things I wanted already :)

This all sounds good and I don't think there are any remaining issues for WebView that I know of. Thanks for also updating the thread names!
Project Member

Comment 68 by bugdroid1@chromium.org, Aug 3

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

commit 1a696109129c8382b0f756cfc365b2edf41f820c
Author: Sam Maier <smaier@chromium.org>
Date: Fri Aug 03 15:15:19 2018

Android: Removed all Params, Progress from AsyncTask

All changes (except for the changes to AsyncTask itself) are entirely
generated with Android Studio's refactor mechanism and git cl format.

This is in an effort to reduce the unused surface of AsyncTask that
doesn't match the ideal future API. Ideally, we will move to something
very similiar to TaskScheduler, which has no concept of "Progress
updates", and forces the usages to call Bind for the parameters.

For parameters now, we use constructor arguments, which provides the
same concurrency guarantees that passing in params via Params would.
This is closer to the ideal because it supports any arbitrary
parameters, not just many of 1 type.

TBR=smaier  #For mechanical refactoring

Bug:  843745 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: If25df5d7276e504983bdc71e6e07993cb044ed04
Reviewed-on: https://chromium-review.googlesource.com/1150692
Commit-Queue: Sam Maier <smaier@chromium.org>
Reviewed-by: Sam Maier <smaier@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580549}
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/android_webview/java/src/org/chromium/android_webview/services/AwVariationsSeedFetcher.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/base/android/java/src/org/chromium/base/PathUtils.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/base/android/javatests/src/org/chromium/base/AsyncTaskTest.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/base/test/android/junit/src/org/chromium/base/test/asynctask/BackgroundShadowAsyncTask.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/base/test/android/junit/src/org/chromium/base/test/asynctask/CustomShadowAsyncTask.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/DefaultBrowserInfo.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabModelSelector.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/cookies/CookiesFetcher.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabTabPersistencePolicy.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/RequestThrottler.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/dynamicmodule/ModuleLoader.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadDirectoryProvider.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/home/glue/FileDeletionQueue.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/home/list/CalendarFactory.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/ui/SpaceDisplay.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/download/ui/StorageSummary.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/feedback/AsyncFeedbackSourceAdapter.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/infobar/DuplicateDownloadInfoBar.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/installedapp/InstalledAppProviderImpl.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaUrlResolver.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaService.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/omaha/UpdateMenuItemHelper.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/VisibleNetworksTracker.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksReader.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizations.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/BitmapScalerTask.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/services/AccountsChangedReceiver.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/InvalidationGcmUpstreamSender.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninFragmentBase.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileRenderer.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/survey/ChromeSurveyController.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassin.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/StorageDelegate.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/vr/VrShellDelegate.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/java/src/org/chromium/chrome/browser/widget/ThumbnailDiskStorage.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/javatests/src/org/chromium/chrome/browser/ShareIntentTest.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/AsyncTaskRunner.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/components/signin/core/browser/android/java/src/org/chromium/components/signin/AccountManagerFacade.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/components/sync/test/android/javatests/src/org/chromium/components/sync/test/util/MockSyncContentResolverDelegate.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/content/public/android/java/src/org/chromium/content/browser/selection/SmartSelectionProvider.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/remoting/android/java/src/org/chromium/chromoting/base/OAuthTokenFetcher.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/third_party/android_async_task/README.chromium
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/third_party/android_async_task/java/src/org/chromium/base/AsyncTask.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/third_party/robolectric/custom_asynctask/java/src/org/chromium/base/test/asynctask/ShadowAsyncTask.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/third_party/robolectric/custom_asynctask/java/src/org/chromium/base/test/asynctask/ShadowAsyncTaskBridge.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/ui/android/java/src/org/chromium/ui/resources/ResourceExtractor.java
[modify] https://crrev.com/1a696109129c8382b0f756cfc365b2edf41f820c/ui/android/java/src/org/chromium/ui/resources/async/AsyncPreloadResourceLoader.java

Project Member

Comment 69 by bugdroid1@chromium.org, Aug 9

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/5914ea851ce06cc574066a0794b51b8ed1d6b012

commit 5914ea851ce06cc574066a0794b51b8ed1d6b012
Author: Sam Maier <smaier@chromium.org>
Date: Thu Aug 09 00:10:44 2018

Project Member

Comment 70 by bugdroid1@chromium.org, Aug 9

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/b900f31a83a738273008f1ebf5b8dda214fd79cb

commit b900f31a83a738273008f1ebf5b8dda214fd79cb
Author: Sam Maier <smaier@chromium.org>
Date: Thu Aug 09 17:01:15 2018

Status: Fixed (was: Started)
The work here has been mostly done, and the stuff that hasn't will be covered by crbug.com/863341:

1) DONE - Switch all usages over to use our third_party copy
2) DONE (didn't check in bytecode check, using ErrorProne check) - Bytecode check to see if android.os.AsyncTask is being used - Add error to enforce usage
3) DONE (not using bytecode rewriting, stealing tasks from android.os.AsyncTask instead) - Investigate prebuilt library usage of AsyncTask (consider bytecode rewrite)
4) DONE (same as 3) - Investigate framework usage of AsyncTask (see if we can call shutdown() to prevent it from using threads - not sure if we care)
5) DONE - See what's running for crashes (modifying SerialExecutor to give more information on queue full RejectedExecutionException)
6) DONE (we may delete more in the future, but we've deleted what's easy right now) - Delete unused methods
7) MITIGATED (can't delete get as the refactorings would be pretty heavy. Mitigated issue by moving as many tasks off SERIAL_EXECUTOR so get() isn't nearly as slow) - Delete get() (convert existing usages to callbacks/promises)
8) NOT DONE (this will hopefully come in with the UI Thread Scheduler change) -  Add tracing
9) DONE - Ensure exceptions can be seen from doInBackground()
10) NOT DONE (SequencedTaskRunner will be added with the UI Thread Scheduler, then refactoring to use it will happen here: crbug.com/877963) - Remove SerialExecutor and force usages to create own SerialExecutor, and have it run each task's onPostExecute before the next one's onPreExecute.
Any chance we could add some quick documentation regarding 2)? I.e. so people don't have to run errorprone to figure out what they ought to be doing?
Regarding #2, all the ErrorProne check does is prevent you from using "android.os.AsyncTask" and tells you (in the error message) that you should use "org.chromium.base.AsyncTask" instead. More documentation will be added when we get the Java equivalent of TaskScheduler implemented and landed.
OK. SGTM with waiting to the TaskScheduler framework in Java.
Thanks for the great work here Sam!

Would we still want to open a follow up bug to get rid of all usages of .get()?
With some retrospection after actually trying to get rid of .get() calls, it looks like actually getting rid of these calls will be quite difficult. get() tends to be used for warmed up resources that aren't needed right now, but will eventually be needed. This use case makes sense to me, and would be pretty tough to rewrite with callbacks.

Moving stuff off the SERIAL_EXECUTOR should help with this, as it will be much less likely now that a task submitted early won't be done by the time we need it.

Our next thought with .get() is to change it's behaviour to raise priority/run the task on the UI thread if the get() call is happening on the UI thread. Bug here: https://bugs.chromium.org/p/chromium/issues/detail?id=878529

Sign in to add a comment