Closed
Bug 1260499
Opened 8 years ago
Closed 8 years ago
Handle incoming messages before push service is loaded
Categories
(Core :: DOM: Notifications, defect)
Core
DOM: Notifications
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jchen, Assigned: wchen)
References
Details
(Whiteboard: btpp-active)
Attachments
(1 file, 2 obsolete files)
8.55 KB,
patch
|
nalexander
:
review+
jchen
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•8 years ago
|
||
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).
Assignee: nobody → shuang
Flags: needinfo?(shuang)
Updated•8 years ago
|
Whiteboard: btpp-active
Comment 3•8 years ago
|
||
: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)
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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+
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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-
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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+
Reporter | ||
Comment 12•8 years ago
|
||
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+
Reporter | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
Callers are posted to the background thread so there should be no race.
Assignee | ||
Comment 16•8 years ago
|
||
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
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd77e25dd600
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•