Closed Bug 842421 Opened 9 years ago Closed 9 years ago

Make GeckoAsyncTask cancellable

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: bnicholson, Assigned: mfinkle)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Refactor GeckoAsyncTask (obsolete) — Splinter Review
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).
Attachment #715294 - Flags: review?(bugmail.mozilla)
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.
Attached patch alt patch (obsolete) — Splinter Review
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.
Attachment #715318 - Flags: review?(bugmail.mozilla)
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.
Attachment #715318 - Flags: review?(bnicholson)
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.
Attachment #715294 - Flags: review?(bugmail.mozilla) → review-
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.
Attachment #715318 - Flags: review?(bugmail.mozilla) → review-
Attached patch alt patch v2Splinter Review
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.
Attachment #715318 - Attachment is obsolete: true
Attachment #715318 - Flags: review?(bnicholson)
Attachment #715533 - Flags: review?(bugmail.mozilla)
Attachment #715533 - Flags: review?(bnicholson)
Comment on attachment 715533 [details] [diff] [review]
alt patch v2

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

r=me for the GeckoAsyncTask bits
Attachment #715533 - Flags: review?(bugmail.mozilla) → review+
Attachment #715294 - Attachment is obsolete: true
Summary: Refactor GeckoAsyncTask → Make GeckoAsyncTask cancellable
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?
Attachment #715533 - Flags: review?(bnicholson) → review+
(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.
Assignee: bnicholson → mark.finkle
https://hg.mozilla.org/mozilla-central/rev/c36abb35fb9b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.