When Fennec is running in the background (e.g. after receiving a GCM message), we still need support for openWindow(), which will launch the Fennec activity and open a tab.

Just curious, does that indicate a push notification can cause a pop-up?

(In reply to Samael Wang [:freesamael][:sawang] from comment #1) > Just curious, does that indicate a push notification can cause a pop-up? Yes, but in Firefox, openWindow is permitted only when the user clicks a notification.

(In reply to Jim Chen [:jchen] [:darchons] from comment #2) > Yes, but in Firefox, openWindow is permitted only when the user clicks a > notification. Sounds reasonable. Thank you!

Catalin, are you able/interested to take this?

Yes, though I'm not very familiar with android.

(In reply to Cătălin Badea (:catalinb) from comment #5) > Yes, though I'm not very familiar with android. Thanks! If you have questions, I'm sure snorp or Margaret can help.

After debugging this a bit, it turns out clicking a notification while fennec is not running will not launch the associated service worker.

Any update on this?

Very rough patch, but I'm uploading it since I won't be able to work on this until next week and someone else might take it. The problem with openWindow on android is that when gecko is running as a result of push notification click there's no browser window active to use for a new window (which is what the SW code is trying to use). To fix this, I'm making a call from gecko to java to launch the main activity and then wait for the browser window to be active using the observer service. There are two cases that need to be addressed: 1. fennec is running, but in the background: here we need to just foreground the application 2. fennec is not running: launch the main activity and wait until we can open the window.

Cătălin, any update? Are you still working on it? Andreas, do you have this now?

I have this, and I've started working on it, but basically the patch from Cătălin is the way to go. Unfortunately it crashes, and I haven't been able to sort that out yet. I'm glad to hand this over to someone else if you want?

OK. Dylan, please take this one and see if you can get the patch into a landable state.

In this patch, if there is no browser window, we launch Fennec, wait for the "BrowserChrome:Ready" message, and try to open the window once again.

Comment on attachment 8799051 [details] [diff] [review] Launch Fennec if necessary before opening a window Review of attachment 8799051 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for working on this! The main issue is that ServiceWorkerClients shouldn't be used on the main thread. Also, there's a second case that should be handled, with Fennec running in the background - openWindow will successfully open the tab, but the app will still be hidden in the background. Could you please also add a test for this? ::: dom/workers/ServiceWorkerClients.cpp @@ +474,5 @@ > { > RefPtr<PromiseWorkerProxy> mPromiseProxy; > nsString mUrl; > nsString mScope; > + ServiceWorkerClients* mServiceWorkerClients; ServiceWorkerClients is ref-counted on the worker thread and shouldn't be passed to the main thread. You should use ServiceWorkerPrivate on the main thread instead. My preference would be to make the runnable an Observer and just do ServiceWorkerPrivate->StoreISupports() to add-ref it while it waits for "Browser:Ready", but it's up to you. @@ +566,5 @@ > + java::GeckoAppShell::OpenWindowForNotification(); > + mServiceWorkerClients->AddPendingWindow(this); > + return NS_OK; > + } > +#endif Please also handle the situation where Fennec is running in the background. In this case, the openWindow operation will succeed but Fennec would continue to stay in the background. @@ +809,5 @@ > + pendingWindows.AppendElement(aPendingWindow); > +} > + > +nsresult > +ServiceWorkerClients::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData) nit coding style: nsISupports* aSubject, const char* aTopic etc.

Alright, I think this addresses everything you brought up aside from your request for a test. I talked to snorp a bit about this and it looks like trying to figure out how to do a good job of testing push notifications on Android might be quite difficult and probably outside the scope of this bug.

Comment on attachment 8804459 [details] [diff] [review] Launch Fennec if necessary before opening a window v2 Review of attachment 8804459 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks! ::: dom/workers/ServiceWorkerPrivate.cpp @@ +2019,5 @@ > + pendingWindows.AppendElement(aPendingWindow); > +} > + > +nsresult > +ServiceWorkerPrivate::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *aData) coding style: nsISupports* aSubject, const char* aTopic, const char16_t* aData

Pushed by droeh@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0349a81229e7 Make openWindow() launch Fennec if it isn't already running. r=catalinb

I had to back this out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=38314000&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/378639d730e476941e68d69ee7157a253b54879c

Ugh, sorry about that. I just need to wrap the AndroidBridge.h include in #ifdef MOZ_WIDGET_ANDROID.

Pushed by droeh@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/582e5f96a727 Make openWindow() launch Fennec if it isn't already running. r=catalinb

Backout by philringnalda@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1145a10a2fd5 Backed out changeset 582e5f96a727 for Linux serviceworker bustage

e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=38333127&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=38333189&repo=mozilla-inbound

Oh, and not just Linux, I just hadn't looked again since we finally got around to running some tests on Mac and Windows, both of which were also affected.

Alright, this doesn't seem like it should affect anything other than Android, so that's strange. I'll look into it.

So it looks like the test failures were related to cycle collection, which is not an area I'm super familiar with. I was able to fix it (verified on a local OSX build and on try) by changing NS_INTERFACE_MAP_ENTRY(nsIObserver) with NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver) in ServiceWorkerPrivate.cpp, but as I said I'm not too familiar with this and am not 100% sure this is the correct way to go. Catalin, do you have any thoughts about this? (The rest of the patch is identical to the previous version.)

(In reply to Dylan Roeh (:droeh) from comment #25) > Created attachment 8805678 [details] [diff] [review] > Launch Fennec if necessary before opening a window v2.1 > > So it looks like the test failures were related to cycle collection, which > is not an area I'm super familiar with. I was able to fix it (verified on a > local OSX build and on try) by changing NS_INTERFACE_MAP_ENTRY(nsIObserver) > with NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver) in > ServiceWorkerPrivate.cpp, but as I said I'm not too familiar with this and > am not 100% sure this is the correct way to go. Catalin, do you have any > thoughts about this? (The rest of the patch is identical to the previous > version.) It doesn't look like ServiceWorkerPrivate leaked, just that the QI to nsISupports failed. test_service_worker_liftime.html is the only test which uses the QI to nsISupports, when dispatching a test only event for service worker shutdown.

Pushed by droeh@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5abd7301134e Make openWindow() launch Fennec if it isn't already running. r=catalinb