1280446 - Fennec crashes when launching as the result of a push message

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cătălin Badea (:catalinb)]

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

Comment 1

[Nick Alexander :nalexander [he/him]]

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.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

48 is probably affected, right?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Create PushService if needed (v1)

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.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/29386385b953
Create PushService if needed; r=snorp

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29386385b953

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 8

[Gerry Chang [:gchang]]

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.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ad38f4e2b4d

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7403ba7e1508

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Follow-up

This is a follow-up to fix another possible crash when using push. r=me for trivial patch.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/749004283a01
Follow-up to fix another possible crash; r=me

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/632484723c99
https://hg.mozilla.org/releases/mozilla-beta/rev/7c9c052d51bb

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/749004283a01

Comment 15

[miralobontiu]

Hi Cătălin,

Is the qe-verify+ flag still valid?

Thank you!

Comment 16

[Sylvestre Ledru [:Sylvestre]]

It isn't
(as a general rule, if the flag is older than 6 months, there is no point in checking it...)

Comment 17

[miralobontiu]

Hi Sylvestre,

Thanks for the tip!

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

Have a good day!