Closed Bug 1207744 Opened 5 years ago Closed 4 years ago

unregisters lost on reconnect

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: benbangert, Assigned: Lina)

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Attached patch durableUnregister.patch (obsolete) — Splinter Review
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.
Attachment #8760529 - Flags: review?(dd.mozilla)
Attachment #8760529 - Flags: feedback?(bbangert)
Whiteboard: btpp-active
Attachment #8760529 - Flags: review?(dd.mozilla) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e1ab1bd99c
Track and re-send push unregister requests on reconnect. r=dragana
Attachment #8760529 - Flags: feedback?(bbangert)
Backout by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fb88074a1f
Back out changeset f7e1ab1bd99c for X(3) failures.
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.
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
Attachment #8760529 - Attachment is obsolete: true
Attachment #8762701 - Flags: review?(dd.mozilla)
Attachment #8762701 - Flags: review?(dd.mozilla) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1223a519bdf
Track and re-send push unregister requests on reconnect. r=dragana
https://hg.mozilla.org/mozilla-central/rev/e1223a519bdf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.