1229835 - Web notification doesn't load page if Fennec is closed

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

We don't extract the uri to load when launching (a closed) Fennec from a web notification, so we don't load the correct website included in the notification.

The uri is built here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#1318

And we process it elsewhere here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1924

Instead of just returning null, we should extract the uri, format it correctly to match an intent datastring, and return that. (This will get used by BrowserApp.onNewIntent, which will open the correct page.)
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1988

Hanging this bug off of android-push so it doesn't get lost.

(Bug 849653 can probably also be fixed in a similar manner, by tracing the Intent/datauri that Downloads creates and passing that on correctly.)

Comment 1

[Samuel Mendes]

Hi Liu! 

I'm using the user story from BUG 849653 to debugging.

I've tryed to solve this bug but after while I figured it out I can't do it alone.

I've tryed to build the URI inside http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1988 but I wasn't getting it right. I did get the raw uri from cookie, example below, but I don't know if I'm in the right path. 

{"cookie":"\"\/storage\/emulated\/0\/Download\/googlelogo_tablet_tier1_hp_color_183x64dp(38).pnghttp:\/\/www.google.com.br\/images\/branding\/searchlogo\/1x\/googlelogo_tablet_tier1_hp_color_183x64dp.pngSun Dec 06 2015 10:16:05 GMT-0200 (BRST)\"","id":"{cab5fcdd-7fdc-47e2-9e85-3cd6d4b939f6}","handlerKey":"downloads","eventType":"notification-closed"}

Is there any method which processes that JSONObject so that I can get a valid URI?

Sorry for any dumb questions...

Comment 2

[Chenxia Liu [:liuche]]

Hi Samuel! Sorry for the delay. I started looking through this code more (as well as the downloads code) and you have an excellent point - the cookie as passed in doesn't give you the url (in the downloads or notifications code).

So the problem is that our Java code doesn't handle opening the url through an Intent or whatever - Gecko has a listener that acts on the notification id and handles the callback - that's what the "cookie" is used for. Unfortunately, this listener dies when Fennec is killed, so when we reopen Fennec, there isn't anything listening for that id anymore.

On the one hand, we could try to get this url and pass it up all the way to Java, but that might interfere with Push notifications - eventually, we'll have a Service that handles these kinds of notifications.

I'm going to remove the "next good bug" from this, because it's not as well-scoped as I initially thought.

Nick, do you have any thoughts on this, specifically if opening target urls from notifications is something that our Push notifications will end up taking care of?

Comment 3

[Samuel Mendes]

Hi Liuche!

I'm really in the beginning of Android development and I had some trouble to debugging this situation.

Is there any nice way to debug Fennec starting the app from a Notification?

Is there any reading that I could learn about it?

Sorry to bother you with those questions.

Comment 4

[Nick Alexander :nalexander [he/him]]

> Nick, do you have any thoughts on this, specifically if opening target urls
> from notifications is something that our Push notifications will end up
> taking care of?

I have a few thoughts on this.  First, I don't expect Web Push to impact this.  The tricky part is waking Gecko to run the Service Worker, which then uses the Web Notification API independently.  If Gecko then goes away, we're left with this situation just the same.

Second, we need to be more clever about using an Intent (PendingIntent?) which includes the desired URL.  I think you have this under control in Comment 0.  There's no sense trying to preserve the complete web context, so no possibility to message the page after Gecko restarts.  (I'd be curious to know if Chrome tries harder here.  I doubt it; it's hard to preserve a page's web context across web engine restarts.)
 
Third, there's a small security risk here.  We need to be careful if we do anything different in this context, since there's additional data: a UUID that can possibly trigger a callback across domains.

Comment 5

[SUN Haitao]

Restoring web context seems unnecessary.

According to current version of https://notifications.spec.whatwg.org/#lifetime-and-ui-integrations, notifications created by the `Notification` constructor shouldn't appear in notification center. So there should be no chance for them to be clicked while Fennec is not running. Only notifications created by service workers need to be considered. And they just needs a Intent to wake up a service worker and dispatch an event.

I believe the Intent should be the same one to wake up service workers when a push notification arrives. See also Bug 1191367.

Comment 6

[Nick Alexander :nalexander [he/him]]

liuche: any chance you can pick this up?  It's a sore spot for Push.

Comment 7

[Chenxia Liu [:liuche]]

Sure. I need to finish some other things but what is your ideal timeline/priority on this?

Comment 8

[:Margaret Leibovic]

Tracking 48 for push.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Chenxia Liu [:liuche] from comment #7)
> Sure. I need to finish some other things but what is your ideal
> timeline/priority on this?

liuche: can you pick this up?  We'd like this to make 48 and I haven't been able to get to it.

Comment 10

[Chenxia Liu [:liuche]]

Sure! I can put this on my list of things to do for 48.

Comment 11

[SUN Haitao]

(In reply to Chenxia Liu [:liuche] from comment #10)
> Sure! I can put this on my list of things to do for 48.

If you haven't started yet, would you mind let me try? I believe I have just made a working prototype based on patches of Bug 1252650.

Comment 12

[Chenxia Liu [:liuche]]

Nick, I'm sorry to bother you, but do you think I can grab you for a quick chat about the specifics in comment #4? Or if you can answer my questions in a comment, that's fine too.

Can you explain a little further your suggestion of using a Service Worker with this bug? Basically, Notifications create an alertCookie [1], sort of like an id (this is not a web context cookie), and there's some observer that associates that alertCookie with a url so it can be opened from the notification [2]. There is no url being passed around at all - I thought that was the case in comment #0, but I amended it in comment #2.

Is your suggestion to use a ServiceWorker to start the notification, therefore keeping an observer alive that remembers what url is associated with that alertCookie? I don't really see where these observers are being created, so I can't tell if using a ServiceWorker to start the notification would in fact persist the observer across Gecko restarts.

Or is there an alternative way (instead of using the Notifications API maybe?) to persist an observer across Gecko restarts?

Sun Haitao, thank you for the offer! However, I think you're addressing a different problem. This is in fact a persistent notification that's created by the Notification API not a service worker, so it should be present in the notification center, and can't be handled by just waking a service worker.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/nsAlertsService.cpp#213
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java#907

Comment 13

[SUN Haitao]

(In reply to Chenxia Liu [:liuche] from comment #12)
> Sun Haitao, thank you for the offer! However, I think you're addressing a
> different problem. This is in fact a persistent notification that's created
> by the Notification API not a service worker, so it should be present in the
> notification center, and can't be handled by just waking a service worker.

Wait a minute. According to https://notifications.spec.whatwg.org/#persistent-notification, all notifications created by Notification constructor should be non-persistent. It seems that such notifications should be presented as toasts/snack-bars and never appear in the notification center.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Chenxia Liu [:liuche] from comment #12)
> Nick, I'm sorry to bother you, but do you think I can grab you for a quick
> chat about the specifics in comment #4? Or if you can answer my questions in
> a comment, that's fine too.
> 
> Can you explain a little further your suggestion of using a Service Worker
> with this bug? Basically, Notifications create an alertCookie [1], sort of
> like an id (this is not a web context cookie), and there's some observer
> that associates that alertCookie with a url so it can be opened from the
> notification [2]. There is no url being passed around at all - I thought
> that was the case in comment #0, but I amended it in comment #2.

Ah, I see, no URL -- but the equivalent is happening.  It's my belief that the mapping from cookie to URL is not persisted across process restarts, so:

start Fennec
new Notification()
observe notification in system tray
kill Fennec process
observe notification in system tray remains
tap notification
start Fennec, but not to the correct page, since the cookie doesn't map to the URL

> Is your suggestion to use a ServiceWorker to start the notification,
> therefore keeping an observer alive that remembers what url is associated
> with that alertCookie? I don't really see where these observers are being
> created, so I can't tell if using a ServiceWorker to start the notification
> would in fact persist the observer across Gecko restarts.

No.  We can't inject an SW into this process.

> Or is there an alternative way (instead of using the Notifications API
> maybe?) to persist an observer across Gecko restarts?
> 
> Sun Haitao, thank you for the offer! However, I think you're addressing a
> different problem. This is in fact a persistent notification that's created
> by the Notification API not a service worker, so it should be present in the
> notification center, and can't be handled by just waking a service worker.

If what Sun says is correct, we should re-evaluate this entire code path for Web notifications (including ones opened from an SW).  I'd hope we can show these notifications in the system tray, for some amount of time.  (Possibly until the process dies.)

Comment 15

[SUN Haitao]

In https://notifications.spec.whatwg.org/#api, the spec says:
> A non-persistent notification is represented one Notification objects and can
> be created through Notification's constructor.
> 
> A persistent notification is represented by zero or more Notification objects
> can be created through the showNotification() method. 

I suggest we:
0. separate persistent/non-persistent code paths first. 
1. make persistent notifications work like the spec.
2. make non-persistent notifications appear in the notification center, but just disappear while tapped if Fennec is not running.
3. After patches for step 0-3 landed, make non-persistent notifications disappear automatically while Fennec is killed if necessary.

Comment 16

[Chenxia Liu [:liuche]]

I filed bug 1264797, to remove a notification on Gecko death.

Comment 17

[SUN Haitao]

And I filed Bug 1264815 for step 0 & 1.