Provide per-websocket way to enable keepalive pings

RESOLVED FIXED in mozilla22

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
mozilla22
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)

Updated

5 years ago
Assignee: nobody → jduell.mcbugs
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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).
Blocks: 822712
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: 822712
(Assignee)

Comment 7

5 years ago
Created attachment 725269 [details] [diff] [review]
v1

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.

   https://tbpl.mozilla.org/?tree=Try&rev=9a67e3e6eb03
Attachment #725269 - Flags: review?(mcmanus)
Comment on attachment 725269 [details] [diff] [review]
v1

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+
(Assignee)

Comment 9

5 years ago
Created attachment 730862 [details] [diff] [review]
v2: only allow setting value before AsyncOpen

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+
(Assignee)

Comment 10

5 years ago
thanks patrick!

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f264e8f22cf
Blocks: 822712
No longer blocks: 763198
(Assignee)

Updated

5 years ago
No longer blocks: 822712
Blocks: 822712
Removing 'blocking 822712' because this doesn't affect the correctness of push in anyway. Filed follow up 855906
No longer blocks: 822712
https://hg.mozilla.org/mozilla-central/rev/6f264e8f22cf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Assignee)

Updated

5 years ago
Blocks: 855906
(Assignee)

Comment 13

5 years ago
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: ? → ---

Updated

5 years ago
Blocks: 870579
You need to log in before you can comment on or make changes to this bug.