1311445 - Push notifications do not respond to being clicked after Fennec is killed

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Dylan Roeh (:droeh) (he/him)]

Steps to reproduce:
* Launch Fennec, go to [0], tap "Send notification"
* When the notification arrives, click it
* Kill Fennec (eg by swiping in the app switcher)
* Launch Fennec again, go to [0] again, tap "Send notification" again
* Before the notification arrives, navigate to another site
* Click the notification

Expected behavior: Fennec opens a new tab at [0]
Actual behavior: Nothing happens

Ehsan, I'm cc'ing you because I'm pretty sure this is caused by your patches for bug 1261019. I got a (somewhat wide) regression range from mozregression[1] and your patches seemed the most likely because you touched NotificationStorage.js[2], but I don't think the changes to that file are actually the culprit (at least as far as I can tell). I backed out your patches on a local build and can confirm that doing so puts push notifications back in a working state on Android. I'm continuing to investigate this, but if you have any ideas/recommendations please let me know.

[0]: https://serviceworke.rs/push-clients_demo.html
[1]: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b62b2d88fd0320fc05e32952c0ffd411ea738011&tochange=8002cb8086cd19892c926d7d511db954bde8653b
[2]: https://hg.mozilla.org/mozilla-central/diff/144625d6fafa/dom/notification/NotificationStorage.js

Comment 1

[(no longer active)]

Hmm.  I looked at the change in [2], but I really can't imagine a scenario where that would cause a regression, since getAppByManifestURL() should have been returning null before my patch.

Since my patch is enormous, unfortunately drilling into it is difficult.  I have a few ideas that may help:

* Try turning on debugging logs from NotificationStore.js by setting the DEBUG variable there to true and see if you see any interesting logs?
* You can also try turning out logging from both other files in the notifications directory and also the ones in dom/apps.
* Try turning on these debug logs on a build before my patches to at least find some code that's involved in this, and then look to see how the patch changed that code.
* Try asking someone familiar with the notifications backend if they can give you good guesses on what underlying code can be broken.
* There are a few places in the patch where I switched some APIs to throw NS_ERROR_NOT_IMPLEMENTED, assuming that they won't get called.  Maybe add some debug logs in those places to see if one of them is throwing an exception.

Also, is there anything logged to stderr/adb log?

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Attachment: Update NotificationsDB.jsm for fallout from 1261019

Thanks, Ehsan. Once I found what was going on it was a simple enough fix, NotificationDB was still calling NotificationStorage.canPut at one point; removing that fixed the problem. Let me know if you see any issues with the way I fixed it.

Comment 3

[(no longer active)]

Comment on attachment 8803346 [details] [diff] [review]
Update NotificationsDB.jsm for fallout from 1261019

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

Thanks!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c391315cc769
Update NotificationDB.filterNonAppNotifications to no longer use NotificationStorage.canPut. r=ehsan

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c391315cc769