Closed Bug 1185544 Opened 4 years ago Closed 4 years ago

Add data delivery to the WebSocket backend

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(2 files)

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.
Depends on: 1172502
Assignee: nobody → kcambridge
Summary: WebSocket backend: Send a "data delivery" flag in the opening handshake → Add data delivery to the WebSocket backend
Blocks: 1149195
Attachment #8654269 - Flags: feedback?(dd.mozilla)
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
Attachment #8654269 - Flags: feedback?(dd.mozilla) → feedback+
(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.
Status: NEW → ASSIGNED
Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana
Attachment #8661441 - Flags: review?(dd.mozilla)
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
Blocks: 1205109
Blocks: 1205112
Blocks: 1205137
Attachment #8661441 - Attachment description: MozReview Request: Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana → MozReview Request: Bug 1185544 - Add data delivery to the WebSocket backend. r?dragana,nsm
Attachment #8661441 - Flags: review?(nsm.nikhil)
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 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?
Attachment #8661441 - Flags: review?(dd.mozilla) → review+
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!
(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.
(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?
https://hg.mozilla.org/mozilla-central/rev/d0a7044bb280
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.