Closed Bug 1201128 Opened 4 years ago Closed 4 years ago

Don't send channel IDs in the Push handshake

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
blocking-b2g 2.5?
Tracking Status
firefox44 --- fixed
b2g-v2.2r --- fixed

People

(Reporter: jrconlin, Assigned: Lina)

References

Details

Attachments

(2 files)

The SimplePush client currently sends a list of known ClientIDs as part of the "Hello" packet. The client IDs are ignored by the server, seldom purged by the client, and can cause buffer rejection issues if there are a large number. It is requested that the client send an empty set of ClientIDs instead.

NOTE: This fix requires that the pending TEF server shutdown and traffic shift be completed. This is in negotiation and Sept. 21, 2015 is proposed as a candidate date, but this has not been finalized or agreed to.

NOTE: This fix need only be for clients using SimplePush and not the upcoming WebPush protocol. If WebPush is accelerated or the specification completed, this bug may be closed as WONTFIX.
[Blocking Requested - why for this release]: Nominating for blocking, as this can cause bug 1189729, which causes both data usage and battery run-down. We should make sure to keep track of this.
blocking-b2g: --- → 2.5?
Bug 1201128 - Don't send channel IDs in the WebSocket handshake. r?nsm
Attachment #8656065 - Flags: review?(nsm.nikhil)
Comment on attachment 8656065 [details]
MozReview Request: Bug 1201128 - Don't send channel IDs in the Push handshake. r?nsm

Bug 1201128 - Don't send channel IDs in the Push handshake. r?nsm
Attachment #8656065 - Attachment description: MozReview Request: Bug 1201128 - Don't send channel IDs in the WebSocket handshake. r?nsm → MozReview Request: Bug 1201128 - Don't send channel IDs in the Push handshake. r?nsm
OK, I've removed `channelIDs` from Simple Push and the WebSocket backend for Web Push.
Comment on attachment 8656065 [details]
MozReview Request: Bug 1201128 - Don't send channel IDs in the Push handshake. r?nsm

https://reviewboard.mozilla.org/r/18077/#review16211
Attachment #8656065 - Flags: review?(nsm.nikhil) → review+
Removed 2.5 nomination.
blocking-b2g: 2.5? → ---
Assignee: nobody → kcambridge
Duplicate of this bug: 1201138
Duplicate of this bug: 1217800
n?mahe - Why has blocking 2.5 been denied/removed? Surely if we ship 2.5 without this, we'll end up with lots of people wasting data and battery life? (sorry if I've misunderstood the consequence of this bug)
Flags: needinfo?(mpotharaju)
Hello Chris, 

The bug seemed to be for Hello feature on desktop and not specific to phone. 

It seems to be fixed now and will land for 2.5. 

Thanks for the NI Chris.
Flags: needinfo?(mpotharaju)
[Blocking Requested - why for this release]: Same reason as comment #1.

Note, I don't think this bug has anything to do with the Hello feature on desktop, as I read it, it's to do with the 'Hello' packet for communicating with the push notification server. This bug, as I understand (and no one has refuted thus far) has serious repurcussions for FxOS devices, and regardless of it being fixed in time, we should make sure to track it correctly so that it doesn't slip or regress in 2.5.
blocking-b2g: --- → 2.5?
(In reply to Chris Lord [:cwiiis] from comment #13)
> [Blocking Requested - why for this release]: Same reason as comment #1.
> 
> Note, I don't think this bug has anything to do with the Hello feature on
> desktop, as I read it, it's to do with the 'Hello' packet for communicating
> with the push notification server.

That's correct. Sorry for the confusion; the "hello" refers to the protocol handshake, not Firefox Hello.

> This bug, as I understand (and no one has refuted thus far) has serious
> repurcussions for FxOS devices, and regardless of it being fixed in time,
> we should make sure to track it correctly so that it doesn't slip or
> regress in 2.5.

Agreed. We definitely want the fix in 2.5.
Summary: Remove list of ClientIDs from Hello packet. → Don't send channel IDs in the Push handshake
https://hg.mozilla.org/mozilla-central/rev/567711c16ae8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
I guess we will want to uplift this on 2.2 and 2.2r at least.
Flags: needinfo?(kcambridge)
Sure, that's fine. The Push code changed quite a bit between 2.2 and tip, so we'll need a separate uplift patch. What are the correct branches for 2.2 and 2.2r?
Flags: needinfo?(kcambridge) → needinfo?(lissyx+mozillians)
b2g37_v2.2 and b2g37_v2.2r I think?
Flags: needinfo?(lissyx+mozillians)
Attached patch 1201128.patchSplinter Review
Rebased against 2.2 and 2.2r.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1201138, bug 1217800.
User impact if declined: Continued use of apps that depend on the Push API (e.g., Find My Device) will eventually exceed the maximum Push channel limit, causing a TCP reset on reconnect. The Push client will then try to reconnect indefinitely, resulting in battery drain and excessive data usage.
Testing completed: The patch on `mozilla-central` updates the xpcshell tests. No test changes in the rebased patch, since the Push code in 2.2 is untested.
Risk to taking this patch (and alternatives if risky): Omitting the channel IDs in the handshake may cause compatibility issues with third-party Push servers. Does FxOS use any other Push services, apart from wss://push.services.mozilla.com and wss://ua.push.tefdigital.com?
String or UUID changes made by this patch: None
Attachment #8679554 - Flags: approval‑mozilla‑b2g37_v2_2r?
Attachment #8679554 - Flags: approval-mozilla-b2g37?
Comment on attachment 8679554 [details] [diff] [review]
1201128.patch

Approved to land ONLY on 2.2R

QC has completed CS on 2.2, and we are not landing any new patches on 2.2 unless they are sec critical or high
Attachment #8679554 - Flags: approval‑mozilla‑b2g37_v2_2r? → approval‑mozilla‑b2g37_v2_2r+
Thanks Kit and Chris for correcting me. I blocked this for 2.5+, and thanks for landing it.
Attachment #8679554 - Flags: approval-mozilla-b2g37?
You need to log in before you can comment on or make changes to this bug.