bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

GeckoAsyncTask should run on GeckoBackgroundThread

RESOLVED WONTFIX

Status

()

Firefox for Android
General
RESOLVED WONTFIX
6 years ago
5 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
GeckoAsyncTask should always run on GeckoBackgroundThread. In case we want to use a separate Executor for this, we might be able to add it later easily.

Also, GeckoAsyncTask will post onPostExecute() on the thread it is constructed. So, it's the responsibility of the developer to instantiate on the UI thread, if onPostExecute() should be called on UI thread.
(Assignee)

Comment 1

6 years ago
Created attachment 668233 [details] [diff] [review]
Patch

This patch does the needful.
I am not sure why I should be calling handler.getLooper(). But AsyncTask does it (and read somewhere that we might need to do). I'm happy to remove it in case we find we don't need it.
Attachment #668233 - Flags: review?(cpeterson)
Comment on attachment 668233 [details] [diff] [review]
Patch

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

LGTM with nits.

::: mobile/android/base/GeckoAsyncTask.java
@@ +8,5 @@
>  import android.os.Handler;
>  
>  // AsyncTask runs onPostExecute on the thread it is constructed on
>  // We construct these off of the main thread, and we want that to run
>  // on the main UI thread, so this is a convenience class to do that

Please update this comment to reflect GeckoAsyncTask's new behavior.

@@ +17,5 @@
>      private Priority mPriority = Priority.NORMAL;
>  
> +    public GeckoAsyncTask() {
> +        mHandler = new Handler();
> +        mHandler.getLooper();

You can remove the call to getLooper(). AsyncTask calls getLooper() to force initialization of static variable AsyncTask.sHandler. We don't need to do this because our GeckoAsyncTask.mHandler is not static.

https://github.com/android/platform_frameworks_base/blob/master/core/java/android/os/AsyncTask.java#L267
Attachment #668233 - Flags: review?(cpeterson) → review+

Comment 4

6 years ago
https://hg.mozilla.org/mozilla-central/rev/628c6823da4f
https://hg.mozilla.org/mozilla-central/rev/4158281d3995
Assignee: nobody → sriram
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Depends on: 798858

Comment 5

6 years ago
Backed out on suspicion of causing the #1 mobile topcrasher bug 798858 (want to make the Nightly in an hour, so we can tell for sure):
https://hg.mozilla.org/mozilla-central/rev/d3113617c43a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 18 → ---
(Assignee)

Comment 6

6 years ago
Here are few design decisions to take:
1. AsyncTask "requires" the constructor to be created on UI thread. Thereby it posts the result back to UI thread.
2. GeckoAsyncTask was designed to start on any thread, but post result on UI thread. But, UI thread got messier where AwesomeBar was using GeckoApp.mAppContext's UI thread.

The above patch fixed the second part as:
GeckoAsyncTask will post the result back on the thread it was constructed.

But the catch is:
Of the 3 threads (UI, Gecko, GeckoBackground), we are trying to do background processing of tasks on GeckoBackground. So basically we are just trying to post a task from GeckoBackground thread to itself. Which is actually not needed.

So, the options:
1. We can make sure we construct the AsyncTask on UI thread, and we can happily get the result back in UI thread -- which is exactly what we want. Other chunks of code will happily execute in GeckoBackgroundThread as always.
--- or ---
2. We can have a separate thread for GeckoAsyncTasks, to be executed on GeckoAsyncThread, so, GeckoBackgroundThread can happily worry about just parsing JSON from Gecko.
(In reply to Sriram Ramasubramanian [:sriram] from comment #0)
> GeckoAsyncTask should always run on GeckoBackgroundThread. In case we want
> to use a separate Executor for this, we might be able to add it later easily.
> 

This bug confuses me. Currently GeckoAsyncTask does always run the "background" code on the GeckoBackgroundThread. Your changes didn't touch that behaviour at all. Instead, you changed GeckoAsyncTask to be more like AsyncTask in that it runs the post-execute code on the thread that GeckoAsyncTask was spawned from.

What is the goal you're trying to accomplish with this bug?
Instead, you changed GeckoAsyncTask to be more like
> AsyncTask in that it runs the post-execute code on the thread that
> GeckoAsyncTask was spawned from.
That is specifically not what GeckoAsyncTask was supposed to do.

> What is the goal you're trying to accomplish with this bug?
Hopefully not
(Assignee)

Comment 9

6 years ago
(In reply to Kartikaya Gupta (:kats) from comment #7)
> What is the goal you're trying to accomplish with this bug?

That! is exactly what I'm trying to figure out! :(
"Do we need a GeckoAsyncTask or not?"
So, about a year ago people were using AsyncTask wrong and assuming that it would always run the post-exectute on the UI thread. So I created GeckoAsyncTask such that it worked how everyone seemed to think AsyncTask was supposed to work. Given that a bunch of code has been written under that assumption at this point, changing the behavior to match that of AsyncTask would require a fair bit of auditing and rewriting callers.
(In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> "Do we need a GeckoAsyncTask or not?"

Yes we do. We need it for the same reason that it was originally added - it makes the code easier to write. We could accomplish the same end functionality without it, but we would have a lot more bugs if we did that.
(Assignee)

Comment 12

6 years ago
Using GeckoApp.mAppContext in AwesomeBar for getting the UI thread -- isn't the best way to make things easy.
(In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> Using GeckoApp.mAppContext in AwesomeBar for getting the UI thread -- isn't
> the best way to make things easy.

This isn't about making things easy, it is about making things right.
(Assignee)

Comment 14

6 years ago
(In reply to Brad Lassey [:blassey] from comment #13)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> > Using GeckoApp.mAppContext in AwesomeBar for getting the UI thread -- isn't
> > the best way to make things easy.
> 
> This isn't about making things easy, it is about making things right.

Which is exactly what I meant.

UI thread of AwesomeBar activity is derived from GeckoApp.mAppContext -- which is not right.
https://hg.mozilla.org/mozilla-central/file/22d192c5d1fd/mobile/android/base/AwesomeBar.java#l626
So what exactly is this bug trying to fix?
(Assignee)

Comment 16

6 years ago
(In reply to Brad Lassey [:blassey] from comment #15)
> So what exactly is this bug trying to fix?

I'm really confused on the bug now.

1. The GeckoAsyncTask is always going to be run on GeckoBackgroundThread (as per comment 7). Hence, we don't need the second argument in the constructor.
2. GeckoAsyncTask should run onPostExecute() on UI thread. This is fair enough. And we pass an argument (defaulted to GeckoApp.mAppContext everywhere) as:

new GeckoAsyncTask(GeckoApp.mAppContext).

How different it is from:

GeckoApp.mAppContext.post(new Runnable() { void run() { new GeckoAsyncTask() } });

In the longer run, this can actually kill the anonymous GeckoAsyncTasks with a meaningful name. And, when we think of "post" ing the construction of GeckoAsyncTask to the UI thread instead of pass the mAppContext as an argument -- we are actually defaulting to Android's AsyncTask (but for the GeckoBackgroundThread part).

So, the question is:
"Could we ensure we instantiate AsyncTask in UI thread and kill the need for a GeckoAsyncTask?"
(In reply to Sriram Ramasubramanian [:sriram] from comment #16)
> (In reply to Brad Lassey [:blassey] from comment #15)
> > So what exactly is this bug trying to fix?
> 
> I'm really confused on the bug now.
> 
> 1. The GeckoAsyncTask is always going to be run on GeckoBackgroundThread (as
> per comment 7). Hence, we don't need the second argument in the constructor.

Correct.

> 2. GeckoAsyncTask should run onPostExecute() on UI thread. This is fair
> enough. And we pass an argument (defaulted to GeckoApp.mAppContext
> everywhere) as:
> 
> new GeckoAsyncTask(GeckoApp.mAppContext).
> 
> How different it is from:
> 
> GeckoApp.mAppContext.post(new Runnable() { void run() { new GeckoAsyncTask()
> } });

The difference is that the second one is much more convoluted, and people were just doing "new GeckoAsyncTask()" instead, which is wrong. If you can find a clean way to do your longer version then that's great.

> 
> In the longer run, this can actually kill the anonymous GeckoAsyncTasks with
> a meaningful name.

How?

> And, when we think of "post" ing the construction of
> GeckoAsyncTask to the UI thread instead of pass the mAppContext as an
> argument -- we are actually defaulting to Android's AsyncTask (but for the
> GeckoBackgroundThread part).
> 

Agreed.

> So, the question is:
> "Could we ensure we instantiate AsyncTask in UI thread and kill the need for
> a GeckoAsyncTask?"

If you can ensure that you instantiate AsyncTask on the UI thread, then yes, you can kill the need for the GeckoAsyncTask. However you haven't shown how to do that yet.
(Assignee)

Comment 18

6 years ago
This does the instantiation in the UI thread. (Which sadly no one does).

GeckoApp.mAppContext.post(new Runnable() { void run() { new GeckoAsyncTask() } });

I can stick to this and ensure I create AsyncTask in the UI thread. But I can't ensure that everyone does that. (And I hate passing an argument to GeckoAsyncTask).

-----
Better example:
https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/AboutHomeContent.java.in#l603

When the hide() (few lines above it) can be posted to the UI thread (in a convoluted way), the async task can also be, as:

post(new Runnable() {
    void run() {
        TabsAccessor.getTabs(getContext(), NUMBER_OF_REMOTE_TABS, AboutHomeContent.this);
    }
});
(In reply to Sriram Ramasubramanian [:sriram] from comment #18)
> This does the instantiation in the UI thread. (Which sadly no one does).
> 
> GeckoApp.mAppContext.post(new Runnable() { void run() { new GeckoAsyncTask()
> } });
> 
> I can stick to this and ensure I create AsyncTask in the UI thread. But I
> can't ensure that everyone does that. (And I hate passing an argument to
> GeckoAsyncTask).

Posting a runnable to the UI thread adds extra overhead.

But if people want that style, we would not need the GeckoAsyncTask class. And the "post runnable to UI thread just to fork an AsyncTask" boilerplate could be extracted into a utility method.
No, I specifically don't want that style or it's associated overhead.
When this bug was filed, were we trying to fix a specific bug? Or is this a refactor bug?
The general consensus is that we don't want this patch, so closing as WONTFIX.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.