unregisters lost on reconnect

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: benbangert, Assigned: lina)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

4 years ago
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

Updated

3 years ago
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Posted 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)
Assignee

Updated

3 years ago
Whiteboard: btpp-active
Attachment #8760529 - Flags: review?(dd.mozilla) → review+

Comment 4

3 years ago
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
Assignee

Updated

3 years ago
Attachment #8760529 - Flags: feedback?(bbangert)

Comment 5

3 years ago
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+

Comment 8

3 years ago
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

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e1223a519bdf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.