Closed Bug 1223202 Opened 4 years ago Closed 4 years ago

Only send subscription change events if the Push permission is granted

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file, 1 obsolete file)

There are two problems with how we handle `pushsubscriptionchange` events:

1. We fire `pushsubscriptionchange` before removing the record from IndexedDB. We also use read-then-delete instead of transactions. This is racy, since the worker could receive the event before the subscription is dropped. It's a contrived case, but I ran into it when I applied the patch in bug 1219064.

2. When we drop all records (in case the client ID or server URL changes), we shouldn't fire `pushsubscriptionchange` for expired subscriptions. A subscription can only expire if it exceeds its quota, or its permission is revoked. Both cases are already handled by the `idle-daily` and permission change observers, respectively.
Bug 1223202 - Only send subscription change events if the Push permission is granted. r?mt
Attachment #8685150 - Flags: review?(martin.thomson)
Attachment #8685150 - Flags: review?(martin.thomson) → review+
Comment on attachment 8685150 [details]
MozReview Request: Bug 1223202 - Only send subscription change events if the Push permission is granted. r?mt

https://reviewboard.mozilla.org/r/24739/#review22275

::: dom/push/PushService.jsm:292
(Diff revision 1)
> -              _ => this._unregisterIfConnected(record),
> +              record => this._unregisterIfConnected(record),

Name shadowing here.  Which reveals that you are unregistering a record that isn't deleted.  That's the right thing to do, and you were doing that before, but it looks like an accident. I think that this is cleaner, especially with the comment:

```
 this._db.getAllByOriginAttributes(originAttributes)
   .then(records => Promise.all(records.map(
           record => this._db.delete(record.keyID)
                       .catch(err => {
                         debug("webapps-clear-data:" + ...);
                         // note that this is the record we were unable to delete
                         return record;
                       })
                       .then(maybeDeleted => this._unregisterIfConnected(maybeDeleted))
        ))
   .catch(err => debug(...))
```

::: dom/push/PushService.jsm:1348
(Diff revision 1)
> +    let subscriptionChanges = [];
>      if (isAllow && isChange) {
>        // Permission set to "allow". Drop all expired registrations for this
>        // site, notify the associated service workers, and reset the quota
>        // for active registrations.
> -      return this._updateByPrincipal(
> +      updatePromise = this._updateByPrincipal(
>          permission.principal,
> -        record => this._permissionAllowed(record)
> +        record => this._permissionAllowed(record, subscriptionChanges)

I think that I would prefer to see this function take responsibility for pushing into `subscriptionChanges`.  Passing the array into `_permissionAllowed` just causes that function to have new side effects.

::: dom/push/PushService.jsm:1368
(Diff revision 1)
> -    return Promise.resolve();
> +    return updatePromise.then(() => {
> +      this.notifySubscriptionChanges(subscriptionChanges);
> +    });

the notify only gets called on the isAllow/isChange branch, why not move it there?

::: dom/push/PushService.jsm:1416
(Diff revision 1)
>        return null;
>      }
>      if (record.isExpired()) {
>        // If the registration has expired, drop and notify the worker
>        // unconditionally.
> -      this._notifySubscriptionChangeObservers(record);
> +      subscriptionChanges.push(record);
>        return false;
>      }
>      record.resetQuota();
>      return record;

The return values on this function are pretty screwy.  I think that 1421 should return `null`.

::: dom/push/test/xpcshell/test_drop_expired.js:53
(Diff revision 1)
> +    userAgentID,

Colon please.
Comment on attachment 8685150 [details]
MozReview Request: Bug 1223202 - Only send subscription change events if the Push permission is granted. r?mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24739/diff/1-2/
https://reviewboard.mozilla.org/r/24739/#review22275

> Name shadowing here.  Which reveals that you are unregistering a record that isn't deleted.  That's the right thing to do, and you were doing that before, but it looks like an accident. I think that this is cleaner, especially with the comment:
> 
> ```
>  this._db.getAllByOriginAttributes(originAttributes)
>    .then(records => Promise.all(records.map(
>            record => this._db.delete(record.keyID)
>                        .catch(err => {
>                          debug("webapps-clear-data:" + ...);
>                          // note that this is the record we were unable to delete
>                          return record;
>                        })
>                        .then(maybeDeleted => this._unregisterIfConnected(maybeDeleted))
>         ))
>    .catch(err => debug(...))
> ```

Good call. This needs to be rewritten for bug 1170115 anyway, but no reason we can't clean it up.

> The return values on this function are pretty screwy.  I think that 1421 should return `null`.

Please take a look at the latest patch. I refactored `updateByOrigin` into `reduceByOrigin` to remove the screwy return values. Does that seem sensible?
Attachment #8685150 - Flags: review+
Comment on attachment 8685150 [details]
MozReview Request: Bug 1223202 - Only send subscription change events if the Push permission is granted. r?mt

https://reviewboard.mozilla.org/r/24739/#review22395

::: dom/push/PushDB.jsm:224
(Diff revisions 1 - 2)
> -   * @param {Function} updateFunc A function that receives the existing
> -   *  registration record as its argument, and returns a new record. The record
> -   *  will not be updated if the function returns `null`, `undefined`, or an
> +   * @param {Function} iterator A function with the signature `(result,
> +   *  record, cursor)`, where `result` is the value returned by the previous
> +   *  invocation, `record` is the registration, and `cursor` is an `IDBCursor`.

What you describe is not an iterator.  It is, as you describe, a function that performs the reduce operation.

::: dom/push/PushDB.jsm:251
(Diff revisions 1 - 2)
> -            let newRecord = updateFunc(record);
> +            aTxn.result = iterator(aTxn.result, record, cursor);

I think that this function should be the one to call `cursor.continue()`.  I was initially uneasy about exposing the cursor, but I can see that you have made the code as a whole simpler a result.  No point worrying too much about that then.

::: dom/push/PushService.jsm
(Diff revisions 1 - 2)
> -

I'm a bit of a fan of whitespace between functions.

::: dom/push/PushService.jsm:1115
(Diff revisions 1 - 2)
>                return this.dropRegistrationAndNotifyApp(record.keyID).then(_ => null);

You don't need to adapt this value to null, now that you ignore the value

::: dom/push/PushService.jsm:1120
(Diff revisions 1 - 2)
> +        return record.toRegister();

Seen out of context, I realize that `toRegister()` is a poor name.  To `toPushSubscription()` perhaps.
https://reviewboard.mozilla.org/r/24739/#review22395

> Seen out of context, I realize that `toRegister()` is a poor name.  To `toPushSubscription()` perhaps.

I just realized we don't need `toRegister()` at all. We can just rename `toRegistration()` to `toSubscription()`. (How many more times can I use "to" in that sentence?)
Comment on attachment 8685150 [details]
MozReview Request: Bug 1223202 - Only send subscription change events if the Push permission is granted. r?mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24739/diff/2-3/
Attachment #8685150 - Flags: review?(martin.thomson)
Comment on attachment 8685150 [details]
MozReview Request: Bug 1223202 - Only send subscription change events if the Push permission is granted. r?mt

https://reviewboard.mozilla.org/r/24739/#review22405
Attachment #8685150 - Flags: review?(martin.thomson) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7347ad233dd011ea1381fd516b7afe5f40bdce0a
Bug 1223202 - Only send subscription change events if the Push permission is granted. r=mt
https://hg.mozilla.org/mozilla-central/rev/7347ad233dd0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Attached patch 4.patchSplinter Review
Please use the attached patch. A graft won't apply cleanly because it depends on the refactoring in bug 1210896, which doesn't really need to be uplifted.

Approval Request Comment
[Feature/regressing bug #]: Bug 1233387.
[User impact if declined]: A site will not be able to restore its push subscription if the user re-grants permission. Consequently, the user will never be able to receive push messages from that site.
[Describe test coverage new/current, TreeHerder]: This patch updates existing xpcshell tests, and adds a new one for the correct behavior.
[Risks and why]: Medium risk, same as bug 1219063.
[String/UUID change made/needed]: None.
Attachment #8685150 - Attachment is obsolete: true
Attachment #8700921 - Flags: review+
Attachment #8700921 - Flags: approval-mozilla-beta?
Comment on attachment 8700921 [details] [diff] [review]
4.patch

From the user impact, this sounds like a mainline scenario we should fix in FF44. This fix has been in Nightly for almost 6 weeks, seems safe to uplift to Beta44.
Attachment #8700921 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm hitting conflicts uplifting this to beta. Can we get a rebased patch if this needs to get to beta?
Flags: needinfo?(kcambridge)
Odd, this applied cleanly after rebasing. I wonder if I had a newer patch in my local tree. Pushed in https://hg.mozilla.org/releases/mozilla-beta/rev/c0b161f60c6b. Thanks, Wes and Ritu!
Flags: needinfo?(kcambridge)
Verified Beta 44.0b4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.