Closed Bug 750358 Opened 9 years ago Closed 9 years ago

AsyncTask onPostExecute sometimes runs on GeckoBackgroundThread instead of main thread

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox15 fixed, blocking-fennec1.0 -, fennec15+)

RESOLVED FIXED
Firefox 16
Tracking Status
firefox15 --- fixed
blocking-fennec1.0 --- -
fennec 15+ ---

People

(Reporter: Margaret, Assigned: bnicholson)

References

Details

Attachments

(2 files, 1 obsolete file)

I'm filing this as a follow-up to bug 746946. We landed a band-aid patch there, but a real solution involves finding out why this is happening, and perhaps using GeckoAsyncTask instead of AsyncTask to ensure onPostExecute always gets run on the main thread. However, to make this switch, we'd need to implement cancel() for GeckoAsyncTask.
blocking-fennec1.0: --- → ?
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → -
Attached patch debug patchSplinter Review
I just ran into this after landing patches in bug 769145. Here's the STR I'm using - they only seem to work on my Galaxy Nexus:

1) Go to Manage Applications > Fennec, and click "Clear data"
2) Open Fennec
3) Quickly tap the AwesomeBar
4) Wait a few seconds
5) If there's no crash, click back, then go to step 3 (if the favicon on about:home has loaded, it's too late; start over from step 1)

This crashes fairly reliably for me with the signature from bug 746946. I followed these STR using the attached patch, and here's the output I see when it crashes:

D/BRN     ( 1056): 1 calling from main thread? true
D/BRN     ( 1056): 2 calling from main thread? false
D/BRN     ( 1056): 3 calling from main thread? false

This seems like a bug in AsyncTask since, according to [1], onPostExecute() should be called on same thread as the AsyncTask (which is the UI thread here). This led me to [2], which validates that it is, in fact, a recognized Android bug. I tried the solution given by romainguy, and it fixes the problem.

[1] http://developer.android.com/reference/android/os/AsyncTask.html
[2] http://code.google.com/p/android/issues/detail?id=20915
Assignee: nobody → bnicholson
Attached patch patch (obsolete) — Splinter Review
Here's the suggested fix. Hopefully this fixes other similar bugs.
Attachment #639933 - Flags: review?(blassey.bugs)
(In reply to Brian Nicholson (:bnicholson) from comment #1)
> I just ran into this after landing patches in bug 769145. Here's the STR I'm
> using - they only seem to work on my Galaxy Nexus:

s/Galaxy Nexus/Galaxy S/

This is the same phone, and very similar steps, to what Aaron used to reproduce bug 769145.
Comment on attachment 639933 [details] [diff] [review]
patch

Shouldn't this be in GeckoApplication instead of GeckoActivity? See:
http://code.google.com/p/android/issues/detail?id=20915#c7
Comment on attachment 639933 [details] [diff] [review]
patch

As Finkle said in comment 4, this should be done in Application::onCreate(). I'd also note that this doesn't guaranty to work around the problem, just makes it much less likely that we'll hit it. The sure fire way to fix this is to stop using AsyncTasks and use GeckoAsyncTasks.
Attachment #639933 - Flags: review?(blassey.bugs) → review-
(In reply to Brad Lassey [:blassey] from comment #5)
> The sure fire way to fix this is to stop using AsyncTasks and use GeckoAsyncTasks.

I think we needed a few more features in GeckoAsyncTask to be able to use it in more places.
Attached patch patch v2Splinter Review
Moved to GeckoApplication
Attachment #639933 - Attachment is obsolete: true
Attachment #639935 - Flags: review?(blassey.bugs)
Attachment #639935 - Flags: review?(blassey.bugs) → review+
Comment on attachment 639935 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: potential to crash using AsyncTask
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none
Attachment #639935 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/da718420b647
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Comment on attachment 639935 [details] [diff] [review]
patch v2

[Triage Comment]
Given the low risk profile and the mobile team's tracking-fennec designation, let's uplift to Aurora 15.
Attachment #639935 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.