Closed Bug 1260499 Opened 8 years ago Closed 8 years ago

Handle incoming messages before push service is loaded

Categories

(Core :: DOM: Notifications, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jchen, Assigned: wchen)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 2 obsolete files)

IRC context,

> jchen: kitcambridge: nalexander: i'm looking at receiving push msg when gecko is not running. in that case we need to start gecko, load PushService.jsm, and deliver the msg to it. afaict PushService loads/initializes asynchronously, and I don't know what's the best way to wait for PushService to finish initializing before delivering the msg to it?
> nalexander: jchen: I imagine PushService is not designed to handle this.  I wonder if we could expose the startup promise or similar, so you can hang stuff off of it.
> nalexander: jchen: we could also add Fennec-only startup code in PushServiceAndroidGCM to let you write notifications earlier than startup complete.
> jchen: right
> kitcambridge: right, exposing the startup promise makes sense. or we could have nsIPushService/PushService queue messages, then deliver when ready
> ...
> nalexander: kitcambridge: jchen: the startup promise model seems wrong because there's some state machine in PushService, i.e. you might be able to startup multiple times depending on network conditions, etc.
> kitcambridge: nalexander: the Sync use case was slightly different. we wanted to delay initialization of the receiver (fxa) until 10 seconds in, or it had an incoming message.
> nalexander: kitcambridge: ok.  I'm not sure there's an example trying to get a handle to the PushService early in the tree.
> kitcambridge: nalexander: I don't believe there is. and it's wrapped by nsIPushService, so initialization is opaque to the caller :-/
> nalexander: kitcambridge: this must be a well solved xpcom problem.
> nalexander: kitcambridge: but I'm not aware of the solution.  I guess you must be able to get a handle to the service, regardless of its state.  Suggesting a queue of inbound notifications to deliver.
> kitcambridge: nalexander: indeed, that seems cleanest. get a handle to nsIPushService; queueing a message kicks off PushService.init()
> kitcambridge: nalexander: xpcom services are lazily created, afaik, so init will kick off on the first do_GetService("@mozilla.org/push/Service") call
> nalexander: kitcambridge: aye, so you can always get a handle, we just might not have inited and be able to accept inocming messages yet.  Suggests the service must be the one to do the queuing.
> kitcambridge: nalexander: yup
> nalexander: kitcambridge: which strongly suggests this should be at the PushService.jsm level, as a general concern; rather than at the PushServiceAndroidGCM level, as a specific-to-Fennec concern.
> nalexander: kitcambridge: i.e., Desktop is mildly likely to grow additional sources of incoming notifications, and will need to be able to accommodate them.
> kitcambridge: nalexander: indeed, we are in agreement. :-) I think it belongs here: https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/push/nsIPushService.idl

Looks like the way to go is to have PushService.jsm queue up incoming messages from nsIPushService until initialization is complete.
Thinking about this a bit more on the train...I think nsIPushService could grow a method like `injectMessage`, that receives the encrypted payload and crypto headers from Android. We'd queue the message until PushService.jsm finished initializing, then call `PushService.receivedPushMessage` to decrypt the message and notify the service worker.

Nick pointed out in the chat that Desktop could use a similar notification mechanism, so we should probably decide how to pass the injected message (jsval, JSON string, separate params for payload and headers, and so on).
Hi Shawn,
Could you please take a look? Thank you!
Flags: needinfo?(shuang)
Assignee: nobody → shuang
Flags: needinfo?(shuang)
Whiteboard: btpp-active
:shawnjohnjr - any updates on progress with this bug? we'd like to land this for the current dev cycle.  Please let us know if you have time for this otherwise we can work to get it re-assigned.  Thanks.
Flags: needinfo?(shuang)
Assignee: shuang → nobody
Flags: needinfo?(shuang)
Whiteboard: btpp-active
:wchen are you investigating this?
Flags: needinfo?(wchen)
I can take this.
Assignee: nobody → wchen
Flags: needinfo?(wchen)
It isn't clear to me what an injectMessage API should look like. Messages right now are tied to a subscription which is somewhat tied to the backend and it does not seem like a good idea to have an API with backend dependent parameters. Also, having a way to inject messages that don't use the backend opens up the possibility for state to get out of sync such as the version number.

This patch sends messages to Fennec when the message observer is added/removed and does the queuing in Fennec.
Attachment #8739538 - Flags: review?(nalexander)
Comment on attachment 8739538 [details] [diff] [review]
Handle incoming messages before push service is initialized.

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

Neat approach.  In principle I'm fine with this, but your queue is not sufficiently durable.  That is, the Java service might wake up to service a message, queue it in memory, and then the Fennec process could be killed before we have a chance to start Gecko and drain the message queue.

I guess this window is short, and difficult to distinguish from "we crashed while servicing the queue".  Did you consider this window?  If you did, document these shortcomings in class comments please.

If you wanted to make this more durable, you'd write the pending push messages into the on-disk state upon receipt.

Finally, let's have jchen review as well.  Could you flag him as well next iteration?  Thanks!

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java
@@ +77,5 @@
>      }
>  
>      protected final PushManager pushManager;
>  
> +    private final List<JSONObject> pendingPushMessages;

Comment on how `pendingPushMessages` is synchronized.

@@ +79,5 @@
>      protected final PushManager pushManager;
>  
> +    private final List<JSONObject> pendingPushMessages;
> +
> +    private boolean geckoServiceInitialized;

nit: This is poorly named.  Gecko, Service, Initialized -- all these words have many meanings in this App.  How about...

`canSendPushMessagesToGecko`?

Might as well be descriptive.
Attachment #8739538 - Flags: review?(nalexander) → feedback+
Whiteboard: btpp-active
Addressed comments. For persisting the queue I've hooked into the activity lifecycle callbacks to save/restore the queue to/from the instance state bundle.
Attachment #8739538 - Attachment is obsolete: true
Attachment #8740154 - Flags: review?(nchen)
Attachment #8740154 - Flags: review?(nalexander)
Comment on attachment 8740154 [details] [diff] [review]
Handle incoming messages before push service is initialized. v2

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

This isn't sensible.  There's no reason to expect `GeckoApp` to have run in any particular sequence with respect to any of the GCM listener services.  That is, after the device boots, a push message could come from GCM before any other interaction with Gecko.

Conflating the Activity (or even Application!) lifecycle with the GCM/PushService lifecycle is not a viable approach.

I expected you to explain that yes, you had considered the window and to argue that it was small and not worth paying the overhead of disk access, etc.  I'm surprised you didn't, which suggests that you don't think the window is small and therefore must be addressed.  I am not aware of any way to have the system maintain your state across process re-starts, save manually managing it on disk or tucking it into a pending intent/alarm/etc.

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java
@@ +295,5 @@
>                  }
>                  return;
>              }
> +            if ("PushServiceAndroidGCM:Initialized".equals(event)) {
> +                // Send all pending messages to gecko and set the

nit: capital Gecko.
Attachment #8740154 - Flags: review?(nalexander) → review-
When I was working on this I thought that the PushService stuff was running in GeckoApp, under that assumption it made sense to use the provided mechanisms to preserve state. It's not that I think the window for losing messages large, but if there is already a mechanism to prevent it, it would be better to use it.

Since there isn't an equivalent mechanism to preserve state for the service, I don't think it's worth it to manage persisting the pending message queue ourselves and having to deal with any format or schema changes that may come up.

I've also removed synchronization since it looks like PushService class is background thread only and isn't thread-safe anyways (unless I'm mistaken, which may be the case since getInstance and onCreate are synchronized for some reason).
Attachment #8740154 - Attachment is obsolete: true
Attachment #8740154 - Flags: review?(nchen)
Attachment #8740539 - Flags: review?(nchen)
Attachment #8740539 - Flags: review?(nalexander)
Comment on attachment 8740539 [details] [diff] [review]
Handle incoming messages before push service is initialized. v3

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

Looks good to me!  If jchen is happy, ship it.

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java
@@ +278,5 @@
> +                // all new push messages are sent directly to Gecko
> +                // instead of being queued.
> +                canSendPushMessagesToGecko = true;
> +                for (String pushMessage : pendingPushMessages) {
> +                    sendMessageToGeckoService(pushMessage);

Consider being conservative and catching exceptions here, to not drop messages in rare circumstances.
Attachment #8740539 - Flags: review?(nalexander) → review+
Comment on attachment 8740539 [details] [diff] [review]
Handle incoming messages before push service is initialized. v3

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

Works for me. I'll get the rest sorted out in bug 1252650.
Attachment #8740539 - Flags: review?(nchen) → review+
Comment on attachment 8740539 [details] [diff] [review]
Handle incoming messages before push service is initialized. v3

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

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java
@@ +276,5 @@
> +                // Send all pending messages to Gecko and set the
> +                // canSendPushMessageToGecko flag to true so that
> +                // all new push messages are sent directly to Gecko
> +                // instead of being queued.
> +                canSendPushMessagesToGecko = true;

This has a potential race condition BTW. The event listener runs on the Gecko thread, but the flag is checked on the UI thread, so you need to synchronize between the two.
Callers are posted to the background thread so there should be no race.
I just noticed that I was working in a different local repository than I thought and ended up with the wrong patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0866e929d9e15b00fe85164cac0c09f48c7aba1e
https://hg.mozilla.org/mozilla-central/rev/cd77e25dd600
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.