1210211 - Relax quotas for visible notifications

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

After the initial push event is received, set a timer for a short period (e.g., 3 seconds; it should be less than the default duration for transient notifications). If there are any notifications visible for that origin when the timer fires, don't reduce the quota.

This addresses the case of receiving a burst of push messages, or where a subsequent message updates an existing one with the same tag. It does mean we'll still dock a worker that receives a "show this notification" push message, followed by a "never mind" (say, because the user responded to the notification on another device).

Martin, did I capture that accurately?

Comment 1

[Lina Butler [:lina]]

(In reply to Kit Cambridge [:kitcambridge] from comment #0)
> After the initial push event is received...

That should be "processed," once the promise passed to `pushEvent.waitUntil()` resolves.

Comment 2

[Martin Thomson [:mt:]]

Yes.  See also new proposed policy here: https://docs.google.com/document/d/1yYUB4nn9Hu6vPHKp_eN_kXOG47gjteEv8QIf_l18q4w/edit#

Comment 3

[Edwin Wong [:edwong]]

Here's a breakdown of interactions with quota:
https://docs.google.com/document/d/1rJLTb0XRRPpn82rDGNuDk0Rf63OoYnhAhC6wO2aRnmo/edit

Comment 4

[Edwin Wong [:edwong]]

NI to track this task

Comment 5

[Andrew Overholt [:overholt]]

We should probably document what we do here.

Comment 6

[William Chen [:wchen]]

Attachment: Part 1: Delay updating push quota and check for visible notifications before applying quota penalty.

Comment 7

[William Chen [:wchen]]

Attachment: Part 2: Notify Push service of visible notifications.

Comment 8

[William Chen [:wchen]]

Attachment: Part 3: Test for push notification quota with web notifications.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8680999 [details] [diff] [review]
Part 2: Notify Push service of visible notifications.

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

::: dom/notification/Notification.cpp
@@ +1189,5 @@
> +      } else {
> +        rv = pushQuotaManager->NotificationForOriginClosed(origin.get());
> +      }
> +
> +      NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to call quota manager?");

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

::: dom/push/PushNotificationService.js
@@ +80,5 @@
> +  // nsIPushQuotaManager methods
> +
> +  notificationForOriginShown: function(origin) {
> +    if (!isParent) {
> +      Services.cpmm.sendAsyncMessage("Push:NotificationForOriginShown", origin);

is this handled in other patches?

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8680998 [details] [diff] [review]
Part 1: Delay updating push quota and check for visible notifications before applying quota penalty.

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

Looks great! Please fix up the conditional in `_notificationForOriginShown`. The other three comments are nits.

::: dom/push/PushService.jsm
@@ +855,5 @@
>            notified = this._notifyApp(record, message);
>          }
> +        // Update quota after 3 seconds, at which point
> +        // we check for visible notifications.
> +        setTimeout(() => this._updateQuota(keyID), 3000);

Nit: Could you move this into a top-level constant or pref, please?

@@ +898,5 @@
> +  _notificationForOriginShown(origin) {
> +    debug("_notificationForOriginShown, origin: " + origin);
> +
> +    var count;
> +    if (!this._visibleNotifications.has(origin)) {

if (this._visibleNotifications.has(origin))

@@ +1108,5 @@
>      }
>  
> +    if (aMessage.name === "Push:NotificationForOriginShown") {
> +      debug("Notification shown from child.");
> +      this._notificationForOriginShown(aMessage.data);

Noting for the future: I think we'll want to use the principal on B2G, or at least include the origin attributes.

@@ +1128,5 @@
>          }
>        }
> +
> +      debug("Clearing notifications from child");
> +      this._visibleNotifications.clear();

I'm not sure about this, but we might need to keep track of the listener on B2G...the architecture is one child process per app, right? We can save that for a follow-up bug, since Push doesn't currently work on B2G, anyway.

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8681000 [details] [diff] [review]
Part 3: Test for push notification quota with web notifications.

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

::: dom/push/test/xpcshell/test_quota_with_notification.js
@@ +62,5 @@
> +  let updateQuotaPromise = new Promise((resolve, reject) => {
> +    let quotaUpdateCount = 0;
> +    PushService._updateQuotaTestCallback = function() {
> +      quotaUpdateCount++;
> +      if (numMessages == 10) {

if (quotaUpdateCount == 10) {

Comment 12

[Edwin Wong [:edwong]]

:wchen, once you land this please request uplift to aurora.

Comment 13

[Edwin Wong [:edwong]]

In speaking with overholt, wchen is out most of this week.  Kit can you try to land this patch for him.

Comment 14

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=222edbb7684e

Comment 15

[Lina Butler [:lina]]

Attachment: 0001-Bug-1210211-Part-1-Delay-updating-push-quota.-r-kitc.patch

Fixed conditional and rebased. Carrying over r+.

Comment 16

[Lina Butler [:lina]]

Attachment: 0002-Bug-1210211-Part-2-Notify-Push-service-of-visible-no.patch

Rebased.

Comment 17

[Lina Butler [:lina]]

Attachment: 0003-Bug-1210211-Part-3-Test-for-push-notification-quota-.patch

Rebased.

Comment 18

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b78d00c9af42cedda773b0c7cb5bf61c0546f3cc
Bug 1210211 - Part 1: Delay updating push quota. r=kitcambridge

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5d16111e809899afa052475788be820d66dccc4
Bug 1210211 - Part 2: Notify Push service of visible notifications. r=baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/204a1746f4213c92d3d0f8ca908e5e604d75b038
Bug 1210211 - Part 3: Test for push notification quota with web notifications. r=kitcambridge

Comment 19

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d83ec2c8e116 for failing to build on b2g, https://treeherder.mozilla.org/logviewer.html#?job_id=17375275&repo=mozilla-inbound

Comment 20

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3dbcb73f56

Comment 21

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7343fdb5d5819bc663deafe6df2034e1d7bae49c
Bug 1210211 - Part 1: Delay updating push quota. r=kitcambridge

https://hg.mozilla.org/integration/mozilla-inbound/rev/e282a99d0321a2ff8c599f88d352568e1dbe7bf2
Bug 1210211 - Part 2: Notify Push service of visible notifications. r=baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/17b69dd8b8ad53f9081e74865056ca6b4de30d20
Bug 1210211 - Part 3: Test for push notification quota with web notifications. r=kitcambridge

Comment 22

[Landry Breuil (:gaston)]

Fwiw, this breaks on OpenBSD:

12:43.58 In file included from /usr/obj/m-c/dom/notification/Unified_cpp_dom_notification0.cpp:11:
12:43.58 /home/othersrc/mozilla-central/dom/notification/Notification.cpp:1196:12: error: use of undeclared identifier 'nsIPushQuotaManager'
12:43.59   nsCOMPtr<nsIPushQuotaManager> pushQuotaManager =
12:43.59            ^
12:43.66 /home/othersrc/mozilla-central/dom/notification/Notification.cpp:1198:8: error: use of undeclared identifier 'pushQuotaManager'
12:43.67   if (!pushQuotaManager) {
12:43.67        ^
12:43.70 /home/othersrc/mozilla-central/dom/notification/Notification.cpp:1209:12: error: use of undeclared identifier 'pushQuotaManager'
12:43.70     return pushQuotaManager->NotificationForOriginShown(origin.get());
12:43.71            ^
12:43.76 /home/othersrc/mozilla-central/dom/notification/Notification.cpp:1211:10: error: use of undeclared identifier 'pushQuotaManager'
12:43.77   return pushQuotaManager->NotificationForOriginClosed(origin.get());

Probably some missing #ifdef/#define.

Comment 23

[Landry Breuil (:gaston)]

Or maybe CLOBBER should be touched since this adds an interface.. will retry after a local clobber

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7343fdb5d581
https://hg.mozilla.org/mozilla-central/rev/e282a99d0321
https://hg.mozilla.org/mozilla-central/rev/17b69dd8b8ad

Comment 25

[Lina Butler [:lina]]

Landry, did clobbering fix your local build?

Comment 26

[Landry Breuil (:gaston)]

Yes - wasnt there a policy to touch CLOBBER when adding interfaces?

Comment 27

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a564724473e95df045bb0e3ee7759f6113d86e4
Bug 1210211 - Touch CLOBBER. r=me

Comment 28

[Lina Butler [:lina]]

Thanks! I didn't know about that policy. :-)

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a564724473e

Comment 30

[Andrew Overholt [:overholt]]

Wait, I thought we decided to request uplift?

Comment 31

[William Chen [:wchen]]

Comment on attachment 8688080 [details] [diff] [review]
0001-Bug-1210211-Part-1-Delay-updating-push-quota.-r-kitc.patch

Requesting uplift for all the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: This bug
[User impact if declined]: Push API quota will be more restrictive for common use cases leading to poor experience.
[Describe test coverage new/current, TreeHerder]: One of the patches adds tests that run in treeherder.
[Risks and why]: Possibly introduce bugs in the push service. Low risk, changes to the push service are relatively minor and have test coverage.
[String/UUID change made/needed]: None

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8688080 [details] [diff] [review]
0001-Bug-1210211-Part-1-Delay-updating-push-quota.-r-kitc.patch

These patches have been in Nightly for ~2 weeks, and push notification is planned for FF44. Let's uplift to Aurora44.

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8688081 [details] [diff] [review]
0002-Bug-1210211-Part-2-Notify-Push-service-of-visible-no.patch

Aurora44+

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8688083 [details] [diff] [review]
0003-Bug-1210211-Part-3-Test-for-push-notification-quota-.patch

Aurora44+

Comment 35

[Carsten Book [:Tomcat]]

this has problems with the aurora uplift:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 7343fdb5d581
grafting 315311:7343fdb5d581 "Bug 1210211 - Part 1: Delay updating push quota. r=kitcambridge"
merging dom/push/PushRecord.jsm
merging dom/push/PushService.jsm
merging dom/push/test/xpcshell/head.js
merging modules/libpref/init/all.js
warning: conflicts while merging dom/push/PushService.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take a look ?

Comment 36

[William Chen [:wchen]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e2780789509
https://hg.mozilla.org/releases/mozilla-aurora/rev/d7a61804d40e
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a278f97f465

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2e2780789509
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d7a61804d40e
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3a278f97f465

Comment 38

[Chris Mills [:cmills]]

I added a note to try to added this; see the first "Note: ... " box in the following section:

https://developer.mozilla.org/en-US/docs/Web/API/Push_API#Push_concepts_and_usage

Is the wording ok here?

Comment 39

[Andrew Overholt [:overholt]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #38)
> I added a note to try to added this; see the first "Note: ... " box in the
> following section:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/
> Push_API#Push_concepts_and_usage
> 
> Is the wording ok here?

I asked wchen and he suggested mt would be a better person to verify so I asked him and he suggested the following small change which I made (hope that's ok):

https://developer.mozilla.org/en-US/docs/Web/API/Push_API$compare?locale=en-US&to=976179&from=972961

Comment 40

[Chris Mills [:cmills]]

(In reply to Andrew Overholt [:overholt] from comment #39)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #38)
> > I added a note to try to added this; see the first "Note: ... " box in the
> > following section:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/
> > Push_API#Push_concepts_and_usage
> > 
> > Is the wording ok here?
> 
> I asked wchen and he suggested mt would be a better person to verify so I
> asked him and he suggested the following small change which I made (hope
> that's ok):
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Push_API$compare?locale=en-
> US&to=976179&from=972961

Looks fine - cheers Andrew.