Closed Bug 1153504 Opened 9 years ago Closed 9 years ago

Implement per origin quotas for Push Notifications

Categories

(Core :: DOM: Push Subscriptions, defect)

Product:
Core
Component:
DOM: Push Subscriptions
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: dougt, Assigned: lina)

References

Details

Attachments

(2 files)

0001-Bug-1153504-Add-per-origin-push-quotas.patch
57.97 KB, patch
mt
: feedback-
Details | Diff | Splinter Review
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh
40 bytes, text/x-review-board-request
markh
: review+
Details 
      No description provided.
mt's proposal:

tl;dr
 We should count push messages and cut off an origin that sends too
many over time.

---

The biggest concern with enabling push is the potential for a
permission grant to be abused by a site.  As currently designed and
implemented, this is the sort of mistake that is made once and
regretted forever.

I don't want to tattoo our users in this way, but nor do I think that
creating additional incentives for sites to pester users is
advantageous either.

We need better mechanisms for accounting and visibility.  Doug has
implemented some accounting that he has exposed on
about:serviceworkers, but I think that we need a scheme that
encourages accountability more directly.

To that end, I propose the following process:

1. Each time a push message is received, the number of messages
received since the user last visited the site is counted.

2. If the message count exceeds a threshold, after the message is
delivered, the subscription for that origin is removed.  The service
worker is not given an opportunity to restore the subscription until
the user returns to the site.

3. The threshold will diminish with time, so that sites that don't see
any user engagement are more likely to get cut off.

There is no need to remove the permission that has been granted.  If
the user returns to the site, the service worker will be re-activated
and the subscription can be re-acquired.  The nature of the API is
such that this should happen automatically.

Applications will learn about this when the push service denies them
the push attempt.  They will have to resort to the classic modes of
bothering their users at this point.

Notes:

* This addresses the flight notice use case.  In that scenario, a user
buys a plane ticket, during which time they give consent for a push
notification.  A month later, the airline sends a push message,
informing them of their upcoming flight.  Since the push message is
delivered prior to killing the subscription, this use case is enabled.
The only limit here is the duration of a push subscription.

From the service perspective, we should do what we can to ensure that
subscriptions last long enough to support this model.

* The default threshold model is important to get right.  I'm going to
propose a formula that take just one input: the number of days since
the user last visited the site.
  messages = 8*days^-0.8
Capping this at 16 seems reasonable.

You can map this out on a table:
 | messages | time |
 | 1 | 13d10h |
 | 2 | 5d15h |
 | 3 | 3d9h |
 | 4 | 2d9h |
 | 8 | 1d0h |
 | 16 | 0d10h |

I think that this sort of shape (or something like it) draws a
reasonable compromise between the needs of applications and the needs
of users.  This sets an upper bound on the time that an application
can hide pushes, and it simultaneously limits the degree of that
activity if the application does operate on a longer timescale.

* In the future, we might want to help applications a little by
applying a different accounting and delivery regimen for messages that
are marked as lower priority.  That is, low priority messages are
subject to delays and they might not be delivered at all. Therefore, a
low priority message might be counted less toward this limit.  I think
that creates some good incentives to do the right thing with respect
to priorities.
(In reply to Doug Turner (:dougt) from comment #1)
> 2. If the message count exceeds a threshold, after the message is
> delivered, the subscription for that origin is removed.  The service
> worker is not given an opportunity to restore the subscription until
> the user returns to the site.

Is there anything in the push spec that lets us drop the subscription like this?  Can we instead just transparently re-enable the subscription once the user visits the site?

Also, how does this play with things like background sync?  In theory something like sync could start the service worker again before the user visits the site.  That would in theory give them the opportunity to re-subscribe prior to a user visit.  Again, a "suspended" subscription flag instead of subscription removal could possibly avoid that problem too.
Flags: needinfo?(martin.thomson)
:bkelly, I think that we can retain the subscription and simply stop monitoring for new messages, rather than deleting it.  The problem with that is the it results in no feedback to the application server.  Removing the subscription allows the push service to report failures.

We can potentially add to the API a means for the browser to report to the application that this is happening.

Oh, important note, we'd have to deny requests to resubscribe at that point too.

On consideration, I think that background sync probably needs to use a similar scheme if we do this.  I don't think that we want a background sync that runs indefinitely without the user actually using the results of all that work by at least visiting the origin.
Flags: needinfo?(martin.thomson)
Another thing that may respin the service worker is periodic updates, if the script changes on the server side.  I guess avoiding resubscription will take care of that case too though.
Also, doesn't capping at 16 messages make this impractical for let's say Gmail to show notifications for incoming email if the user receives many emails?
I think that the main point here is that any events that we deliver to a service worker that are are a) under the control of the origin, and b) keep the service worker alive outside of the usual fetch interception, need to be accounted.  That means background sync (the setInterval of SW) and web push right now.

On the push side of things, dropping messages is almost certainly OK.  Failing to raise on*sync seems safe by the same measure.

The main questions I have is: What sort of reporting do we need to provide so that applications don't become convinced that Firefox is broken?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> Also, doesn't capping at 16 messages make this impractical for let's say
> Gmail to show notifications for incoming email if the user receives many
> emails?

It does if they naively send notifications for every message.  That's partly why I ask about how we report status.  We might also need to look at different numbers; the actual numbers are entirely strawman values.

Note that if the site is open in a tab, then we should let them have as many SW activations as they like (though we might want to consider throttling if the tab has been GC'd, or maybe we can throttle to save power).

I can see us using other inputs to this logic as well: increase the budget if site has lots and lots of visits, increase if they have lots of persistent permissions, especially the geolocation one, etc...
(In reply to Ben Kelly [:bkelly] from comment #2)
> Also, how does this play with things like background sync?  In theory
> something like sync could start the service worker again before the user
> visits the site.  That would in theory give them the opportunity to
> re-subscribe prior to a user visit.

Subscription should probably only work if there's a corresponding browsing context available.

This throttling idea seems like a great way to avoid abuse. Love it. I guess background synchronization might need some changes to make it clear that you have permission, but no "subscription" (as it has been removed).
(In reply to Martin Thomson [:mt:] from comment #7)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #5)
> > Also, doesn't capping at 16 messages make this impractical for let's say
> > Gmail to show notifications for incoming email if the user receives many
> > emails?
> 
> It does if they naively send notifications for every message.  That's partly
> why I ask about how we report status.  We might also need to look at
> different numbers; the actual numbers are entirely strawman values.

I don't have a great answer for reporting, unfortunately.  One thing we could do is to report to the browser console, but I'm not sure how many developers look at that.  Another thing that we can do is to remember the reports, and show them on the web console the next time that the web developer opens it up.  What do you think?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> I don't have a great answer for reporting, unfortunately.  One thing we
> could do is to report to the browser console, but I'm not sure how many
> developers look at that.  Another thing that we can do is to remember the
> reports, and show them on the web console the next time that the web
> developer opens it up.  What do you think?

I was thinking API-level feedback, but using the web console is a good idea.  In terms of what we remember, I think a count would be a fine thing.  The trick there is that over extended periods of time, we would have to persist that information to indexedDB or something like that.  We could save a couple of messages too, but I wouldn't want to be on the hook for remembering ALL of them, and dumping up to 4k of binary data might not be that much use.

As for API, we might benefit from a simple flag somewhere that indicates that permission has expired.  Or we could generate another event I suppose (after the last triggering event expires).

(In reply to Anne (:annevk) from comment #8)
> Subscription should probably only work if there's a corresponding browsing
> context available.

That might break push in some corner cases.  If a subscription breaks or expires, then we'll want to deliver an event and allow the app to re-subscribe.  We might even allow that event to be "free".  Otherwise we could see breakage.

The example I used is particularly prone to this.  If we decide on the server end that subscriptions expire after 2 weeks, then potentially we are expiring subscriptions faster than the notifications are arriving.  I don't know what the actual numbers are likely to be for this; if the subscription duration was 2 years, like I hear Apple promises, then it might be OK.
Does the backend push server infrastructure guarantee delivery of every push message?  Or is it best effort?
(In reply to Ben Kelly [:bkelly] from comment #11)
> Does the backend push server infrastructure guarantee delivery of every push
> message?  Or is it best effort?

That depends, but it's best to assume reliable delivery.
What a bout having something like a history - weighted moving average instead of the time since last visit. So more visited sites can send more messages.
Work-in-progress sketch for push quotas, per :mt's proposal.
Assignee: dougt → kcambridge
Status: NEW → ASSIGNED
Attachment #8625127 - Flags: feedback?(nsm.nikhil)
Attachment #8625127 - Flags: feedback?(martin.thomson)
Attachment #8625127 - Flags: feedback?(dd.mozilla)
(In reply to Dragana Damjanovic [:dragana] from comment #13)
> What a bout having something like a history - weighted moving average
> instead of the time since last visit. So more visited sites can send more
> messages.

That makes sense, too. We could use the same frecency score as the location bar.
Comment on attachment 8625127 [details] [diff] [review]
0001-Bug-1153504-Add-per-origin-push-quotas.patch

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

Repeat after me: hg push -r . review

It's easier to do than a patch upload.  And it now sucks less than splinter.
Comment on attachment 8625127 [details] [diff] [review]
0001-Bug-1153504-Add-per-origin-push-quotas.patch

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

I'm not reviewing this on splinter, sorry.
Attachment #8625127 - Flags: feedback?(nsm.nikhil) → feedback-
Comment on attachment 8625127 [details] [diff] [review]
0001-Bug-1153504-Add-per-origin-push-quotas.patch

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

We discussed this in person at whistler and apart from reordering the read-write to IDB to not use locks, most things seem fine. Kit is going to update this and upload to reviewboard.

::: dom/push/PushDB.jsm
@@ +177,5 @@
>        this.newTxn(
>          "readonly",
>          this._dbStoreName,
>          function txnCb(aTxn, aStore) {
> +          aTxn.result = [];

Let's remove this filtering from here right now.

@@ +180,5 @@
>          function txnCb(aTxn, aStore) {
> +          aTxn.result = [];
> +
> +          let index = aStore.index("quota");
> +          let range = IDBKeyRange.bound([1, " "], [Infinity, "~"]);

Please add a comment explaining the magic constants here.

::: dom/push/PushService.jsm
@@ +734,5 @@
>            globalMM.broadcastAsyncMessage('pushsubscriptionchange', data);
>          }));
>    },
>  
> +  _calculateQuota: function(aPushRecord, aCurrentPush) {

aCurrentPush can be removed.

@@ +749,5 @@
> +      } else if (lastVisit > aPushRecord.lastPush) {
> +        // If the user visited the site since the last time we received a
> +        // notification, reset their quota.
> +        let daysElapsed = (aCurrentPush - lastVisit) / 24 / 60 / 60 / 1000;
> +        // TODO: 16 should be a pref.

Pref?

::: dom/push/PushServiceWebSocket.jsm
@@ +51,5 @@
>  // Don't modify this, instead set dom.push.debug.
>  let gDebuggingEnabled = true;
>  
>  function debug(s) {
> +  if (true || gDebuggingEnabled) {

get rid of debugging
Attachment #8625127 - Flags: feedback?(martin.thomson)
Attachment #8625127 - Flags: feedback?(dd.mozilla)
Bug 1153504 - Add per-origin push quotas. r?nsm
Attachment #8626400 - Flags: review?(nsm.nikhil)
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review10485

::: dom/push/PushServiceWebSocket.jsm:164
(Diff revision 1)
> +    objectStore.createIndex("quota", ["pushQuota", "scope"]);

needs to consider originAttributes too

::: dom/push/PushServiceWebSocket.jsm:966
(Diff revision 1)
> +    if (action == "register") {

I'm not sure why this was moved considering we could have an unregister request as the first thing and then we should still start the timer.

::: dom/push/PushServiceWebSocket.jsm:1037
(Diff revision 1)
> -      if (!this._ws && this._notifyRequestQueue) {
> +      if (!this._ws) {
> +        if (this._notifyRequestQueue) {
> -        this._notifyRequestQueue();
> +          this._notifyRequestQueue();
> -        this._notifyRequestQueue = null;
> +          this._notifyRequestQueue = null;
> -      }
> +        }
> -    }
> +      }

This isn't a logical change, please revert it.

::: dom/push/test/xpcshell/head.js:76
(Diff revision 1)
> +      handleCompletion: resolve

purely stylistic, could you move handleCompletion to the top of these 3.

::: dom/push/test/xpcshell/test_quota_exceeded.js:90
(Diff revision 1)
> +  let unregisterDefer = Promise.defer();

Please don't use the old style defer, even if it means writing the long form of the Promise ctor.

::: modules/libpref/init/all.js:4400
(Diff revision 1)
> +pref("dom.push.maxQuota", 16);

maxQuotaPerSubscription?

::: dom/push/PushDB.jsm:223
(Diff revision 1)
> +          let index = aStore.index("quota");
> +          // Fetch all unexpired records for all scopes, where
> +          // 1 <= quota <= Infinity.
> +          let range = IDBKeyRange.bound([1, MIN_UTF8], [Infinity, MAX_UTF8]);
> +          index.openCursor(range).onsuccess = function(event) {
> +            let cursor = event.target.result;
> +            if (cursor) {
> +              aTxn.result.push(cursor.value);
> +              cursor.continue();
> +            }
> +          };

I'm not fond of this index because it puts together two unrelated items, pushQuota and scope. Can't we only have an index on the quota and have a query of `IDBKeyRange.lower(1)`. It seems the scope isn't really relevant here, we care about the entire record when the quota is positive.

::: dom/push/test/xpcshell/head.js:220
(Diff revision 1)
> +    maxQuota: 16

Here and everywhere else where a new pair was introduced in a dictionary, put a trailing comma at the end. JS accepts it and it prevents future changes from affecting blame on unrelated lines due to just adding a comma to the previous entry.

::: dom/push/PushDB.jsm:213
(Diff revision 1)
>    getAllKeyIDs: function() {

This should be `getAllUnexpiredRecords` since it doesn't return just the keys, but entire records.

::: dom/push/PushService.jsm:1278
(Diff revision 1)
> +  onVisit: function(uri, visitId, lastVisit, sessionId, referringId, transitionType, guid, added) {
> +    if (!QUOTA_REFRESH_TRANSITIONS.has(transitionType)) {
> +      return;
> +    }
> +    let origin = uri.prePath;
> +    if (!this._expiredOrigins.has(origin)) {
> +      return;
> +    }

It sounds quite inefficient to do this 'garbage collection' all times that the user is browing. Do any of these options work?
1) Expire the subscription immediately when we reduce the quota to zero.
2) Have a once daily or so pass (see the nsIIdleServiceDaily that fires a `idle-daily` notification via the observer service approximately once every 24 hours) that 'GC's the expired registrations.

::: dom/push/PushService.jsm:1300
(Diff revision 1)
> +          originAttributes: {}, // TODO bug 1166350

These changes landed so use `record.originAttributes`.

::: dom/push/PushDB.jsm:276
(Diff revision 1)
> +   */
> +  update: function(aKeyID, aUpdateFunc) {
> +    return new Promise((resolve, reject) =>
> +      this.newTxn(

On a related note, could we add code to PushDB.put that enforces that the record has `{scope, originAttributes, pushQuota}` set and reject otherwise.
Attachment #8626400 - Flags: review?(nsm.nikhil)
I guess my review is a little paradoxical in that first i asked to add originAttributes to the index, then to have the index only have the quota. I mean the latter, the scope and originAttributes have to be matched against,  but they don't have to be in the index.
https://reviewboard.mozilla.org/r/12095/#review10637

> I'm not fond of this index because it puts together two unrelated items, pushQuota and scope. Can't we only have an index on the quota and have a query of `IDBKeyRange.lower(1)`. It seems the scope isn't really relevant here, we care about the entire record when the quota is positive.

The scope is only used in `dropExpired()`, when we remove all expired subscriptions for an origin. I thought having a compound index would be useful for that case, but that was before we had `originAttributes`. I'll just filter out unexpired subscriptions via `cursor.value.pushQuota > 0`.

> It sounds quite inefficient to do this 'garbage collection' all times that the user is browing. Do any of these options work?
> 1) Expire the subscription immediately when we reduce the quota to zero.
> 2) Have a once daily or so pass (see the nsIIdleServiceDaily that fires a `idle-daily` notification via the observer service approximately once every 24 hours) that 'GC's the expired registrations.

Thinking out loud: we only clean up and notify service workers when the user visits a site in the `expiredOrigins` set. I'd expect most visits to hit the `!this._expiredOrigins.has(origin)` case and return early, avoiding the database lookup. If you think this will happen more frequently, though, using `nsIIdleServiceDaily` would make sense—and we could then jettison the history observer and `expiredOrigins` set.

I don't think (1) will work, though. We only want to tell the service worker about the expired registration when the user visits the site again. If we dropped the subscription and notified the service worker immediately, it could just request a new subscription.

> These changes landed so use `record.originAttributes`.

Factored out into a separate function, since we're emitting `pushsubscriptionchange` 3 times now.

> I'm not sure why this was moved considering we could have an unregister request as the first thing and then we should still start the timer.

`unregister` is fire-and-forget; we only ever track and time out `register` requests, so we're starting the timer for no reason. Regardless, that doesn't belong in this patch. Thanks for the catch!

> This isn't a logical change, please revert it.

Whoops. I reverted this, and some other leftover `storageReady()` changes.

> Please don't use the old style defer, even if it means writing the long form of the Promise ctor.

Agreed, though our existing tests use `Promise.defer()`, too (I shouldn't have used the old-style deferreds in the first place, but so it goes). Would you like me to fix up just the quota tests in this patch, or file a follow-up bug to fix all of them? It's a trivial search-and-replace, but fixing them all seems out of scope for this patch.
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

Bug 1153504 - Add per-origin push quotas. r?nsm
Attachment #8626400 - Flags: review?(nsm.nikhil)
https://reviewboard.mozilla.org/r/12095/#review10655

> Agreed, though our existing tests use `Promise.defer()`, too (I shouldn't have used the old-style deferreds in the first place, but so it goes). Would you like me to fix up just the quota tests in this patch, or file a follow-up bug to fix all of them? It's a trivial search-and-replace, but fixing them all seems out of scope for this patch.

Don't bother doing that here. You could file a good first bug for that and set yourself as the mentor.

> Thinking out loud: we only clean up and notify service workers when the user visits a site in the `expiredOrigins` set. I'd expect most visits to hit the `!this._expiredOrigins.has(origin)` case and return early, avoiding the database lookup. If you think this will happen more frequently, though, using `nsIIdleServiceDaily` would make sense—and we could then jettison the history observer and `expiredOrigins` set.
> 
> I don't think (1) will work, though. We only want to tell the service worker about the expired registration when the user visits the site again. If we dropped the subscription and notified the service worker immediately, it could just request a new subscription.

nsIIdleServiceDaily seems better in terms of avoiding a IDB call every navigation.
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review10657

This looks good for the most part. I'd like to see the idle-daily change. Meanwhile :mt can start reviewing the quota policy kernel implementation.

::: dom/push/PushDB.jsm:342
(Diff revision 2)
> +  dropExpired: function(aOrigin) {

If we switch to daily observer, we would remove all expired registrations for all origins when that happens, correct? In that case we don't need `aOrigin` and the wonky (sorry, I'm aware this is a valid way to query IDB, it just isn't very elegant :)) UTF8 codepoint lookup.

::: dom/push/PushDB.jsm:30
(Diff revision 2)
> +         aPageRecord.originAttributes != null;

You can assert that `originAttributes` has to be a string. The default is an empty string.

::: dom/push/PushDB.jsm:33
(Diff revision 2)
> +function isValidRecord(aRecord) {
> +  return isValidPageRecord(aRecord) && aRecord.pushQuota >= 0;
> +}

`isValidUnexpiredRecord()`?

::: dom/push/PushDB.jsm:302
(Diff revision 2)
> +   * @param {Function} aUpdateFunc A function that receives the existing
> +   *  registration record as its argument.

[as its argument ] and returns a boolean. The update is only applied if the function returns true.

::: dom/push/PushService.jsm:841
(Diff revision 2)
> +      if (condition(aPushRecord) === false) {
> +        return false;
> +      }
> +      aPushRecord.pushCount = aPushRecord.pushCount + 1;
> +      aPushRecord.lastPush = Date.now();
> +    });

`return true` at the end?

::: dom/push/PushService.jsm:929
(Diff revision 2)
> +  _isTabOpen: function(origin) {
> +    let windows = Services.wm.getEnumerator("navigator:browser");
> +    while (windows.hasMoreElements()) {
> +      let window = windows.getNext();

If this was copied from somewhere, can it be refactored? If not, this needs browser peer review.

::: dom/push/PushServiceWebSocket.jsm:898
(Diff revision 2)
> +        pushQuota: this._mainPushService.getInitialQuota(tmp.record)

Trailing comma.

I'd like
Attachment #8626400 - Flags: review?(nsm.nikhil)
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

Bug 1153504 - Add per-origin push quotas. r?nsm,mt,markh
Attachment #8626400 - Attachment description: MozReview Request: Bug 1153504 - Add per-origin push quotas. r?nsm → MozReview Request: Bug 1153504 - Add per-origin push quotas. r?nsm,mt,markh
Attachment #8626400 - Flags: review?(nsm.nikhil)
Attachment #8626400 - Flags: review?(martin.thomson)
Attachment #8626400 - Flags: review?(markh)
https://reviewboard.mozilla.org/r/12093/#review10675

I don't know how useful this will be, because I see a new review for this has just come out.

::: dom/push/PushDB.jsm:24
(Diff revision 2)
> +const MAX_UTF8 = "\uffff";

The name and value make no sense to me.  UTF-8 encodes 32-bit values as 8-bit values.  A 16-bit value suggests that there is something wrong.

::: dom/push/PushDB.jsm:29
(Diff revision 2)
> +  return aPageRecord && typeof aPageRecord.scope == "string" &&
> +         aPageRecord.originAttributes != null;

nit: typeof aPageRecord.scope === "string" && aPageRecord.originAttributes
or: typeof aPageRecord.scope === "string" && typeof aPageRecord.originAttributes === "object"

::: dom/push/PushDB.jsm:67
(Diff revision 2)
> +        `Scope, originAttributes, and pushQuota are required! ${
> +          JSON.stringify(aRecord)}`)

The next person who uses new and unnecessary JS syntax will be put to the wheel.  Please use strings and the + operator.

::: dom/push/PushDB.jsm:272
(Diff revision 2)
> +  getAllExpired: function() {

This almost completely duplicates getAllUnexpired.  Fix that please.

::: dom/push/PushDB.jsm:322
(Diff revision 2)
> +            if (shouldUpdate === false) {

!shouldUpdate

::: dom/push/PushDB.jsm:321
(Diff revision 2)
> +            let shouldUpdate = aUpdateFunc(record);
> +            if (shouldUpdate === false) {

I would have aUpdateFunc return an updated record, without modifying the record in place.  It can return null/undefined if no update is needed.

::: dom/push/PushDB.jsm:313
(Diff revision 2)
> +          aStore.get(aKeyID).onsuccess = function getTxnResult(aEvent) {

Don't bother naming spontaneous functions.  Instead, use the arrow form:

foo.onsuccess = e => {
};

::: dom/push/PushDB.jsm:357
(Diff revision 2)
> +          let range = IDBKeyRange.bound([aOrigin], [aOrigin + MAX_UTF8]);

This works just as well:

    aOrigin, aOrigin + '0'

Since all scopes will start with '/'.  All you need is a comment.

::: dom/push/PushService.jsm:86
(Diff revision 2)
> +const QUOTA_REFRESH_TRANSITIONS = new Set([

More unnecessary new JS.  A set might be useful if you had thousands of entries: you have 5.

::: dom/push/PushService.jsm:556
(Diff revision 2)
> +      for (let record of records) {
> +        this.trackExpiredRegistration(record);
> +      }

records.forEach(record => this.trackExpiredRegistration(record))

::: dom/push/PushService.jsm:559
(Diff revision 2)
> +    }).then(() => {
> -    this._startObservers();
> +      this._startObservers();
> -    return Promise.resolve();
> +    }, err => {
> +      debug("startService: Error loading expired registrations: " + err);
> +      this._startObservers();
> +    });

.catch(err => debug(...))
    .then(_ => this._startObservers());

::: dom/push/PushService.jsm:746
(Diff revision 2)
>        .then(_ => this._db.put(aRecord)
>          .then(record => {
> -          let globalMM = Cc['@mozilla.org/globalmessagemanager;1']
> -                           .getService(Ci.nsIMessageListenerManager);
> -          Services.obs.notifyObservers(
> +          this._notifySubscriptionChangeObservers(record);
> +        }));
> +  },

Deindent:
    .then(_ => this._db.put(aRecord))
    .then(record => this._notifyblah(record))

::: dom/push/PushService.jsm:763
(Diff revision 2)
> +      // If the user cleared their history since visiting the site, allow a
> +      // final notification before expiring the registration.
> +      pushQuota = 1;

This reads poorly, and I think it is wrong.

I think that you meant to say: "If the user cleared their history *but retained their push permission*, ..."

I think that we should make service workers dormant at that point.  That's going to be sucky, but we have to pretend that this is a site that we have never seen before.

::: dom/push/PushService.jsm:752
(Diff revision 2)
> -          let data = {
> +  getInitialQuota: function(aPageRecord) {

All this quota-related stuff can be moved to a different class.

::: dom/push/PushService.jsm:800
(Diff revision 2)
> +      if (aPushRecord.pushQuota == Infinity) {
> +        return this._updateRecord(keyID, aPushRecord => {
> +          return typeof condition != "function" || condition(aPushRecord);
> +        });
> +      }

I'm pretty sure that I don't understand this Infinity thing.

::: dom/push/PushNotificationService.js:43
(Diff revision 2)
> +      scope,
> +      originAttributes,

The shorthand here is not not bad, can you use the explicit syntax please?

scope: scope,
originAttributes: originAttributes,

::: dom/push/PushNotificationService.js:45
(Diff revision 2)
> +      maxQuota: Infinity,

Why does this register with infinite duration?

::: dom/push/PushService.jsm:796
(Diff revision 2)
> +      if (!aPushRecord) {
> +        debug(`receivedPushMessage: No record for key ID ${keyID}`);
> +        return;
> +      }

Throw.  That avoids having to null-check the record later.

::: dom/push/PushService.jsm:800
(Diff revision 2)
> +      if (aPushRecord.pushQuota == Infinity) {
> +        return this._updateRecord(keyID, aPushRecord => {
> +          return typeof condition != "function" || condition(aPushRecord);
> +        });
> +      }
> +      return this._getLastVisit(aPushRecord).then(lastVisit => {
> +        return this._updateRecord(keyID, aPushRecord => {

This can be simplified:
```
let p;
if (aPushRecord.hasQuota()) {
  return this._getLastVisit();
} else {
  return Promise.resolve(Date.now());
}
return p.then(lastVisit => {
  // condition function?
  aPushRecord.updateQuota(lastVisit);
})
```

I need to check on the condition function use case, but I would not make its presence optional like you currently have it.

::: dom/push/PushService.jsm:821
(Diff revision 2)
> +        this._sendRequest("unregister", aPushRecord).catch(error => {
> +          debug(`receivedPushMessage: Unregister error: ${error}`);
> +        });
> +        // Track the expired registration until the user revisits the site.
> +        this.trackExpiredRegistration(aPushRecord);

Please move this to a new function call `makeDormant` or something like that.  Auditing this for correctness will be a big problem otherwise.

::: dom/push/PushService.jsm:842
(Diff revision 2)
> +        return false;
> +      }
> +      aPushRecord.pushCount = aPushRecord.pushCount + 1;
> +      aPushRecord.lastPush = Date.now();
> +    });

A function that returns a boolean should return true if it doesn't return false.  Returning undefined looks like a bug.

::: dom/push/PushService.jsm:834
(Diff revision 2)
> -      .then(_ => this._notifyApp(aPushRecord, message));
> +      if (aPushRecord.pushQuota <= 0) {
> +        // Because `unregister` is advisory only, we can still receive messages
> +        // for stale registrations from the server.
> +        debug(`receivedPushMessage: Ignoring update for expired key ID ${keyID}`);
> +        this.trackExpiredRegistration(aPushRecord);
> +        return false;
> +      }

This is happening twice when a push message is received.  That's not good.

::: dom/push/PushService.jsm:831
(Diff revision 2)
>  
> -  receivedPushMessage: function(aPushRecord, message) {
> -    this._db.put(aPushRecord)
> +  _updateRecord: function(keyID, condition) {
> +    return this._db.update(keyID, aPushRecord => {

All this nesting of condition functions is very hard to follow.

::: dom/push/PushServiceWebSocket.jsm:135
(Diff revision 2)
>      //XXXnsm We haven't shipped Push during this upgrade, so I'm just going to throw old
>      //registrations away without even informing the app.

Please don't drop old subscriptions as a result of this change.  You can make all the other code conditional on v <= 3 and the new index conditional on v <= 4.

::: dom/push/PushServiceWebSocket.jsm:1024
(Diff revision 2)
> -        this._enqueue(_ => {
> +        let promise = new Promise((resolve, reject) => {
> -          return new Promise((resolve, reject) => {
> -                               this._notifyRequestQueue = resolve;
> +          this._notifyRequestQueue = resolve;
> -                             });
> +        });
> -        });
> +        this._enqueue(_ => promise);

Seems unnecessary.

::: dom/push/PushServiceWebSocket.jsm
(Diff revision 2)
> -                                                  "careless waste of time.");
>        }
>        else {
>          debug("No significant version change: " + aLatestVersion);
> +        return false;
>        }

See previous comment regarding the return of both undefined and false from a function.

::: dom/push/PushService.jsm:967
(Diff revision 2)
> +  _getLastVisit: function(pushRecord) {

New class please.

::: dom/push/PushService.jsm:1017
(Diff revision 2)
> +            if (lastVisit > pushRecord.lastPush) {
> +              // Drop stale registrations if the user visited the page since
> +              // we expired the subscription.

This comment is misleading.  You want to make a new registration if the user has since visited the site.  "Drop stale registrations" implies something else.

::: dom/push/PushService.jsm:1028
(Diff revision 2)
> +            // TODO: Clarify if we should reject with an error instead of
> +            // returning the stale record.
> +            return pushRecord;

Yes, rejecting here is the right thing to do.

::: dom/push/PushService.jsm:1255
(Diff revision 2)
> +  trackExpiredRegistration: function(pushRecord) {
> +    let origin = Services.io.newURI(pushRecord.scope, null, null).prePath;

This can be moved to the new helper class.

::: dom/push/test/xpcshell/test_quota_exceeded.js:27
(Diff revision 2)
> +    pageURL: 'https://example.com/auctions/123',

Now that I see this here, I wonder why we are storing the original URL that triggered the registration.  We only need to store the origin.
https://reviewboard.mozilla.org/r/12095/#review10657

> If this was copied from somewhere, can it be refactored? If not, this needs browser peer review.

Mark, could I trouble you to review this function, please? I cribbed most of this from the tab Sync engine (and `sdk/tabs`), but I'm all ears if there's a better way to check if a user has a tab open for a particular origin.

> `isValidUnexpiredRecord()`?

Renamed to `isValidPushRecord()`, since `pushQuota === 0` for expired records.
https://reviewboard.mozilla.org/r/12093/#review10675

> Don't bother naming spontaneous functions.  Instead, use the arrow form:
> 
> foo.onsuccess = e => {
> };

Sounds good. I used the named form because that's the dominant style in this file.

> Now that I see this here, I wonder why we are storing the original URL that triggered the registration.  We only need to store the origin.

Copy-pasted from an older test. Thanks for the catch!

> Yes, rejecting here is the right thing to do.

Would you like a new error type, or does `NotFoundError` suffice? (Our other current options are `SecurityError`, `NetworkError`, `TimeoutError`, and `AbortError`).

> I'm pretty sure that I don't understand this Infinity thing.

We don't want to enforce a quota for subscriptions created through the XPCOM API. For example, Hello could use `app://loop/calls` for the scope URL; Sync will probably use `app://sync/{collectionType}`. This skips the history lookup for that case. I think your proposed `hasQuota()` method below would be `return Number.isFinite(this.pushQuota)`.

> This reads poorly, and I think it is wrong.
> 
> I think that you meant to say: "If the user cleared their history *but retained their push permission*, ..."
> 
> I think that we should make service workers dormant at that point.  That's going to be sucky, but we have to pretend that this is a site that we have never seen before.

Yes.

To clarify: if the user clears their history, and the site then tries to send a message...you're suggesting we make the worker dormant immediately, without letting it handle that message?

> This is happening twice when a push message is received.  That's not good.

Sorry? Are you referring to `trackExpiredRegistration` here?
https://reviewboard.mozilla.org/r/12093/#review10675

> Sounds good. I used the named form because that's the dominant style in this file.

The guidance I received was that the named form is a relic of a time when identifying a function needed a name.  That isn't necessary any more, and it could be harmful.  I now remove names when I see them.  (And I only use the function() {} form for constructors and class methods.)

> Yes.
> 
> To clarify: if the user clears their history, and the site then tries to send a message...you're suggesting we make the worker dormant immediately, without letting it handle that message?

I think that we can drop the message and then make the worker dormant.  Rather than having another hook installed to watch for history clearing.

> We don't want to enforce a quota for subscriptions created through the XPCOM API. For example, Hello could use `app://loop/calls` for the scope URL; Sync will probably use `app://sync/{collectionType}`. This skips the history lookup for that case. I think your proposed `hasQuota()` method below would be `return Number.isFinite(this.pushQuota)`.

OK.  In that case, setting the value to Infinity should be sufficient.  Infinity - 1 is Infinity, so you can run those applications through almost the same process.  All the special case stuff is a little annoying to wade through.

> Sorry? Are you referring to `trackExpiredRegistration` here?

Yep.

> Would you like a new error type, or does `NotFoundError` suffice? (Our other current options are `SecurityError`, `NetworkError`, `TimeoutError`, and `AbortError`).

I would use AbortError, I think.  But that's a bike shed and it doesn't matter much.

> Copy-pasted from an older test. Thanks for the catch!

Yeah, the push code seems to be overdoing it.

I think that this module is going to need a serious rework very soon.
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

Bug 1153504 - Add per-origin push quotas. r?nsm,mt,markh
https://reviewboard.mozilla.org/r/12095/#review10751

::: dom/push/PushRecord.jsm:40
(Diff revision 4)
> +function PushRecord(props) {

Ideally, we'd have all the database methods return `PushRecord` instances, instead of POJOs.

This would then allow us to move methods like `getKeyFromRecord`, `prepareRegister`, and `prepareRegistration` out of `PushService*` and into `PushRecord*`.

But I think that's out of scope for this patch. In the meantime, the naming convention I'm using is `record` for POJOs, and `pushRecord` for instances.
https://reviewboard.mozilla.org/r/12093/#review10675

> I think that we can drop the message and then make the worker dormant.  Rather than having another hook installed to watch for history clearing.

Cool. I don't think we need a hook either way, since we won't expire the subscription unless the site actively tries pushing to it. At that point, the site won't exist in the visits table if the user cleared history. I went ahead and updated the patch to drop updates if we haven't seen the site before.

> OK.  In that case, setting the value to Infinity should be sufficient.  Infinity - 1 is Infinity, so you can run those applications through almost the same process.  All the special case stuff is a little annoying to wade through.

Done. Having a separate `PushRecord` class makes things a bit easier.

> Yeah, the push code seems to be overdoing it.
> 
> I think that this module is going to need a serious rework very soon.

Let's file some follow-up tickets!
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review10785

::: dom/push/PushDB.jsm:306
(Diff revision 4)
> +            if (!newRecord) {
> +              debug("update: Ignoring update for key ID " + aKeyID);
> +              return;
> +            }
> +            if (!isValidPushRecord(newRecord)) {

Delete the if (!newRecord) block, since isValidPushRecord has a null check.

::: dom/push/PushRecord.jsm:38
(Diff revision 4)
> +const MAX_UTF8 = "\x7f";

This still has the wrong name.  Maybe you can just use the value inline and add a comment about the choice of value there.

::: dom/push/PushRecord.jsm:51
(Diff revision 4)
> +    let pushQuota = +suggestedQuota;

Nit: drop the redundant '+'

::: dom/push/PushRecord.jsm:52
(Diff revision 4)
> +    this.pushQuota = !isNaN(pushQuota) && pushQuota >= 0 ? pushQuota :

Parentheses around the conditional.

::: dom/push/PushRecord.jsm:50
(Diff revision 4)
> +  setQuota(suggestedQuota) {

More shorthand.  I'm not sure how I feel about this one...

::: dom/push/PushRecord.jsm:62
(Diff revision 4)
> +    if (lastVisit == -1) {

A weak comparison, with -1.  Would lastVisit < 0 be OK?

::: dom/push/PushRecord.jsm:53
(Diff revision 4)
> +                     prefs.get("maxQuotaPerSubscription");

This pref didn't hit all.js.  What is the default value?

::: dom/push/PushRecord.jsm:98
(Diff revision 4)
> +    return PlacesUtils.withConnectionWrapper("PushService.getLastVisit", db => {

s/PushService/PushRecord

::: dom/push/PushRecord.jsm:130
(Diff revision 4)
> +    return Services.io.newURI(this.scope, null, null).prePath;

Perhaps you can store scope as a URI and only call newURI in the constructor.  Since you only need the origin, perhaps this can store only the origin...

::: dom/push/PushRecord.jsm:120
(Diff revision 4)
> +      if (!rows.length) {
> +        return -1;
> +      }
> +      // Places records times in microseconds.
> +      let lastVisit = rows[0].getResultByIndex(0);
> +      return lastVisit / 1000;
> +    });

It might be better to return a Date() here, which allows for the value to be null.  -1 is a valid time offset, after all.

::: dom/push/PushService.jsm:746
(Diff revision 4)
> +        newRecord.updateQuota(lastVisit);
> +        newRecord.pushCount = newRecord.pushCount + 1;
> +        newRecord.lastPush = Date.now();

Rather than updateQuota() maybe you can use a receivedPush() method to update all three variables here.  Encapsulation FTW.

::: dom/push/PushService.jsm:861
(Diff revision 4)
> -        if (pushRecord === undefined) {
> +        if (aRecord === undefined) {

!aRecord is fine.

::: dom/push/PushService.jsm:1056
(Diff revision 4)
>        .then(_ => this._db.getByIdentifiers(aPageRecord))
> -      .then(pushRecord => {
> -        if (!pushRecord) {
> +      .then(aRecord => {
> +        if (!aRecord) {
>            return null;
>          }
> +        let pushRecord = this._service.newPushRecord(aRecord);

I wonder if the db couldn't be made responsible for construction of the PushRecord.

::: dom/push/test/xpcshell/test_quota_exceeded.js:48
(Diff revision 4)
> +    uri: Services.io.newURI('https://example.com/login', null, null),

It seems like you could move this newURI call into addVisit.

::: dom/push/test/xpcshell/test_quota_exceeded.js:91
(Diff revision 4)
> +    serverURI: 'wss://push.example.org/',

I'm not a big fan of adding new tests that are based on a thing that we intend to deprecate.
Attachment #8626400 - Flags: review?(martin.thomson)
Summary: Decide what to do about push events that do not generate UI → Implement per origin quotas for Push Notifications
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review10831

::: dom/push/PushService.jsm:448
(Diff revision 4)
> -      this._changeStateConnectionEnabledEvent(prefs.get("connection.enabled"));
> +        return this._changeStateConnectionEnabledEvent(prefs.get("connection.enabled"));

The return here seems a little strange, given it's not used. I think we should probably have a .catch on the "outer" promise.

::: dom/push/PushRecord.jsm:138
(Diff revision 4)
> +      if (window.closed || PrivateBrowsingUtils.isWindowPrivate(window)) {

It seems surprising we want to skip private windows here - is that by intent?

::: dom/push/PushService.jsm:738
(Diff revision 4)
> +          return;

I think this should explicitly return null (and similarly a few lines below)

::: dom/push/PushServiceHttp2.jsm:417
(Diff revision 4)
> +    if (aOldVersion <= 4) {

I'm surprised to see <= here, as I'd expect a future version 5 upgrade would attempt to create an already existing index, but I may well be missing something.

(Ditto in PushServiceWebSocket.jsm below)
Attachment #8626400 - Flags: review?(markh)
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review10833

Ship It!
I'm still learning how to use review board - I thought it would give me the chance to add general comments before publishing! I didn't look at the entire patch carefully as it seems to be getting enough love from people more familiar with the code. The existingTab() function looks fine notwithstanding my query re private windows.
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review11103

::: dom/push/PushDB.jsm:288
(Diff revision 4)
> +   * @returns {Promise} A promise for the updated record, or `undefined` if the

"A promise resolved with the updated record, or with undefined if the record was not updated."

::: dom/push/PushRecord.jsm:58
(Diff revision 4)
> +      // Ignore updates if the registration is already expired, or isn't

hasQuota() only checks if the quota is finite, and nothing about if it is expired (indicated by 0). Is the comment correct or the definition of hasQuota()?

::: dom/push/PushRecord.jsm:155
(Diff revision 4)
> +  hasQuota() {

Please call this 'quotaApplies()' indicating that a quota does apply to it even if that quota is zero.

::: dom/push/PushServiceWebSocket.jsm:129
(Diff revision 4)
>    upgradeSchema: function(aTransaction,

Can we consolidate most of this schema creation function except the service specific indices into PushService?

::: dom/push/PushServiceWebSocket.jsm:1052
(Diff revision 4)
>          debug("Version changed, notifying app and updating DB");
> -        aPushRecord.version = aLatestVersion;
> -        aPushRecord.pushCount = aPushRecord.pushCount + 1;
> +        pushRecord.version = aLatestVersion;
> +        return pushRecord;
> -        aPushRecord.lastPush = new Date().getTime();

This debug no longer applies here.

::: dom/push/PushService.jsm:760
(Diff revision 4)
> +        // Drop the registration in the background. The service worker will
> +        // not be given an opportunity to restore the registration until the
> +        // next time the user visits the site.

Where is the SW given an opportunity to update the registration when the user visits the site next?
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

Bug 1153504 - Add per-origin push quotas. r?nsm,mt
Attachment #8626400 - Attachment description: MozReview Request: Bug 1153504 - Add per-origin push quotas. r?nsm,mt,markh → MozReview Request: Bug 1153504 - Add per-origin push quotas. r?nsm,mt
Attachment #8626400 - Flags: review?(nsm.nikhil)
Attachment #8626400 - Flags: review?(martin.thomson)
Attachment #8626400 - Flags: review+
https://reviewboard.mozilla.org/r/12095/#review10485

> Don't bother doing that here. You could file a good first bug for that and set yourself as the mentor.

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=1183440
https://reviewboard.mozilla.org/r/12095/#review10785

> It might be better to return a Date() here, which allows for the value to be null.  -1 is a valid time offset, after all.

Good call, though I don't think we're recording any visits from 1969. ;-) I don't want to `null`-check the result of `getLastVisit()` each time—and > will coerce `null` to 0, anyway—so I changed it to `-Infinity`. That'll always be < `record.lastPush`.

> This pref didn't hit all.js.  What is the default value?

Sorry, `modules/libpref/init/all.js` still had the old name. Fixed.
https://reviewboard.mozilla.org/r/12095/#review10831

> I'm surprised to see <= here, as I'd expect a future version 5 upgrade would attempt to create an already existing index, but I may well be missing something.
> 
> (Ditto in PushServiceWebSocket.jsm below)

Whoops. It should, indeed, be <. Thanks for catching that!

> It seems surprising we want to skip private windows here - is that by intent?

Good question! ;-) IIUC, page visits in private windows aren't recorded in Places, so the most recent visit (assuming one exists) will always be from a non-private window. I wanted to match that for consistency...but maybe this is actually inconsistent, and should be removed. What do you think?

More broadly, I'm not sure how Push should interact with private windows (do we want separate quotas for pages visited in private windows?)...assuming we even want to allow Push for them.
https://reviewboard.mozilla.org/r/12095/#review11103

> Can we consolidate most of this schema creation function except the service specific indices into PushService?

Consolidated and added a `keyPath` argument to `PushDB`, since that's the only value that's different between the two.

> Where is the SW given an opportunity to update the registration when the user visits the site next?

It'll receive a `pushsubscriptionchange` notification on the next `idle-daily` event, instead of the next visit. Updated the comment to match.
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review11833

::: dom/push/PushDB.jsm:83
(Diff revision 5)
> +      objectStore.createIndex("pushQuota", "pushQuota", { unique: false });

: you don't have to prefix everything with "push".

::: dom/push/PushService.jsm:1088
(Diff revisions 3 - 5)
> -        let keyID = this._service.getKeyFromRecord(pushRecord);
> -        return this._getLastVisit(pushRecord).then(lastVisit => {
> -          if (lastVisit > pushRecord.lastPush) {
> +        let keyID = record.getKeyID();
> +        return record.getLastVisit().then(lastVisit => {
> +          if (lastVisit > record.lastPush) {
>              // If the user revisited the site, drop the expired push
>              // registration and notify the associated service worker.
>              return this._db.delete(keyID).then(() => {

Move the key id down to here.

::: dom/push/PushServiceHttp2.jsm:793
(Diff revisions 3 - 5)
> +  getKeyID() {
> +    return this.subscriptionUri;
> +  },

get keyId() { return this.subscriptionUri; }

::: dom/push/PushServiceHttp2.jsm:791
(Diff revisions 3 - 5)
> +  __proto__: PushRecord.prototype,

I've seen almost 10 different ways to inherit in JS.  This is perhaps the worst.  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto

I prefer Child.prototype = Object.create(Parent.prototype);

::: dom/push/PushRecord.jsm:140
(Diff revision 5)
> +      // `gBrowser` on Desktop; `BrowserApp` on Fennec.
> +      let tabs = window.gBrowser ? window.gBrowser.tabContainer.children :
> +                 window.BrowserApp.tabs;

That difference between desktop and fennec is a firing offence.  (Not your fault).
Attachment #8626400 - Flags: review?(martin.thomson) → review+
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review11855

Ship It!
Attachment #8626400 - Flags: review?(nsm.nikhil) → review+
https://reviewboard.mozilla.org/r/12095/#review10831

> The return here seems a little strange, given it's not used. I think we should probably have a .catch on the "outer" promise.

You're right, I think this was left over from a previous incarnation. Thanks!
Thanks, Nikhil and Martin!

Mark, could you give this a final sanity check, please? I'd also be interested in your thoughts on private browsing...I think the `isWindowPrivate` check is correct, but maybe we should all continue the discussion in a separate bug.
Flags: needinfo?(markh)
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

Bug 1153504 - Add per-origin push quotas. r?markh
Attachment #8626400 - Attachment description: MozReview Request: Bug 1153504 - Add per-origin push quotas. r?nsm,mt → MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh
Attachment #8626400 - Flags: review?(markh)
Attachment #8626400 - Flags: review+
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

Bug 1153504 - Add per-origin push quotas. r?markh
Comment on attachment 8626400 [details]
MozReview Request: Bug 1153504 - Add per-origin push quotas. r?markh

https://reviewboard.mozilla.org/r/12095/#review11913

LGTM. Note that I might be confused by reviewboard, but please ensure you have (and list in the commit message) r+ from a DOM peer for the patch you want to land.

::: dom/push/PushDB.jsm:97
(Diff revision 7)
> +          JSON.stringify(aRecord)

there seems a slight risk the stringify will throw here and above - in particular, when it sees a "circular" object. It might make sense to have something like: let recordRepr; try { recordRepr = JSON.stringify(aRecord); } catch (ex) {recordRepr = aRecord}

or similar.

::: dom/push/PushService.jsm:593
(Diff revision 7)
> +    Services.obs.removeObserver(this, "idle-daily");

While you are touching this, I think you might as well remove the trailing "false" from the line above - it doesn't throw, but it's wrong.

::: dom/push/test/worker.js:11
(Diff revision 7)
> -      event.data.text().length > 0) {
> +      event.data.text().length >= 0) {

it looks like this line should just be removed

::: dom/push/PushRecord.jsm:137
(Diff revision 7)
> +      if (window.closed || PrivateBrowsingUtils.isWindowPrivate(window)) {

I've no problem with leaving this private check if private windows can't register push notifications (but yeah, even if they could, we'd probably need a flag on the record to indicate if it originated from a private window and then only consider such windows.)
Attachment #8626400 - Flags: review?(markh) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/340a785af78cea73e1f9b7d5980499ea5b97ddaf
changeset:  340a785af78cea73e1f9b7d5980499ea5b97ddaf
user:       Kit Cambridge <kcambridge@mozilla.com>
date:       Thu Jun 25 14:52:57 2015 -0700
description:
Bug 1153504 - Add per-origin push quotas. r=nsm,mt,markh
Thanks, Mark! I think it's okay to leave the `JSON.stringify` calls, since we only ever pass `PushRecord` wrappers (and POJOs) that we create to the `PushDB` methods.

The fact that we're stringifying the objects every time—regardless of whether debugging is enabled—isn't great, though. I see FxA and Sync use https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Log.jsm, which would give us substitutions for free...but it doesn't look like any of the other DOM modules use it. Maybe it's not so bad.
Flags: needinfo?(markh)
https://hg.mozilla.org/mozilla-central/rev/340a785af78c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: