1223202 - Only send subscription change events if the Push permission is granted

Status: VERIFIED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

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.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1223202 - Only send subscription change events if the Push permission is granted. r?mt

Bug 1223202 - Only send subscription change events if the Push permission is granted. r?mt

Comment 2

[Martin Thomson [:mt:]]

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 3

[Lina Butler [:lina]]

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/

Comment 4

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38a16da7dd8b

Comment 5

[Lina Butler [:lina]]

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?

Comment 6

[Martin Thomson [:mt:]]

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.

Comment 7

[Lina Butler [:lina]]

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 8

[Lina Butler [:lina]]

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/

Comment 9

[Martin Thomson [:mt:]]

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

Comment 10

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7347ad233dd011ea1381fd516b7afe5f40bdce0a
Bug 1223202 - Only send subscription change events if the Push permission is granted. r=mt

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7347ad233dd0

Comment 12

[Lina Butler [:lina]]

Attachment: 4.patch

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.

Comment 13

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

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.

Comment 14

[Lina Butler [:lina]]

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

Comment 15

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

I'm hitting conflicts uplifting this to beta. Can we get a rebased patch if this needs to get to beta?

Comment 16

[Lina Butler [:lina]]

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!

Comment 17

[Carsten Book [:Tomcat]]

setting flags

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c0b161f60c6b

Comment 19

[Michelle Funches - QA]

Verified Beta 44.0b4