Closed Bug 1252650 Opened 8 years ago Closed 8 years ago

Start Gecko when a GCM-based Push message is received and Gecko is not already running

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: nalexander, Assigned: jchen)

References

Details

Attachments

(8 files, 6 obsolete files)

2.62 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
7.86 KB, patch
snorp
: review+
Details | Diff | Splinter Review
3.22 KB, patch
snorp
: review+
Details | Diff | Splinter Review
3.66 KB, patch
jchen
: review+
Details | Diff | Splinter Review
2.44 KB, patch
jchen
: review+
Details | Diff | Splinter Review
7.51 KB, patch
jchen
: review+
Details | Diff | Splinter Review
1.83 KB, patch
jchen
: review+
Details | Diff | Splinter Review
7.74 KB, patch
Details | Diff | Splinter Review
The initial implementation of WebPush in Fennec will drop incoming push notifications on the floor if Gecko is not already running.  This ticket tracks doing better: starting Gecko, delivering the message to the appropriate service worker, and moving on.  There may be additional work to let the service worker upgrade the background Gecko to a full UI process.
Assignee: nobody → nchen
I surely don't want a remote service cause an app on my cellphone or tablet to start! This would make my cellphone effectively a server, with all the security, "DoS" etc. implications that server entails. 

(Esp. given that launching Gecko isn't a small quick thing, and cellphones have limited RAM, and I might be doing something else or even more likely my cellphone is in standby, a DoS is fairly obvious.)

Push notifications should be stored and processed when the user launches Firefox. This is a GUI application, and the user should be in control. E.g. on Android, you might be able to play out a notification in the notification bar, using native Java code, and only when I click on the notification, Gecko launches. That already is a stretch IMHO.
(In reply to Ben Bucksch (:BenB) from comment #1)
> I surely don't want a remote service cause an app on my cellphone or tablet
> to start! This would make my cellphone effectively a server, with all the
> security, "DoS" etc. implications that server entails. 

That is exactly what push notifications are for, and that is exactly how GCM works. An app service is started and invoked to handle a remote intent. Every single push notification on your Android device works this way today. Literally the only special thing about webpush is that we need to run a JS serviceworker environment to handle the intent.

 
> Push notifications should be stored and processed when the user launches
> Firefox. This is a GUI application, and the user should be in control. E.g.
> on Android, you might be able to play out a notification in the notification
> bar, using native Java code, and only when I click on the notification,
> Gecko launches. That already is a stretch IMHO.

Just because Gecko is started to handle the notification doesn't mean that the Firefox browser launches and comes to the foreground. Replace "Gecko" in the title of this bug with "JVM" or "Ruby interpreter" or the environment of your choice" and see if you still don't like it.

If you still don't, then you are probably not the target audience for this feature. Push notifications are opt-in on a per-site basis, so if you don't want pushes from a site to run, just don't allow them.
> > This would make my cellphone effectively a server, with all the
> > security, "DoS" etc. implications that server entails. 

> That is exactly what push notifications are for

That doesn't make it any better. We learned *by experience* to disable all server in default Linux installations. It took us 10 years to come to the realization that opening attack surface is a bad idea, and the attempt to fix the bugs at the root was futile.

The last thing we need is servers in our pocket.

You'll need a better story than "this is what it's designed it for". The same was true for those servers. It was still a bad idea.

> Just because Gecko is started to handle the notification doesn't mean that the Firefox browser
> launches and comes to the foreground.

I wasn't talking about a GUI. I was saying that the Gecko process takes several seconds and a few dozen MB RAM just to start and that's already a problem right there.
Push notification service does not require applications to listen on socket (if that's when you mean(In reply to Ben Bucksch (:BenB) from comment #3)
> > > This would make my cellphone effectively a server, with all the
> > > security, "DoS" etc. implications that server entails. 
> 
> > That is exactly what push notifications are for
> 
> That doesn't make it any better. We learned *by experience* to disable all
> server in default Linux installations. It took us 10 years to come to the
> realization that opening attack surface is a bad idea, and the attempt to
> fix the bugs at the root was futile.
> 
> The last thing we need is servers in our pocket.
> 
> You'll need a better story than "this is what it's designed it for". The
> same was true for those servers. It was still a bad idea.

Push notification services do not require applications to listen on socket (i.e. being servers). 

The way GCM on Android, or APNS on iOS, or MPNS on Windows work is that the device maintains a persistent TCP connection to push notification server by periodically sending "heartbeats" to the server. GCM, for instance, sends heartbeat every 15 mins on WiFi, or 28 mins on cellular network.

No matter gecko does it work not, GCM is always running in the background on most Android devices, and all GCM integrated apps share the single persistent TCP connection. If gecko is going adapt GCM, nothing changes on the device from the networking perspective. 

In addition, GCM is designed to awake integrated application process when notification comes in in the first place, through broadcast receivers.
I understand how GCM works. The fact remains that a remote service can invoke functions on my cellphone, from afar. That effectively means it's a server. That brings exposure and attack surface.

> In addition, GCM is designed to awake integrated application process when notification comes
> in in the first place, through broadcast receivers.

Right, I understand that. What I'm saying is that the most that Fennec should do is:
1. accept the GCM notification using native Java code, *not* the Gecko process.
2. play out a native Android notification in the notification bar, and wait for the user to click on it.
3. only after the user clicks on it, start the Gecko process
4. individually handle the remote message.
Any custom handling per message would happen in step 4, not before.
(In reply to Nick Alexander :nalexander from comment #0)
> The initial implementation of WebPush in Fennec will drop incoming push
> notifications on the floor if Gecko is not already running.  This ticket
> tracks doing better: starting Gecko, delivering the message to the
> appropriate service worker, and moving on.  There may be additional work to
> let the service worker upgrade the background Gecko to a full UI process.

Is there a more detailed spec of what needs to happen? e.g. what kind of message does Gecko receive (an intent?) and what kind of action do we need to take (call a js callback?)
Flags: needinfo?(nalexander)
(In reply to Ben Bucksch (:BenB) from comment #5)
> I understand how GCM works. The fact remains that a remote service can
> invoke functions on my cellphone, from afar. That effectively means it's a
> server. That brings exposure and attack surface.
> 
> > In addition, GCM is designed to awake integrated application process when notification comes
> > in in the first place, through broadcast receivers.
> 
> Right, I understand that. What I'm saying is that the most that Fennec
> should do is:
> 1. accept the GCM notification using native Java code, *not* the Gecko
> process.
> 2. play out a native Android notification in the notification bar, and wait
> for the user to click on it.

Showing a notification is not what this service is for. The Service Worker can take a number of actions based on the receipt of the push message. For instance, refresh an offline cache. If the worker wants to show a notification, however, it can.

> 3. only after the user clicks on it, start the Gecko process
> 4. individually handle the remote message.
> Any custom handling per message would happen in step 4, not before.

That sort of confirmation mechanism just won't work for many use cases. However, the user will be prompted for whether or not they want to allow a given website to use push notifications. In your case you would always answer 'no', or we could consider a global preference so that you aren't nagged all the time.
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> (In reply to Nick Alexander :nalexander from comment #0)
> > The initial implementation of WebPush in Fennec will drop incoming push
> > notifications on the floor if Gecko is not already running.  This ticket
> > tracks doing better: starting Gecko, delivering the message to the
> > appropriate service worker, and moving on.  There may be additional work to
> > let the service worker upgrade the background Gecko to a full UI process.
> 
> Is there a more detailed spec of what needs to happen? e.g. what kind of
> message does Gecko receive (an intent?) and what kind of action do we need
> to take (call a js callback?)

jchen -- sorry for the delayed reply.  Much of this code has landed, but it's not yet prefed on -- I'm pursuing that over in Bug 1252666.  I'm working on finishing that off today and will flag you on the commits you'll need to try this out locally.

However, you can see what needs to happen at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/push/PushService.java#144.  Instead of bailing out, you need to start Gecko and deliver the Push notification to the PushService.jsm, just like at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/push/PushService.java#177.
Depends on: 1252666
Flags: needinfo?(nalexander)
Depends on: 1257319
Depends on: 1258554
Depends on: 1259653
Depends on: 1260243
Depends on: 1260499
Add a data parameter to be passed into any observers implemented by
created services.
notifyCategory loads all services registered under a particular
category, and calls on any observers that the services implement.
Add support for Intents that launch GeckoService, load services
registered under given categories, and notify observers implemented by
those services.
We want to deliver a message even if the push service is not started, My idea is to have the push service registered under the category "android-push" inside dom/push/Push.manifest. That way we can have GeckoService create the push service through the android-push category, if necessary, and then send an observer notification to it that contains the incoming message.
Attachment #8736820 - Flags: review?(nalexander)
Comment on attachment 8736809 [details] [diff] [review]
Add GeckoAppShell.notifyCategory (v1)

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

nit: in the commit message, "calls on any observers" is an odd construction.  I'd rather not say anything about this, since it's just passing through to XPCOM, which is "well understood" and more authoritative about messaging/observers/lifecycle etc.

::: widget/android/nsAppShell.cpp
@@ +308,5 @@
>          obsServ->NotifyObservers(nullptr, aTopic->ToCString().get(),
>                                   aData ? aData->ToString().get() : nullptr);
>      }
> +
> +    static void NotifyCategory(jni::String::Param aCategory,

I find this name confusing -- why not something closer to the underlying "NS_CreateServicesFromCategory"?

I assume that method does the right thing when called multiple times, and when the service is/services are alive, and when Gecko is shutting down, etc?
Comment on attachment 8736810 [details] [diff] [review]
Support Intent to notify categories in GeckoService (v1)

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

I see that GeckoService is not exported by default, but we should make that explicit at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#156.  Are there any security implications of GeckoService I should be aware of?  This is opening a broad hole for things to drive through...

::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java
@@ +25,5 @@
>      private static final boolean DEBUG = false;
>  
>      private static final String INTENT_PROFILE_NAME = "org.mozilla.gecko.intent.PROFILE_NAME";
>      private static final String INTENT_PROFILE_DIR = "org.mozilla.gecko.intent.PROFILE_DIR";
> +    private static final String INTENT_CATEGORY = "org.mozilla.gecko.intent.CATEGORY";

So there's an Android concept of category attached to <intent-filter>.  I think it's well worth being very explicit -- GEECKO_SERVICE_CATEGORY or similar.

@@ +28,5 @@
>      private static final String INTENT_PROFILE_DIR = "org.mozilla.gecko.intent.PROFILE_DIR";
> +    private static final String INTENT_CATEGORY = "org.mozilla.gecko.intent.CATEGORY";
> +
> +    private static final String INTENT_ACTION_UPDATE_ADDONS = "update-addons";
> +    private static final String INTENT_ACTION_NOTIFY_CATEGORY = "notify-category";

Again, "notify-category" is mis-leading.  I'd prefer to avoid the term notifications if possible, since it's so overloaded.  "Push messages" is the best thing I have for incoming GCM messages; and since this is really about starting a Gecko service, why not something like "START_GECKO_SERVICE"?

@@ +176,5 @@
> +
> +            if (category == null) {
> +                break;
> +            }
> +            GeckoAppShell.notifyCategory(category, data);

Nifty.
Attachment #8736810 - Flags: feedback+
Comment on attachment 8736820 [details] [diff] [review]
Use GeckoService to handle incoming push messages (v1)

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

I'd like to see another version of this patch series after we hash out naming, but I think this is some great work and like the approach.

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java
@@ +163,5 @@
>  
>              Log.i(LOG_TAG, "Delivering dom/push message to Gecko!");
> +
> +            final Intent intent = GeckoService.getIntentForNotifyCategory(
> +                    context, "android-push", data.toString());

Nifty.

@@ +166,5 @@
> +            final Intent intent = GeckoService.getIntentForNotifyCategory(
> +                    context, "android-push", data.toString());
> +            GeckoService.setIntentProfile(intent, profileName, profilePath);
> +
> +            context.startService(intent);

nit: no trailing newline.
Attachment #8736820 - Flags: review?(nalexander) → feedback+
(In reply to Nick Alexander :nalexander from comment #13)
> Comment on attachment 8736809 [details] [diff] [review]
> Add GeckoAppShell.notifyCategory (v1)
> 
> Review of attachment 8736809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/nsAppShell.cpp
> @@ +308,5 @@
> >          obsServ->NotifyObservers(nullptr, aTopic->ToCString().get(),
> >                                   aData ? aData->ToString().get() : nullptr);
> >      }
> > +
> > +    static void NotifyCategory(jni::String::Param aCategory,
> 
> I find this name confusing -- why not something closer to the underlying
> "NS_CreateServicesFromCategory"?

The name was to match the "NotifyObservers" function above. I actually found "CreateServicesFromCategory" misleading because the function does two things - 1) create XPCOM services if needed; 2) notify any XPCOM observers that those services implement.

> I assume that method does the right thing when called multiple times, and
> when the service is/services are alive, and when Gecko is shutting down, etc?

Yeah, should be okay.

(In reply to Nick Alexander :nalexander from comment #14)
> Comment on attachment 8736810 [details] [diff] [review]
> Support Intent to notify categories in GeckoService (v1)
> 
> Review of attachment 8736810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see that GeckoService is not exported by default, but we should make that
> explicit at
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> AndroidManifest.xml.in#156.  Are there any security implications of
> GeckoService I should be aware of?  This is opening a broad hole for things
> to drive through...

GeckoService is for handling internal tasks, and shouldn't be exported because we implicitly trust any code that starts the service.

> ::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java
> @@ +25,5 @@
> >      private static final boolean DEBUG = false;
> >  
> >      private static final String INTENT_PROFILE_NAME = "org.mozilla.gecko.intent.PROFILE_NAME";
> >      private static final String INTENT_PROFILE_DIR = "org.mozilla.gecko.intent.PROFILE_DIR";
> > +    private static final String INTENT_CATEGORY = "org.mozilla.gecko.intent.CATEGORY";
> 
> So there's an Android concept of category attached to <intent-filter>.  I
> think it's well worth being very explicit -- GEECKO_SERVICE_CATEGORY or
> similar.

Okay, I think I'll go with INTENT_XPCOM_CATEGORY because category is an XPCOM concept.

> @@ +28,5 @@
> >      private static final String INTENT_PROFILE_DIR = "org.mozilla.gecko.intent.PROFILE_DIR";
> > +    private static final String INTENT_CATEGORY = "org.mozilla.gecko.intent.CATEGORY";
> > +
> > +    private static final String INTENT_ACTION_UPDATE_ADDONS = "update-addons";
> > +    private static final String INTENT_ACTION_NOTIFY_CATEGORY = "notify-category";
> 
> Again, "notify-category" is mis-leading.  I'd prefer to avoid the term
> notifications if possible, since it's so overloaded.  "Push messages" is the
> best thing I have for incoming GCM messages; and since this is really about
> starting a Gecko service, why not something like "START_GECKO_SERVICE"?

The Intent action does more than starting GeckoService though. It also performs certain tasks (e.g. start addons update, send an observer notification, etc.). I think in the context of XPCOM, "notification" is well-known to be referring to observers (nsIObserver), which we are using here.
Attachment #8736808 - Flags: review?(nfroyd)
Attachment #8736808 - Flags: review?(nfroyd) → review+
Changed names to "xpcom category" in several places for clarification.
Attachment #8738629 - Flags: review?(snorp)
Attachment #8736810 - Attachment is obsolete: true
Attachment #8736809 - Flags: review?(snorp)
Comment on attachment 8738629 [details] [diff] [review]
Support Intent to notify categories in GeckoService (v2)

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

I think we should just have a special push intent instead of exposing xpcom category notifications. If you want to send a category notification when you receive this, that's fine, I just don't think it needs to be exposed in the Intent.
Attachment #8738629 - Flags: review?(snorp) → review-
Comment on attachment 8736809 [details] [diff] [review]
Add GeckoAppShell.notifyCategory (v1)

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

::: widget/android/nsAppShell.cpp
@@ +308,5 @@
>          obsServ->NotifyObservers(nullptr, aTopic->ToCString().get(),
>                                   aData ? aData->ToString().get() : nullptr);
>      }
> +
> +    static void NotifyCategory(jni::String::Param aCategory,

I agree, something closer to CreateServicesFromCategory might make more sense.
Attachment #8736809 - Flags: review?(snorp) → review+
Normally we delay-load PushService on session restore. However, we won't
have session restore when Gecko is running in background without GUI. So
we need a way to load PushService immediately.
Attachment #8740860 - Flags: review?(kcambridge)
Attachment #8736809 - Attachment is obsolete: true
Attachment #8736820 - Attachment is obsolete: true
Attachment #8738629 - Attachment is obsolete: true
Add support for Intents that launch GeckoService and create services
based on a category registered with the category manager.
Attachment #8740861 - Flags: review?(snorp)
canUseProfile returns whether we're currently using the given profile or
we can potentially start Gecko with the given profile.
Attachment #8740862 - Flags: review?(snorp)
(In reply to Jim Chen [:jchen] [:darchons] from comment #21)
> Created attachment 8740861 [details] [diff] [review]
> Add intent to create XPCOM services in GeckoService (v1)
> 
> Add support for Intents that launch GeckoService and create services
> based on a category registered with the category manager.

Shouldn't the ability to passing some extra data with the category name while creating services be kept? Is there a better plan to handle cases that wake some service worker then do something?
Flags: needinfo?(nchen)
Comment on attachment 8740860 [details] [diff] [review]
Support loading PushService immediately on Android (v1)

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

Thanks! r+ with the ifdef removed.

::: dom/push/Push.manifest
@@ +8,5 @@
>  category app-startup PushServiceParent @mozilla.org/push/Service;1
> +
> +#ifdef MOZ_WIDGET_ANDROID
> +# For immediate loading of PushService instead of delayed loading.
> +category android-push-service PushServiceParent @mozilla.org/push/Service;1

I think it's OK to remove the ifdef. This category isn't used on Desktop, anyway; and the comment seems descriptive enough. Maybe mentioning that this is Android-specific would be good, although "android-push-service" implies that already.

::: dom/push/PushComponents.js
@@ +82,5 @@
>        Services.obs.removeObserver(this, "sessionstore-windows-restored");
>        this._handleReady();
>        return;
>      }
> +#ifdef MOZ_WIDGET_ANDROID

As above, please remove the ifdef, or use AppConstants.jsm.
Attachment #8740860 - Flags: review?(kcambridge) → review+
(In reply to SUN Haitao from comment #23)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #21)
> > Created attachment 8740861 [details] [diff] [review]
> > Add intent to create XPCOM services in GeckoService (v1)
> > 
> > Add support for Intents that launch GeckoService and create services
> > based on a category registered with the category manager.
> 
> Shouldn't the ability to passing some extra data with the category name
> while creating services be kept? Is there a better plan to handle cases that
> wake some service worker then do something?

We could. We don't need that anymore for this bug because of the patch in bug 1260499. But we can add back an extra data parameter in a future bug if needed.
Flags: needinfo?(nchen)
Attachment #8740861 - Flags: review?(snorp) → review+
Attachment #8740862 - Flags: review?(snorp) → review+
Blocks: 1264815
Shouldn't GeckoService.setIntentProfile be merged into GeckoService.getIntentForAction (or marked as private)? It seems not quite useful for other classes.
Here's the new patch to launch Gecko using the correct profile and load push
service, if necessary.
Attachment #8742478 - Flags: review?(nalexander)
Comment on attachment 8742478 [details] [diff] [review]
Use GeckoService to launch Gecko for incoming push messages (v2)

Looks good to me.  (I didn't think too thoroughly about this.)
Attachment #8742478 - Flags: review?(nalexander) → review+
Enable sending event response when Gecko state is PROFILE_READY. This
happens when Gecko is loaded in the background and GeckoApp is not
active. This is safe because it's only a response to an event from
Gecko, so there is definitely a listener for the response on the Gecko
side already. r=me for a trivial change.
Attachment #8743338 - Flags: review+
Attachment #8742478 - Attachment is obsolete: true
Attachment #8740860 - Attachment is obsolete: true
Assignee: nchen → sunhaitao
(In reply to SUN Haitao from comment #39)
> Created attachment 8752571 [details] [diff] [review]
> Add a component to handle 'persisent-notification-click'

Can you work on this in another bug? This bug has already been closed.
Assignee: sunhaitao → nchen
Flags: needinfo?(sunhaitao)
Sorry, this is an accident while I try using bzexport.
Flags: needinfo?(sunhaitao)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.