Closed Bug 1280446 Opened 3 years ago Closed 3 years ago

Fennec crashes when launching as the result of a push message

Categories

(Firefox for Android :: General, defect)

x86_64
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed
firefox49 --- fixed
fennec 48+ ---
firefox50 --- fixed

People

(Reporter: catalinb, Assigned: jchen)

References

Details

Attachments

(2 files)

When receiving a push message while fennec is not running, android will launch fennec which will immediately crash.

Steps to reproduce:
1. Register for push messages on https://people.mozilla.org/~ewong2/push-notification-test/
2. close fennec
3. send a push message using curl
4. fennec launches and crashes with the following stack trace:
06-16 14:14:26.083  3499  3514 E AndroidRuntime: FATAL EXCEPTION: GeckoBackgroundThread
06-16 14:14:26.083  3499  3514 E AndroidRuntime: Process: org.mozilla.fennec_catalin, PID: 3499
06-16 14:14:26.083  3499  3514 E AndroidRuntime: java.lang.IllegalStateException: PushService not yet created!
06-16 14:14:26.083  3499  3514 E AndroidRuntime:        at org.mozilla.gecko.push.PushService.getInstance(PushService.java:71)
06-16 14:14:26.083  3499  3514 E AndroidRuntime:        at org.mozilla.gecko.gcm.GcmMessageListenerService$1.run(GcmMessageListenerService.java:33)
06-16 14:14:26.083  3499  3514 E AndroidRuntime:        at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:39)

https://pastebin.mozilla.org/8877558
Blocks: android-push
Ah, I think I see the race -- right where I said there wouldn't be a race!

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#188

I think what is happening is that the Application startup and the message listener startup are happening more or less at the same time.  We will need to wait for the PushService to be initialized in the message listener, in some way, shape, or form.
tracking-fennec: --- → ?
48 is probably affected, right?
tracking-fennec: ? → 48+
Assignee: nobody → nchen
Status: NEW → ASSIGNED
If PushService has not been created when getInstance is called, create
the PushService instead of throwing an error. This fixes a possible race
condition between initializing PushService and receiving a push message,
where we can receive a push message first.
Attachment #8766445 - Flags: review?(snorp)
Comment on attachment 8766445 [details] [diff] [review]
Create PushService if needed (v1)

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

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java
@@ +73,5 @@
>          return sInstance;
>      }
>  
>      @ReflectionTarget
>      public static synchronized void onCreate(Context context) {

I guess I never noticed this before, but this method name should probably be changed. It conflicts with the non-static one from Service (or Context, maybe, wherever it comes from). I think 'create', 'createInstance', or 'initialize' would be better choices.
Attachment #8766445 - Flags: review?(snorp) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/29386385b953
Create PushService if needed; r=snorp
https://hg.mozilla.org/mozilla-central/rev/29386385b953
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment on attachment 8766445 [details] [diff] [review]
Create PushService if needed (v1)

Approval Request Comment
[Feature/regressing bug #]: Bug 1272557
[User impact if declined]: Possible crash when receiving push notifications
[Describe test coverage new/current, TreeHerder]: Locally, m-c
[Risks and why]: Small; patch only addresses the crash
[String/UUID change made/needed]: None
Attachment #8766445 - Flags: approval-mozilla-beta?
Attachment #8766445 - Flags: approval-mozilla-aurora?
Comment on attachment 8766445 [details] [diff] [review]
Create PushService if needed (v1)

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

This patch fixes a possible crash when receiving push notifications. Take it in 48 beta 7 and aurora. The fix should be in fennec 48 beta 8.
Attachment #8766445 - Flags: approval-mozilla-beta?
Attachment #8766445 - Flags: approval-mozilla-beta+
Attachment #8766445 - Flags: approval-mozilla-aurora?
Attachment #8766445 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Attached patch Follow-upSplinter Review
This is a follow-up to fix another possible crash when using push. r=me for trivial patch.
Attachment #8769883 - Flags: review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/749004283a01
Follow-up to fix another possible crash; r=me
Hi Cătălin,

Is the qe-verify+ flag still valid?

Thank you!
Flags: needinfo?(catalin.badea392)
It isn't
(as a general rule, if the flag is older than 6 months, there is no point in checking it...)
Flags: needinfo?(catalin.badea392)
Hi Sylvestre,

Thanks for the tip!

Taking into consideration Comment 16, I will remove the qe-verify+ flag.

Have a good day!
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.