1207744 - unregisters lost on reconnect

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Ben Bangert [:benbangert]]

Related to bug: 1207743, unregister's in the _queue will also be lost if the connection drops.

The client will not run notifications for channels it no longer thinks its registered for, however this will continue to eat bandwidth and server-side storage space handling messages that the client tried to unregister for.

A possible solution might be to send unregister's if a notification is received that the client no longer believes it should have received.

Comment 1

[Dragana Damjanovic [:dragana]]

This is relevant for WebSocket version and it is easy to implement - just need to call this._queueRequest({channelId: channelId, messageType: "unregister"});

http2 does not need this because on unregister succeeds when the server receives the unregister request.

Comment 2

[Lina Butler (ex-Mozilla)]

Attachment: durableUnregister.patch

This patch adds "unregister" requests to the queue (and times them out) for Web Push. For Simple Push, it uses the old behavior and returns immediately.

Comment 3

[Lina Butler (ex-Mozilla)]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e1ab1bd99c
Track and re-send push unregister requests on reconnect. r=dragana

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fb88074a1f
Back out changeset f7e1ab1bd99c for X(3) failures.

Comment 6

[Lina Butler (ex-Mozilla)]

So, this fix is more complicated than I thought. Backed out because I pushed the wrong changeset to try, which would have caught this. Writing up some notes for debugging later.

1. `_queueRequest` shouldn't queue "unregister" requests if `this._dataEnabled`. Otherwise, they'll get sent twice: once when the enqueued `this._send(data)` is called, and a second time when `_sendPendingRequests` is called after the server says hello.

2. I think `_send` needs to check if the request is still in `_pendingRequests`, so that it doesn't try to resend timed-out requests. But maybe that check isn't necessary if we don't enqueue requests? (Since we always keep them in the map).

3. If (2) is restored, `_sendRequest` needs to call `this._pendingRequests.set` before `this._queueRequest`. Otherwise, _send will think the request has timed out, when it just hasn't been added yet.

Comment 7

[Lina Butler (ex-Mozilla)]

Attachment: durableUnregister.patch

Another issue: we queue unregisters conditionally, but we don't know if data is enabled until after we finish connecting.

Realistically, I think we can always wait for unregister replies, regardless of whether the server we're talking to is Web or Simple Push. That simplifies the logic a bit. I also added some comments to _queueRequest and _send.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1223a519bdf
Track and re-send push unregister requests on reconnect. r=dragana

Comment 9

[Carsten Book [:Tomcat]]

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