1185544 - Add data delivery to the WebSocket backend

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

Once we support data, we'll want to send a special flag in the Simple Push `hello` message. This will indicate that the Push server should use Web Push semantics for notifications. In particular:

* Push endpoints will treat incoming bodies as opaque (Simple Push assumes a URL-encoded body with an optional `version` param).
* Individual messages will be stored for delivery if the device is offline.
* Incoming `notification` messages will include a `data` field instead of `version`. We'll want to modify `PushServiceWebSocket.jsm` to detect and decrypt this field.

https://github.com/mozilla-services/autopush/issues/57 tracks the server work to support this.

Comment 1

[Lina Butler [:lina]]

Attachment: 0001-Bug-1185544-Add-data-delivery-to-the-WebSocket-backe.patch

Comment 2

[Dragana Damjanovic [:dragana]]

Comment on attachment 8654269 [details] [diff] [review]
0001-Bug-1185544-Add-data-delivery-to-the-WebSocket-backe.patch

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

I have one question: if registration happens during data.enable turned off, it will never be able to receive message even if pref flips, it needs to reregister... we do not have any notification that subscription needs to be updated" except checking pref.

::: dom/push/PushService.jsm
@@ +27,5 @@
>  Cu.import("resource://gre/modules/Promise.jsm");
>  
>  const {PushServiceWebSocket} = Cu.import("resource://gre/modules/PushServiceWebSocket.jsm");
>  const {PushServiceHttp2} = Cu.import("resource://gre/modules/PushServiceHttp2.jsm");
> +const {PushServiceHttp2Crypto} = Cu.import("resource://gre/modules/PushServiceHttp2Crypto.jsm");

we should rename this to PushServiceCrypto

::: modules/libpref/init/all.js
@@ +4477,5 @@
>  // This preference should be used in UX to enable/disable push.
>  pref("dom.push.connection.enabled", true);
>  
> +// Enable data delivery over the WebSocket connection.
> +pref("dom.push.data.enabled", false);

maybe add ws, something like dom.push.ws.data.enable to be clear that it is only ws

Comment 3

[Lina Butler [:lina]]

(In reply to Dragana Damjanovic [:dragana] from comment #2)
> I have one question: if registration happens during data.enable turned off,
> it will never be able to receive message even if pref flips, it needs to
> reregister... we do not have any notification that subscription needs to be
> updated" except checking pref.

Good catch! I should change this to match `PushServiceHttp2`, so we'll drop and recreate all subscriptions without keys if data is enabled.

Comment 4

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana,nsm

Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8661441 [details]
MozReview Request: Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana,nsm

Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8661441 [details]
MozReview Request: Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana,nsm

Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana,nsm

Comment 7

[Lina Butler [:lina]]

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

Comment 8

[Dragana Damjanovic [:dragana]]

Comment on attachment 8661441 [details]
MozReview Request: Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana,nsm

https://reviewboard.mozilla.org/r/19351/#review17555

Looks good, just few comments

::: dom/push/PushService.jsm:728
(Diff revision 3)
> +    // is only going to happen on db upgrade from version 4 to higher.

This is only true for http2. you should change db version for WS.

::: dom/push/PushService.jsm:797
(Diff revision 3)
> +      if (cryptoParams) {

This is only false when enableData is false?

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8661441 [details]
MozReview Request: Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana,nsm

https://reviewboard.mozilla.org/r/19351/#review17583

Great!

Comment 10

[Lina Butler [:lina]]

(In reply to Dragana Damjanovic [:dragana] from comment #8)
> This is only false when enableData is false?

Yup, that's one case (I removed `dom.push.ws.data.enabled`, but the server can still opt out of data by omitting `use_webpush` in the handshake). The second case is if you send a message without crypto headers or a body.

Comment 11

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a7044bb280b26897746b69829ea230e2e5ffec
Bug 1185544 - Add data delivery to the WebSocket backend. r=dragana,nsm

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to Kit Cambridge [:kitcambridge] from comment #10)
> (In reply to Dragana Damjanovic [:dragana] from comment #8)
> > This is only false when enableData is false?
> 
> Yup, that's one case (I removed `dom.push.ws.data.enabled`, but the server
> can still opt out of data by omitting `use_webpush` in the handshake). The
> second case is if you send a message without crypto headers or a body.

If someone sends message without a body we do not need crypto part as well, I agree  with that.

Just a thought: what about just dropping a message that has a body, data delivery is enabled, but there is no crypto headers? Just to discourage such a behavior?

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d0a7044bb280