842421 - Make GeckoAsyncTask cancellable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: Refactor GeckoAsyncTask

Our GeckoAsyncTask is basically a crippled reimplementation of AsyncTask; we could simply extend AsyncTask to get cancellation and other features.

Besides being able to run on the given UI thread, GeckoAsyncTask has a couple of other differences from Android's AsyncTask. One is its priority feature, but this is only used in one place in TabsAccessor (which is actually unused obsolete code being removed by bug 842395).

The other difference is that it runs the background operation on a specific thread (which is always GeckoBackgroundThread). Switching to the implementation in this patch means we may get a bit of a performance boost since AsyncTask uses a thread pool to distribute tasks; our single handler we were using previously means that tasks were being queued behind other tasks, even though there weren't necessarily any execution dependencies. Of course, if there are places where we rely on using a specific handler to guarantee AsyncTask execution order, this patch will break that. I didn't notice any such cases where we might have this problem, but we could always add a second argument (for the background Handler to use) back to the constructor if necessary.

I also renamed GeckoAsyncTask -> HandlerAsyncTask as I felt this was a more sensible name (since this class has nothing to do with Gecko).

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I think we depend on the queued nature of GeckoBackgroundThread for all DB operations. In fact, that's why I started to replace other uses of plain old AsyncTask in the first place.

We have an ungodly amount of crashes due to locked DB issues, which we think could be due to multiple threads trying to access the DB at the same time.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: alt patch

So I offer up my refactor of GeckoAsyncTask, whcih adds support for "cancelling". With this refactor, I can replace the use of AsyncTask in Favicons, which executes three DB operations in a background thread, two of which are not GeckoBackgroundThread. The other one I already band-aided.

This patch removes the band-aid and adds support for "cancelling", including the onCancelling handler which runs on the UI thread just like AsyncTask.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 715318 [details] [diff] [review]
alt patch

Brian - can you take a look at the Favicon parts?

I did add a Logs in onCancelled to make sure it was getting hit.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 715294 [details] [diff] [review]
Refactor GeckoAsyncTask

Review of attachment 715294 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mark Finkle (:mfinkle) from comment #1)
> I think we depend on the queued nature of GeckoBackgroundThread for all DB
> operations.

If this is true, then it's a strong argument to r- this patch.

However I also don't like inheriting potentially wacky behaviour from AsyncTask. If you look at the AsyncTask documentation (and specifically the section on Threading Rules), it has a bunch of requirements with respect to what thread stuff can be created on and what thread stuff should be called on. It's not clear to me that if we just extend from AsyncTask we can throw these rules out the window. For example, with the new HandlerAsyncTask, if you create it on a non-UI thread and call execute() on a non-UI thread, what thread does onPreExecute() run on? What if you call execute from the UI thread? And so on. I think this sort of unclear behaviour can introduce subtle and tricky-to-debug threading issues, and we have enough of those already.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 715318 [details] [diff] [review]
alt patch

Review of attachment 715318 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with the idea here but mCancelled needs to be protected with synchronization.

::: mobile/android/base/GeckoAsyncTask.java
@@ +14,5 @@
>  public abstract class GeckoAsyncTask<Params, Progress, Result> {
>      public enum Priority { NORMAL, HIGH };
>  
>      private final Activity mActivity;
> +    private boolean mCancelled = false;

Since this variable is accessed from multiple threads I think it needs to be guarded with synchronization. One possible failure mode right now is if it changes during the call to onPreExecute(). AIUI if that happens the task should still run, followed by onCancelled(), but with this implementation it will just run onPreExecute() and then stop. Any cleanup-related things in onPostExecute() or onCancelled() will never run.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: alt patch v2

This patch removes isCancelled cheks for onPreExecute and doInBackground. Those are always called. The patch also changes mCancelled to a volatile so it can more sadely be read/written from multiple threads.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 715533 [details] [diff] [review]
alt patch v2

Review of attachment 715533 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the GeckoAsyncTask bits

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 715533 [details] [diff] [review]
alt patch v2

Review of attachment 715533 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoAsyncTask.java
@@ +60,5 @@
>          return this;
>      }
>  
> +    @SuppressWarnings({"UnusedParameters"})
> +    public final boolean cancel(boolean mayInterruptIfRunning) {

I don't understand this part. mayInterruptIfRunning isn't used locally, and the method is final so it can't be overridden. What's the point of this boolean?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> Comment on attachment 715533 [details] [diff] [review]
> alt patch v2
> 
> Review of attachment 715533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoAsyncTask.java
> @@ +60,5 @@
> >          return this;
> >      }
> >  
> > +    @SuppressWarnings({"UnusedParameters"})
> > +    public final boolean cancel(boolean mayInterruptIfRunning) {
> 
> I don't understand this part. mayInterruptIfRunning isn't used locally, and
> the method is final so it can't be overridden. What's the point of this
> boolean?

I just kept the interface used by AsyncTask. We could remove that in a followup, if wanted.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c36abb35fb9b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c36abb35fb9b