Closed Bug 849364 Opened 7 years ago Closed 7 years ago

Provide per-websocket way to enable keepalive pings


(Core :: Networking: WebSockets, defect)

Not set





(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)




(1 file, 1 obsolete file)

websocket pings are turned off by default (we were worried they consume too much power--ie. wake up the phone and/or radio when it would otherwise be asleep.  It's not actually clear if that's the case on android or B2G).  But in any case bug 763198 is simplified by turning keepalive on for its websocket connection, so this will provide a way to do that per-websocket.
Assignee: nobody → jduell.mcbugs
Blocks: 763198
to be clear the proposal is a method on nsIWebsocketChannel, yes?

I think that's fine. I wouldn't want to see us go and making javascript apis without jonas defining it.

but we've got data that shows 60 seconds as the most common NAT timeout period. I think if you put an open ended 55 second heartbeat on b2g you will just crush the battery and consume massive amounts of data. That's likely a dead end.
Isn't it 30 seconds?  According the SPDY_PING_EXPERIMENT_FAIL probe.
(In reply to Honza Bambas (:mayhemer) from comment #2)
> Isn't it 30 seconds?  According the SPDY_PING_EXPERIMENT_FAIL probe.

I'm pretty sure it was 60. We've removed that telem a while ago, so any data points now on the dashboard are stragglers from a few old browsers without much representation, right?

There definitely was a lot of variation though.

in any event - 30 would be even twice as bad for the purposes of this bug.
Metrics doesn't say how many submissions for the one probe I want to examine has been submitted for a date.  Or I don't know how to get that info.
Patrick, you might be right, but from the IRC chat I posted in bug 849377 I couldn't get a clear answer as to how bad the power issue is.  And there were noises about doing the pings less frequently when the screen is off.  Though if that just results in the websocket connection being killed (and the app woken up with onstop() and then just reopening a new WS, then it's not a battery win.

Would we be better off requiring the app to do the ping here?  That way it would presumably only happen when the app is awake (not sure when that is for B2G push--it's part of firefox OS, not a regular app).
B2G push always keep a websocket awake. The only time the socket is off is if nobody has signed up for Push.
The 'optimization' in case of certain carriers is that they can wake up the service over UDP, in which case the WebSocket is switched off. But over wi-fi, always on.
No longer blocks: simple-push-b2g
Attached patch v1 (obsolete) — Splinter Review
Even simple stuff gets complicated with e10s :)

Do we need to clamp() SetPingTimeout values as we do when we read them from prefs?  I wasn't clear on why we're clamping to that particular range.
Attachment #725269 - Flags: review?(mcmanus)
Comment on attachment 725269 [details] [diff] [review]

Review of attachment 725269 [details] [diff] [review]:

Sorry this is so much code for something that is already implemented :(

The clamp is probably ok to skip from chrome.
Attachment #725269 - Flags: review?(mcmanus) → review+
Testing v1 exposed race conditions.  Checked and we can get away with simple fix--just allow setting ping timeout values *before* asyncOpen is called.

Tested by hand the 4-space of (e10s or not, call before/after asyncOpen).  Automated test not possible w/current websockets test infrastructure.
Attachment #725269 - Attachment is obsolete: true
Attachment #730862 - Flags: review?(mcmanus)
Attachment #730862 - Flags: review?(mcmanus) → review+
No longer blocks: simple-push-b2g
Removing 'blocking 822712' because this doesn't affect the correctness of push in anyway. Filed follow up 855906
No longer blocks: simple-push-b2g
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 855906
we only need to track this if we track bug 855906
tracking-b2g18: --- → ?
since we did not track bug 855906, no need to track here.
tracking-b2g18: ? → ---
Blocks: 870579
You need to log in before you can comment on or make changes to this bug.