Rather than wait for TCP to timeout on error, which can be upto 30 minutes long, use the WebSocket protocol's ping/pong mechanism to detect connection loss. ref bug 849364, WebSocketChannel exposes pingInterval and pingTimeout.

We're not using TCP keepalive (which only checks every 2 hours IIRC), so if we're not sending data through the websocket we may not ever get notification of the TCP socket going down. Is that a problem? I assume so. Setting pingInterval is a one-line fix so we may want to do that soon? If you give me the interval you want to use I can write a patch.

55s sounds good.

You asked for it :)

ms seems like a footgun--it's hard to think of a valid use case for sub-second ping intervals. The prefs.js pref is already using seconds.

A ping every 55 sec strikes me as power-munchy, but dougt knows the UDP wakeup stuff, so maybe it's fine in the B2G case. Note that we're apparently planning to use this in Android too--I very much doubt we'll want ping-per-minute there, so do we need a followup to set this differently depending on #if B2G ?

Comment on attachment 733137 [details] [diff] [review] part2: use ping interval of 55 sec for Update notification websocket Review of attachment 733137 [details] [diff] [review]: ----------------------------------------------------------------- nsm - please create a follow up bug to investigate using a variable interval based on if the network drops or not (details can put put in the new bug)

Comment on attachment 733134 [details] [diff] [review] part1: convert pingInterval to use seconds, not ms Review of attachment 733134 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/websocket/BaseWebSocketChannel.cpp @@ +125,2 @@ > { > + *aSeconds = mPingInterval ? mPingInterval / 1000 : 0; just mPingInterval / 1000 right? (i.e. you don't need the conditional) @@ +145,2 @@ > { > + *aSeconds = mPingResponseTimeout ? mPingResponseTimeout / 1000 : 0; just mPingResponseTimeout / 1000, right?

Comment on attachment 733137 [details] [diff] [review] part2: use ping interval of 55 sec for Update notification websocket Review of attachment 733137 [details] [diff] [review]: ----------------------------------------------------------------- I remain concerned about battery and bandwidth issues here. But lets go ahead and see who is right.

Comment on attachment 733137 [details] [diff] [review] part2: use ping interval of 55 sec for Update notification websocket Review of attachment 733137 [details] [diff] [review]: ----------------------------------------------------------------- Just a magic number issue. If you don't mind, I can just fix it up and land it. I'll file a follow up bug for intelligent ack and some other approaches. Appropriate intervals seems to be something for Tef/Mozilla people with actual hardware on their hands to find out. ::: dom/push/src/PushService.js @@ +537,5 @@ > > debug("serverURL: " + uri.spec); > this._wsListener = new PushWebSocketListener(this); > this._ws.protocol = "push-notification"; > + this._ws.pingInterval = 55; this._ws.pingInterval = this._prefs.get("websocketPingInterval"); ? You'll want to add services.push.websocketPingInterval to b2g/app/b2g.js with the other push prefs.

> If you don't mind, I can just fix it up and land it. all yours!

This just removes ternary operator per PAtrick's observation in comment 7. Carrying forward his +r.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5ef06d13fa https://hg.mozilla.org/integration/mozilla-inbound/rev/9509f6be0095

Nihil: is this going to need to get uplifted? Will b2g18 deal with detecting dropped websockets w/o this?

w/o this, dropped sockets will take a long time to be detected unless the network interface state changes. I'd like it to get uplifted. I usually wait for patches to land on m-c before marking them as tracking b2g18.

https://hg.mozilla.org/mozilla-central/rev/5f5ef06d13fa https://hg.mozilla.org/mozilla-central/rev/9509f6be0095

requesting tracking per comment 13 and 14

@jduell: Thanks for the patches, but some new issues raised at the Madrid work week mean that we'll have to disable this and use a application level ping pong. Turns out nsITimer doesn't trigger when b2g devices are in low power mode (screen off, not used for a few minutes). The solution is to use alarms which is backed by the RTC. Except the alarm API is inaccessible from C++ (current also from JS Gecko code) since there can only be *one* watcher for the RTC timeout event, and that watcher is the Alarms API AlarmService. See bug 862322 for more info. The patch is still useful on desktop.

Is tracking-b2g? still required considering comment 17? I think not. AFAIK right now push was the only use of pingInterval on b2g and possibly the only use of websocket outside of apps. Apps don't have access to pingInterval anyway.

Reflected the fact that SimplePush no longer relies on this feature. @Chris - Does Android still block on this?

(In reply to Nikhil Marathe from comment #19) > @Chris - Does Android still block on this? Nope. Thanks for the heads up.