Closed Bug 1311445 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: droeh, Assigned: droeh)

References

Details

Attachments

(1 file)

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
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?
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.
Attachment #8803346 - Flags: review?(ehsan)
Comment on attachment 8803346 [details] [diff] [review]
Update NotificationsDB.jsm for fallout from 1261019

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

Thanks!
Attachment #8803346 - Flags: review?(ehsan) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c391315cc769
Update NotificationDB.filterNonAppNotifications to no longer use NotificationStorage.canPut. r=ehsan
https://hg.mozilla.org/mozilla-central/rev/c391315cc769
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1261019
You need to log in before you can comment on or make changes to this bug.