Paul, how much of this is covered by 1002414? is this entirely covered in it?

As of 1002414, the only thing that the SimplePushServer ping would catch that the reconnect code would not is the PushServer becoming unresponsive without the websocket connection going down. Any network loss of connectivity is covered.

Mark -- Do we need this for release or could we go to release with this?

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #3) > Mark -- Do we need this for release or could we go to release with this? Whilst I'm inclined to agree with Paul, having used websockets on Talkilla, I'm not convinced that all network connectivity issues are covered/discovered with the websockets without extra pings. However, I don't have hard evidence here, so I'd say we probably put this on a back-burner, and fix it if we start seeing issues with it.

(In reply to Mark Banner (:standard8) from comment #4) > (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #3) > > Mark -- Do we need this for release or could we go to release with this? > > Whilst I'm inclined to agree with Paul, having used websockets on Talkilla, > I'm not convinced that all network connectivity issues are > covered/discovered with the websockets without extra pings. > > However, I don't have hard evidence here, so I'd say we probably put this on > a back-burner, and fix it if we start seeing issues with it. The issue with TCP is that a network connection can silently go away. Unless the client tries to send traffic, it can go hours, days, or weeks without finding out about the failure. This is exacerbated by the fact that the vast majority of our users will be behind NATs, and consumer NATs tend to time-out TCP connection bindings with timers that typically range from one to 24 hours. So, to use the example I have at hand: AT&T's Uverse service uses 2Wire gateways for the vast majority of its residential subscribers. The default TCP binding timeout is 24 hours. Without pings, we'll discover that most AT&T Uverse subscribers will mysteriously stop receiving calls after their browser has been running for one day. So, while this isn't the number one priority, I'd say it rates much higher than "back burner".

(In reply to Adam Roach [:abr] from comment #5) > So, to use the example I have at hand: AT&T's Uverse service uses 2Wire > gateways for the vast majority of its residential subscribers. The default > TCP binding timeout is 24 hours. Without pings, we'll discover that most > AT&T Uverse subscribers will mysteriously stop receiving calls after their > browser has been running for one day. To be clear, I'm not calling out AT&T here; I just happen to be a subscriber, so it was easy for me to check the actual timeout value. Every residential service [1] will have this problem. The only thing that's going to change is whether the time-to-fail is 24 hours or something else. ____ [1] There are probably exceptions here, but not within the first several sigmas of services.

Fixing the title (s/second/minute/) -- when I saw this go by in my inbox I nearly had a heart attack.

Default 30 minute ping with a 10 second respose timeout. Configurable via prefs.

Fix handling of empty ping response message.

Updated xpcshell test.

Comment on attachment 8471750 [details] [diff] [review] Part 1: Push server 30 minute ping The general idea looks fine to me but I haven't got time to review this before I go away. Passing across and seeing if Tim will review it.

Moving this to P1. We want to make sure this gets reviewed and landed before fx34 uplifts.

Fixed bit-rot in firefox.js hunk

Comment on attachment 8479193 [details] [diff] [review] Part 1: Push server 30 minute ping Review of attachment 8479193 [details] [diff] [review]: ----------------------------------------------------------------- Nit: Can you please fix your editor settings to not automatically remove trailing white spaces on lines you don't touch? I was confused for a moment about which prefs you changed in firefox.js. The whole state machine as implemented by the PushHandler seems very fragile. Implementing protocols is complicated and if we want to harden against servers that don't respond or send wrong responses we should do it right. Maybe instead of keeping track of the state of the connection we should be keeping track of where we are in the protocol? That would make it easier to take the appropriate action in onMessageAvailable() if an unexpected message arrives or we time out waiting for one. The current back-off mechanism doesn't seem to ever back off. All we do is retry in fixed delays that don't increase with the number of retries. ::: browser/app/profile/firefox.js @@ +1585,5 @@ > pref("loop.do_not_disturb", false); > pref("loop.ringtone", "chrome://browser/content/loop/shared/sounds/Firefox-Long.ogg"); > pref("loop.retry_delay.start", 60000); > pref("loop.retry_delay.limit", 300000); > +pref("loop.ping.wait", 1800000); "loop.ping.interval" maybe? Wait makes me ask what we're waiting for. ::: browser/components/loop/MozLoopPushHandler.jsm @@ +14,5 @@ > > XPCOMUtils.defineLazyModuleGetter(this, "console", > "resource://gre/modules/devtools/Console.jsm"); > > +let connectionStates = { We should do it like: const CONNECTION_STATE_SHUTDOWN = 0; const CONNECTION_STATE_CONNECTING = 1; ... @@ +63,5 @@ > + return Services.prefs.getIntPref("loop.ping.wait") > + } > + catch (e) { > + return 18000000 // 30 minutes > + } Hmm, that seems overly defensive to me. But ok, looks like we have that in the file anyway above so we can keep it. Usually, if we provide a pref with a default value we don't try to catch when people remove/empty it. Reading prefs is cheap so we might also just do it when we need the value as long as we don't do it in a hot loop. @@ +104,5 @@ > } > > this._registerCallback = registerCallback; > this._notificationCallback = notificationCallback; > + this.connectionIs = connectionStates.closed; Why are we changing the initial value to CLOSED here? Can't we just start off with CLOSED on line 40? @@ +136,5 @@ > + this.pushUrl = undefined; > + this.registered = false; > + this._registerCallback = undefined; > + this._notificationCallback = undefined; > + this._mockPushHandler = undefined; Clearing out all values is overkill. We usually only clear out some of the properties to break CC cycles and let objects be GC'ed. @@ +152,5 @@ > // and send the uaID in order to re-synch with the > // PushServer. If a registration has been completed, send the channelID. > let helloMsg = { messageType: "hello", > uaid: this.uaID, > + channelIDs: this.registered ? [this.channelID] : [] }; While you're at it, please fix the indentation for the two lines, they use tabs for some reason. @@ +159,5 @@ > + * be closed and another socket openned in order to re-attempt the handshake. > + * > + * NOTE: Set the retry callback BEFORE the operation that you want to retry. > + * The callback may occur before the code after the sendMsg() can run, > + * especially when running tests with a mock websocket handler. Do you want to say that the retry callback could be called synchronously? Don't we have a minimum delay? Not sure I follow this comment. @@ +161,5 @@ > + * NOTE: Set the retry callback BEFORE the operation that you want to retry. > + * The callback may occur before the code after the sendMsg() can run, > + * especially when running tests with a mock websocket handler. > + */ > + this._retryOperation(() => this._restartConnection(), this._maxRetryDelay_ms); Using retryOperation() seems like the wrong thing to do here. We don't need any retry/back-off scheme here. We want to call _restartConnection() a single time when the handshake fails. It also seems like onMessageAvailable() should reject any other type of message while e didn't receive the handshake yet. Another point is that if the handshake keeps failing we don't ever seem to back off. @@ +207,5 @@ > let msg = JSON.parse(aMsg); > > + if (!msg.messageType) { > + // Treat as a ping response > + this._pingRestartWait(); Isn't this handled by the default case below already? @@ +275,5 @@ > // For tests, use the mock instance. > this._websocket = this._mockPushHandler; > } else if (!Services.io.offline) { > this._websocket = Cc["@mozilla.org/network/protocol;1?name=wss"] > + .createInstance(Ci.nsIWebSocketChannel); Would have been more inline with current practices to either indent it by two more spaces than the original version or just leave it as is :) @@ +280,4 @@ > } else { > this._registerCallback("offline"); > console.warn("MozLoopPushHandler - IO offline"); > + return; // Do not continue - return error. Do we really need that comment? Seems rather obvious, no? @@ +318,5 @@ > + break; > + > + default: > + this._retryEnd(); > + this._registerCallback("error: PushServer registration failure, status = " + msg.status); So if the servers responds with an invalid register message, what are we supposed to do? Re-connect? Seems like this would leave us in "some state". @@ +334,5 @@ > + this._websocket.close(4001, "Response timeout: re-establishing connection to PushServer"); > + } > + catch (e) {} > + console.warn("MozLoopPushHandler - Response timeout: re-establishing connection to PushServer"); > + // This should trigger an OnStop() callback. This comment would be better off next to the websocket.close() call because that's what calls the onStop() callback. @@ +343,5 @@ > * Handles registering a service > */ > _registerChannel: function() { > this.registered = false; > + this._retryOperation(() => this._registerChannel()); If I understand the protocol correctly, we shouldn't accept any message other than "register" as long as we're not registered, right? @@ +377,5 @@ > + if (this._retryCount) { > + clearTimeout(this._timeoutID); > + } > + > + if (!this._retryCount || retryDelay) { It seems weird that we cancel the current retry operation when an optional parameter is given that is supposed to specify the delay. If we want to enforce restarting and cancelling the current retry operation we should probably just call _retryEnd() and then _retryOperation()? @@ +383,5 @@ > this._retryCount = 1; > } else { > let nextDelay = this._retryDelay * 2; > this._retryDelay = nextDelay > this._maxRetryDelay_ms ? this._maxRetryDelay_ms : nextDelay; > this._retryCount += 1; I see that we are increasing _retryCount but why do we do that? Wouldn't we need to let that somehow affect the delay? And when do we back off? After a max. number of retries? @@ +397,5 @@ > if (this._retryCount) { > clearTimeout(this._timeoutID); > this._retryCount = 0; > } > + }, That change seems unnecessary.

Comment on attachment 8471752 [details] [diff] [review] Part 2: xpcshell test updated with ping/restore Cancelling review until the push handler is sorted out.

I noticed that the in the simple PushServer specification: https://wiki.mozilla.org/WebAPI/SimplePush/Protocol#PushServer_-.3E_UserAgent_5, that the UserAgent is supposed to send an ack back after receiving a notification. I do not believe that MozLoopPushHandler has ever done this. Is this a potential problem? Should I add this acknowledge operation to the module?

Moved webSocket callbacks to a intermediary object to eliminate getting callbacks from closed or failed websockets. Fixed callback timing logic.

I did go ahead and add the notification ack to the PushServer user agent. This does not seem to effect the loop server in a negative way.

> @@ +318,5 @@ >> + break; >> + >> + default: >> + this._retryEnd(); >> + this._registerCallback("error: PushServer registration failure, status = " + msg.status); > > So if the servers responds with an invalid register message, what are we supposed to do? Re-connect? Seems like this would leave us in "some state". For these cases, a reconnection attempt should not be made. I have changed to code to put the UA back into an initial state so that the application can call initialize() again as some later time.

Fix for some nits.

(In reply to Paul Kerr [:pkerr] from comment #16) > I noticed that the in the simple PushServer specification: > https://wiki.mozilla.org/WebAPI/SimplePush/Protocol#PushServer_-. > 3E_UserAgent_5, that the UserAgent is supposed to send an ack back after > receiving a notification. I do not believe that MozLoopPushHandler has ever > done this. Is this a potential problem? Should I add this acknowledge > operation to the module? I've looked and it before and I believe it isn't an issue for us - we don't rely on the functionality this gives.

Comment on attachment 8471752 [details] [diff] [review] Part 2: xpcshell test updated with ping/restore Review cancelled due to bit rot introduced by bug 1055139. I will need to merge in the code that queries the loop server for the PushServer URI.

merged with bug 1055139

merged with bug 1055139

Comment on attachment 8486659 [details] [diff] [review] Part 1: Push server 30 minute ping Review of attachment 8486659 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, that's a lot of feedback because implementing a protocol right is quite complicated but I like the direction of the patch a lot more :) ::: browser/app/profile/firefox.js @@ +238,3 @@ > // Themes every day > // Non-symmetric (not shared by extensions) extension-specific [update] preferences > +pref("extensions.dss.enabled", false); // Dynamic Skin Switching Can you please remove those changes from the patch? I know this sounds very nit-picky but white-space changes destroy all the nice blame-information we have. Well, not destroy but make it harder to find the original author and changeset. ::: browser/components/loop/MozLoopPushHandler.jsm @@ +36,5 @@ > + * @param {Function} onClose(aCode, aReason) called when the socket closes. > + * aCode is any status code returned on close > + * aReason is any string returned on close > + */ > + connect: function(pushUri, onMsg, onStart, onClose) { Maybe |, onClose = null) {| to make it more obvious that this parameter is optional? On the other hand, it seems that the only consumer of this API does actually pass "onClose". So we should probably make it a required parameter. @@ +43,5 @@ > + } > + > + this._onMsg = onMsg; > + this._onStart = onStart; > + this._onClose = onClose || (() => {return;}); this._onClose = onClose || function () {}; I wouldn't mind using the arrow function either, I feel like using function here is clearer. The return looks out of place though. @@ +97,5 @@ > + * @param {String} aMsg The message data > + */ > + onMessageAvailable: function(aContext, aMsg) { > + try { > + this._onMsg(JSON.parse(aMsg)); Should move the _onMsg() call out of the try {} block. We're only expecting JSON.parse() to fail but would hide other failures. @@ +110,5 @@ > + * > + * @param {Object} aMsg Message to send. > + */ > + send: function(aMsg) { > + let msg = {}; |let msg;| as you're not going to do anything with that default value if stringify() fails. @@ +123,5 @@ > + try { > + this._websocket.sendMsg(msg); > + } > + catch (error) { // in case websocket has closed before this call. > + this._onClose(1000, error); Do we still need to call _onClose() manually? If the socket closed in the meantime we would have already done that, no? @@ +128,5 @@ > + } > + }, > + > + close: function(aReason) { > + if (this._closing) { This seems to only track whether close() was called before. Shouldn't we also set this to true when onStop() is called? @@ +133,5 @@ > + return; > + } > + > + // Do not pass through any callbacks while socket is closing. > + this._onStart = () => {return;}; Same here as above. @@ +135,5 @@ > + > + // Do not pass through any callbacks while socket is closing. > + this._onStart = () => {return;}; > + this._onMsg = this._onStart; > + this._onClose = this._onStart; So why are we setting this to ._onStart()? When closing the socket we would expect onStop() to be called and would then call _onClose(), no? Looks like _onMsg() shouldn't be called again after closing the socket and the original _onClose() should still be called? @@ +139,5 @@ > + this._onClose = this._onStart; > + this._closing = true; > + > + try { > + this._websocket.close(1000, aReason); What's this magic "1000" here? That should go into a const at the top with a proper comment explaining what it is. @@ +161,5 @@ > pushUrl: undefined, > + // Protocol state variable > + serviceIs: SERVICE_STATE_OFFLINE, > + // Connection state variable > + connectionIs: CONNECTION_STATE_CLOSED, Should probably be called connectionState and serviceState. "Is" make it sound like a function to call instead of just a property. @@ +236,5 @@ > + * Returns MozLoopPushHandler to pre-initialized state. > + */ > + shutdown: function() { > + if (!this.connectionIs) { > + return; Should be |if (this.connectionIs == CONNECTION_STATE_CLOSED) {|, I had no idea what this was doing. @@ +251,5 @@ > + } > + > + this.connectionIs = CONNECTION_STATE_CLOSED; > + this.serviceIs = SERVICE_STATE_OFFLINE; > + this._pushSocket.close(1000, "close"); PushSocket.close() take a single parameter only. @@ +275,5 @@ > + uaid: this.uaID, > + channelIDs: this.serviceIs === SERVICE_STATE_REGISTERED ? [this.channelID] : [] > + }; > + /* > + * The Simple PushServer spec does not allow a retry of the Hello handshake but requires that the socket nit: the indentation of that whole comment is off by a space. @@ +280,5 @@ > + * be closed and another socket openned in order to re-attempt the handshake. > + * > + * NOTE: Set the retry callback BEFORE the operation that you want to retry. > + * A websocket listener callback may occur immediately before this next sendMsg() will return, > + * especially when running tests with a mock websocket handler. This seems weird. We shouldn't need that comment. Maybe the mock websocket handler should just respond after a setTimeout(0) to emulate the async behavior of a network connection better. @@ +297,2 @@ > case "hello": > + if (this.serviceIs === SERVICE_STATE_OFFLINE) { So if we receive a "hello" and the service state isn't offline... Shouldn't we close the connection as the server isn't sticking to the protocol? @@ +314,4 @@ > break; > > case "notification": > + if (this.serviceIs === SERVICE_STATE_REGISTERED) { Close the connection here if we receive a "notification" but aren't even registered yet? @@ +326,5 @@ > + break; > + > + default: > + if (this.serviceIs === SERVICE_STATE_REGISTERED) { > + // Treat this as a ping response What do we do with pings when we're not even registered? Close the connection? @@ +344,3 @@ > */ > + > + _notificationAck: function(chanUpdates) { We should probably move the whole "notification" handling code here and rename it to _onNotification(). It seems weird to artificially split it like this. In any case it seems nice to have _on${msg.messageType} functions for all the possible message types. Should make the code even prettier I hope :) @@ +366,5 @@ > break; > > + case CONNECTION_STATE_CONNECTING: > + // Wait before re-attempting to open the websocket. > + this._retryOperation(() => this._openSocket()); Seems like we should not use _retryOperation() here but just a simple setTimeout()? @@ +376,5 @@ > * Attempts to open a websocket. > * > * A new websocket interface is used each time. If an onStop callback > * was received, calling asyncOpen() on the same interface will > + * trigger a "already open socket" exception even though the channel nit: trigger an "already ..." @@ +400,5 @@ > let performOpen = () => { > + this._pushSocket.connect(this.pushServerUri, > + this._onMsg.bind(this), > + this._onStart.bind(this), > + this._onClose.bind(this)); nit: lots of trailing white space @@ +434,5 @@ > + } > + } else { > + console.warn("MozLoopPushHandler - push server URL retrieve error: " + req.status); > + pushServerURLFetchError(); > + } This whole block changed from white space to tabs? Looks like you didn't change something here and that's only white space? Please revert it and use spaces. We never use tabs anywhere. Good moment btw to check your editor configuration ;) @@ +441,5 @@ > req.send(); > } else { > // this.pushServerUri already set -- just open the channel > performOpen(); > + } nit: lots of trailing white space @@ +451,5 @@ > + * @param {} msg PushServer to UserAgent registration response (parsed from JSON). > + */ > + _onRegister: function(msg) { > + if (this.serviceIs !== SERVICE_STATE_PENDING) { > + return; Should we close the connection if that happens? Seems like the server would do something weird then. @@ +470,5 @@ > + break; > + > + case 500: > + // continue to retry the registration request after a suitable delay > + this._retryOperation(() => this._registerChannel()); Shouldn't that still be the current operation? Nothing called _retryEnd() so we'd be fine with doing nothing here? @@ +506,5 @@ > * Handles registering a service > */ > _registerChannel: function() { > + this.serviceIs = SERVICE_STATE_PENDING; > + this._retryOperation(() => this._registerChannel()); Stupid question but why would we even bother to retry registration if the server doesn't respond? Seems like that would point to a problem on the server or the connection just not working at all. I'd probably just use a setTimeout() here that calls _restartConnection() after a while. @@ +512,5 @@ > + channelID: this.channelID}); > + }, > + > + _pingRestartWait: function () { > + this._pingTimerID && clearTimeout(this._pingTimerID); Neat, but we tend to use |if (this._pingTimerID)| because that's just a tad more obvious :) @@ +518,5 @@ > + }, > + > + _pingSend: function () { > + this._pushSocket.send({}); > + this._pingTimerID = setTimeout(() => this._restartConnection(), this._pingTimeout_ms); I feel like we could have a separate PingTimer (or some other name) object that could handle that for us? Whenever we would like to restart we could call this._pingTimer.restart(). When the connection is closed we call this._pingTimer.stop() - we don't seem to do that currently btw. When a ping is received we just call .restart() again. The constructor could take the pushSocket and a callback that is called when the ping times out. The callback would probably just be _restartConnection(). @@ +530,2 @@ > */ > + _retryOperation: function(delayedOp) { The retry logic could live outside the PushHandler as well btw. Just like the Ping thingy suggested above. Another thing that is worth investigating... do we actually need that whole retry logic? It seems like mostly we would probably just like to restart the connection after a timeout as noted above. Why would we retry registering when communicating over TCP? If the server doesn't respond that points to a non-network layer problem and we should probably just restart. @@ +532,2 @@ > if (!this._retryCount) { > + this._retryDelay = this._minRetryDelay_ms; It's actually not the minimum delay anymore but just "the delay" we start with, right? Care to rename the property? I just saw that the pref is called retry_delay.start too :) @@ +534,3 @@ > this._retryCount = 1; > } else { > let nextDelay = this._retryDelay * 2; It seems that _retryCount still isn't used to back off after a certain number of retries? Increasing _retryCount doesn't affect anything currently afaict.

Comment on attachment 8486661 [details] [diff] [review] Part 2: xpcshell test updated with ping/restore Review of attachment 8486661 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/test/xpcshell/head.js @@ +73,5 @@ > * Mock nsIWebSocketChannel for tests. This mocks the WebSocketChannel, and > * enables us to check parameters and return messages similar to the push > * server. > */ > +let MockWebSocketChannel = function() {}; function MockWebSocketChannel() { } @@ +99,5 @@ > > + if (!message.messageType) { > + // Treat as a ping > + this.listener.onMessageAvailable(this.context, > + JSON.stringify({})); this.listener.onMessageAvailable(this.context, "{}"); Also, couldn't we just move that into the default case of the switch statement below? ::: browser/components/loop/test/xpcshell/test_looppush_initialize.js @@ +55,5 @@ > }); > > + add_test(function test_ping_websocket() { > + let waitForPing = true; > + MozLoopPushHandler.shutdown(); If you have to shutdown MozLoopPushHandler here, should this test maybe live in its own file? Maybe not, was just thinking out loud. @@ +58,5 @@ > + let waitForPing = true; > + MozLoopPushHandler.shutdown(); > + MozLoopPushHandler.initialize( > + function(err, url) { > + Assert.equal(err, null, "Should return null for success"); How about: "'err' should be null to indicate success"? Something that returns null for success sounds weird :) @@ +70,5 @@ > + }, > + function(version) { > + Assert.equal(version, 16, "Should have version number 16"); > + run_next_test(); > + }, nit: trailing white space @@ +81,5 @@ > Services.prefs.setCharPref("services.push.serverURL", kServerPushUrl); > Services.prefs.setIntPref("loop.retry_delay.start", 10); // 10 ms > Services.prefs.setIntPref("loop.retry_delay.limit", 20); // 20 ms > + Services.prefs.setIntPref("loop.ping.interval", 50); // 50 ms > + Services.prefs.setIntPref("loop.ping.timeout", 20); // 20 ms Shouldn't we also have something that resets those prefs when the test is done?

>@@ +344,3 @@ >> */ >> + >> + _notificationAck: function(chanUpdates) { > >We should probably move the whole "notification" handling code here and rename it to _onNotification(). It >seems weird to artificially split it like this. In any case it seems nice to have _on${msg.messageType} >functions for all the possible message types. Should make the code even prettier I hope :) Yes, it was somewhat artificial because I had put it in while waiting of feedback as to whether this was something that could/should be added; I felt is was missing. I will merge as suggested.

>@@ +297,2 @@ >> case "hello": >> + if (this.serviceIs === SERVICE_STATE_OFFLINE) { > >So if we receive a "hello" and the service state isn't offline... Shouldn't we close the connection as the >server isn't sticking to the protocol? Ignoring extra hello responses is specified in the Simple PushServer protocol specification. "PushServers MUST only respond to a hello once. UserAgents MUST ignore multiple hello replies. " (I am referencing https://wiki.mozilla.org/WebAPI/SimplePush/Protocol.)

(In reply to Paul Kerr [:pkerr] from comment #29) > Ignoring extra hello responses is specified in the Simple PushServer > protocol specification. "PushServers MUST only respond to a hello once. > UserAgents MUST ignore multiple hello replies. " Didn't see that in the spec, sorry. Carry on then.

>@@ +326,5 @@ >> + break; >> + >> + default: >> + if (this.serviceIs === SERVICE_STATE_REGISTERED) { >> + // Treat this as a ping response > >What do we do with pings when we're not even registered? Close the connection? The ping operation is only started after a successful registration. If we get one outside of the registered state, then it is most likely a late response from the PushServer. This should be ignored because if not in the registered state the MozPushHandler will be attempting to re-register.

>@@ +123,5 @@ >> + try { >> + this._websocket.sendMsg(msg); >> + } >> + catch (error) { // in case websocket has closed before this call. >> + this._onClose(1000, error); > >Do we still need to call _onClose() manually? If the socket closed in the meantime we would have already >done that, no? This may be a case of "belt and suspenders" for me. If the server has closed the socket or the network is down the first evidence can be a throw from sendMsg(). But, either the onStop or onServerClose callback should eventually be called leading to the same _onClose callback. I think in this case, just making the catch and simply proceeding is an equivalent course.

@@ +135,5 @@ >> + >> + // Do not pass through any callbacks while socket is closing. >> + this._onStart = () => {return;}; >> + this._onMsg = this._onStart; >> + this._onClose = this._onStart; > >So why are we setting this to ._onStart()? When closing the socket we would expect onStop() to be called >and would then call _onClose(), no? Looks like _onMsg() shouldn't be called again after closing the socket >and the original _onClose() should still be called? I want to disconnect the callbacks to MozLoopService once it has signaled it no longer wants the PushSocket. I am assuming that the retry logic is closing on websocket in order to, possibly immediately, open a new websocket. Unless there is some corner race condition, _onClose is probably the only one that needs to be dropped, but I cleared them call. MozPushServer does not wait for an _onClose after calling the close() method.

>@@ +366,5 @@ >> break; >> >> + case CONNECTION_STATE_CONNECTING: >> + // Wait before re-attempting to open the websocket. >> + this._retryOperation(() => this._openSocket()); > >Seems like we should not use _retryOperation() here but just a simple setTimeout()? What I want here is to get the increasing delay to the retry that this method managers instead of a fixed timeout with setTimeout().

>@@ +506,5 @@ >> * Handles registering a service >> */ >> _registerChannel: function() { >> + this.serviceIs = SERVICE_STATE_PENDING; >> + this._retryOperation(() => this._registerChannel()); > >Stupid question but why would we even bother to retry registration if the server doesn't respond? Seems >like that would point to a problem on the server or the connection just not working at all. I'd probably >just use a setTimeout() here that calls _restartConnection() after a while. After re-reading the register section of the spec., I find that is does mention retrying the connection as opposed to doing a simple retry of the message.

Incorporate feedback comments

Incorporate feedback comments

Comment on attachment 8494183 [details] [diff] [review] Part 1: Push server 30 minute ping Review of attachment 8494183 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I know it's my fault but can you please address the issues below and rebase the patch? I would like to apply it locally and check the whole logic again. That's a little hard to do by looking at a huge patch unfortunately. Thanks! ::: browser/components/loop/MozLoopPushHandler.jsm @@ +99,5 @@ > + * @param {String} aMsg The message data > + */ > + onMessageAvailable: function(aContext, aMsg) { > + try { > + var msg = JSON.parse(aMsg); nit: please use |let| and define it before the try block. @@ +115,5 @@ > + * @param {Object} aMsg Message to send. > + */ > + send: function(aMsg) { > + try { > + var msg = JSON.stringify(aMsg); nit: please use |let| and define it before the try block. @@ +170,5 @@ > + } else { > + let nextDelay = this._retryDelay * 2; > + this._retryDelay = nextDelay > this._maxDelay ? this._maxDelay : nextDelay; > + } > + this._timeoutID = setTimeout(delayedOp, this._retryDelay); As a "public" method this should probably call clearTimeout() before setTimeout() just to make sure we don't retry the old action. @@ +187,5 @@ > + > +function PingMonitor(pingFunc, onTimeout, interval, timeout) { > + if (!pingFunc || !onTimeout || !interval || !timeout) { > + throw new Error("PingMonitor: missing required parameters"); > + } nit: please insert a newline @@ +198,5 @@ > +PingMonitor.prototype = { > + > + restart: function () { > + this.stop(); > + this._pingTimerID = setTimeout(() => {this._pingSend()}, this._pingInterval); nit: ... setTimeout(() => this._pingSend(), ... @@ +202,5 @@ > + this._pingTimerID = setTimeout(() => {this._pingSend()}, this._pingInterval); > + }, > + > + stop: function() { > + if (this._pingTimerID){ nit: if (this._pingTimerID) { @@ +251,5 @@ > })(), > > + _pingInterval_ms: (() => { > + try { > + return Services.prefs.getIntPref("loop.ping.interval") nit: missing semicolon @@ +254,5 @@ > + try { > + return Services.prefs.getIntPref("loop.ping.interval") > + } > + catch (e) { > + return 18000000 // 30 minutes nit: missing semicolon @@ +260,5 @@ > + })(), > + > + _pingTimeout_ms: (() => { > + try { > + return Services.prefs.getIntPref("loop.ping.timeout") nit: missing semicolon @@ +263,5 @@ > + try { > + return Services.prefs.getIntPref("loop.ping.timeout") > + } > + catch (e) { > + return 10000 // 10 seconds nit: missing semicolon @@ +298,5 @@ > this._notificationCallback = notificationCallback; > + this._retryManager = new RetryManager(this._startRetryDelay_ms, > + this._maxRetryDelay_ms); > + this._pingMonitor = new PingMonitor(() => {this._pushSocket.send({})}, > + () => {this._restartConnection()}, ... new PingMonitor(() => this._pushSocket.send({}), () => this._restartConnection(), @@ +356,5 @@ > + * > + * NOTE: Set the retry callback BEFORE the operation that you want to retry. > + * A websocket listener callback may occur immediately before this next sendMsg() will return, > + * especially when running tests with a mock websocket handler. > + */ Nit: the comment is wrongly indented by one space to the left. @@ +369,3 @@ > */ > + _onMsg: function(aMsg) { > + switch(aMsg.messageType) { nit: switch (aMsg.messageType) { @@ +429,5 @@ > + if (this.serviceState === SERVICE_STATE_REGISTERED) { > + this._pingMonitor.restart(); > + > + if (Array.isArray(aMsg.updates) && aMsg.updates.length > 0) { > + aMsg.updates.forEach((update) => { nit: aMsg.updates.forEach(update => { @@ +452,5 @@ > + * This method will continually try to open a connection > + * to the PushServer unless shutdown has been called. > + */ > + _onClose: function(aCode, aReason) { > + switch (this.connectionState) { Shouldn't this set |connectionState| to CONNECTION_STATE_CLOSED? @@ +454,5 @@ > + */ > + _onClose: function(aCode, aReason) { > + switch (this.connectionState) { > + case CONNECTION_STATE_OPEN: > + console.log("MozLoopPushHandler: websocket closed - ", aCode); I don't think we want debug output in release builds that isn't at least a warning. @@ +504,2 @@ > this._registerCallback("error: PushServer ChannelID already in use"); > + this.shutdown(); shutdown() calls _retryManager.reset() already, no? @@ +510,2 @@ > this._registerCallback("error: PushServer registration failure, status = " + msg.status); > + this.shutdown(); see above @@ +534,5 @@ > + this._pushSocket = new PushSocket(); > + > + if (this._mockPushHandler) { > + // For tests, use the mock instance. > + this._pushSocket._websocket = this._mockPushHandler; Why don't we just pass the mock websocket to |new PushSocket()|? PushSocket can then in the constructor create one if that's falsy. @@ +541,5 @@ > let performOpen = () => { > + this._pushSocket.connect(this.pushServerUri, > + this._onMsg.bind(this), > + this._onStart.bind(this), > + this._onClose.bind(this)); Is there a reason we use |this._func.bind(this)| here? Seems like we should use .bind() or arrow functions everywhere when passing callbacks. @@ +613,5 @@ > * Handles registering a service > */ > _registerChannel: function() { > + this.serviceState = SERVICE_STATE_PENDING; > + this._retryManager.retry(() => {this._restartConnection()}); nit: this._retryManager.retry(() => this._restartConnection());

Comment on attachment 8494184 [details] [diff] [review] Part 2: xpcshell test updated with ping/restore Review of attachment 8494184 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/test/xpcshell/test_looppush_initialize.js @@ +61,5 @@ > + function(err, url) { > + Assert.equal(err, null, "err should be null to indicate success"); > + if (waitForPing) { > + MozLoopPushHandler.uaID = undefined; > + MozLoopPushHandler.pushUrl = undefined; //Do this to force a new registration callback. I can't quite figure out how this ensures that pinging works. Will a ping force a new registration? Shouldn't we just use the mockWebSocket to check that pings arrive and that if we don't respond it times out and tries to re-connect?

we will take as soon as we can - but not sure we need to uplift to Fx35 over other work Paul has on his plate. ride trains to Fx36 (still 11/25).

Consider adding, under a debug pref, the extra logging and pref-based override capabilities that were used during the debug of the PushServer issues.

merge multi-channel register with ping and ack functions

merge multi-channel register with ping and ack functions

[Tracking Requested - why for this release]: After discussion with the push server folks, we believe this would improve our service reliability.

Comment on attachment 8530904 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler I will take a review from whomever has time to complete a review first. Note that this patch has been updated to apply on a newer version of the MozLoopPushHandler that can handle the registration of multiple notification channels.

Update to ping test: verify the both ping sent and ping timeout action.

clear channels map on shutdown

Comment on attachment 8531386 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler I will take a review from whomever has time to complete a review first. Note that this patch has been updated to apply on a newer version of the MozLoopPushHandler that can handle the registration of multiple notification channels.

fix whitespace nits

Comment on attachment 8531392 [details] [diff] [review] Part 2: xpcshell test updated with ping/restore I will take a review from whomever has time to complete a review first. Note that this patch has been updated to apply on a newer version of the MozLoopPushHandler that can handle the registration of multiple notification channels.

We won't block the release for this. Not tracking.

After discussion with Standard8, I think it makes sense to land this ASAP (in Fx37 Nightly) and ask for uplift to Fx36 only.

Comment on attachment 8531386 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler Looking to land this and get it uplifted for Fx36 if we can. I'll take a review from whoever gets to this first.

Comment on attachment 8531386 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler I'm actively looking at this.

Comment on attachment 8531386 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler Review of attachment 8531386 [details] [diff] [review]: ----------------------------------------------------------------- Ok, I've taken an in-depth look at the code changes and generally it looks good. There's a few minor bits that could be improved though as per my comments below. Also, there's a bit of bitrot - if you can fix that, then I'll take a look at testing some of it out. I'll also look at a code review of the tests tomorrow, regardless of if you've updated the patch by then or not. One other thing - I think it might be worth considering having some consistent text at the start of the log messages to help with searching/filtering when intermingled with other messages. I'm not sure if you get that with the current setup, but I think its worth considering in this, or in a future patch. ::: browser/components/loop/MozLoopPushHandler.jsm @@ +297,5 @@ > + * Function to stop the PingMonitor. > + */ > + stop: function() { > + if (this._pingTimerID){ > + clearTimeout(this._pingTimerID); Probably best to delete this._pingTimerID, at least to be tidy. @@ +511,5 @@ > }; > + // The Simple PushServer spec does not allow a retry of the Hello handshake but requires that the socket > + // be closed and another socket openned in order to re-attempt the handshake. > + this._retryManager.reset(); > + this._retryManager.retry(() => this._restartConnection()); I think it'd be worth documenting here, that retry manager in this instance isn't being used to retry connecting, but as a simple timeout for restarting the connection if the hello hasn't succeeded. @@ +593,2 @@ > > + this._retryManager.reset(); Maybe add a comment along the lines of "Stop the restart connection timeout kicking in". @@ +600,5 @@ > + // are no longer valid and a Registration request is generated. > + if (this.uaID !== aMsg.uaid) { > + consoleLog.log("Registering all channels"); > + this.uaID = aMsg.uaid; > + // re-register all channels. nit: capital R for Re @@ +614,5 @@ > * > + * This method will parse the Array of updates and trigger > + * the callback of any registered channel. > + * This method will construct an ack message containing > + * as set of channel version update notifications. s/as set/a set/ @@ +714,5 @@ > * Attempts to open a websocket. > * > * A new websocket interface is used each time. If an onStop callback > * was received, calling asyncOpen() on the same interface will > + * trigger an "alreay open socket" exception even though the channel s/alreay/already/ ? @@ +790,5 @@ > + if (this.connectionState === CONNECTION_STATE_OPEN) { > + // Close the current PushSocket and start the operation to open a new one. > + this._pushSocket.close(); > + this._openSocket(); > + consoleLog.warn("MozLoopPushHandler - connection error: re-establishing connection to PushServer"); Nit: this warn should probably go before the _openSocket in case _openSocket logs anything.

Fix bit-rot and incorporate review feedback

Fix bit-rot and incorporate review feedback

Normalized debug print prefixes

Comment on attachment 8546856 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler Review of attachment 8546856 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, there's a couple of technical changes, but most of the comments are whitespace cleanups. r=Standard8 with the comments addressed. Sorry for all the delays. ::: browser/components/loop/MozLoopPushHandler.jsm @@ +46,5 @@ > + */ > + > + connect: function(pushUri, onMsg, onStart, onClose) { > + if (!pushUri || !onMsg || !onStart || !onClose) { > + throw new Error("PushSocket: missing required parameter(s):" nit: trailing space @@ +405,5 @@ > > this._openSocket(); > return true; > }, > + nit: whitespace on blank line @@ +518,5 @@ > + this._retryManager.reset(); > + this._retryManager.retry(() => this._restartConnection()); > + this._pushSocket.send(helloMsg); > + }, > + nit: whitespace on blank line @@ +561,2 @@ > > + switch(aMsg.messageType) { This gives an error for the ping case: ReferenceError: reference to undefined property aMsg.messageType for a ping. Maybe pull the default case out, or fake a default value? @@ +615,2 @@ > }, > + nit: whitespace on blank line @@ +632,5 @@ > + consoleLog.error("PushHandler: protocol error - notification received in wrong state"); > + this._restartConnection(); > + return; > + } > + nit: whitespace on blank line @@ +648,5 @@ > + consoleLog.error("PushHandler: notification received for unknown channelID: ", > + update.channelID); > + } > + }); > + nit: whitespace on blank line @@ +673,2 @@ > } > + nit: whitespace on blank line @@ +676,4 @@ > > switch (msg.status) { > case 200: > + this._retryManager.reset(); It looks like we reset the retry manager in every case here. Maybe pull that out before the switch?

Comment on attachment 8545422 [details] [diff] [review] Part 2: xpcshell test updated with ping/restore Review of attachment 8545422 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=Standard8 ::: browser/components/loop/test/xpcshell/test_looppush_initialize.js @@ +83,5 @@ > }); > }); > > + // Test that the PushHander will re-connect after the near-end disconnect. > + // The uaID is cleared to force re-regsitration of all notification channels. nit: spelling re-registration (in a couple of comments) @@ +125,5 @@ > + function(version, id) { > + return; > + }); > + }); > + nit: trailing whitespace in 4 places

Incorporate review feedback. r=standard8

Incorporate review feedback. r=standard8

Comment on attachment 8549011 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler Carry forward r+ Standard8

Comment on attachment 8549012 [details] [diff] [review] Part 2: xpcshell test updated with ping/restore Carry forward r+ Standard8

https://hg.mozilla.org/integration/fx-team/rev/fd843aa3b6f0 https://hg.mozilla.org/integration/fx-team/rev/a3f89050d1f6

https://hg.mozilla.org/mozilla-central/rev/fd843aa3b6f0 https://hg.mozilla.org/mozilla-central/rev/a3f89050d1f6

Comment on attachment 8549011 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: [Describe test coverage new/current, TBPL]: [Risks and why]: [String/UUID change made/needed]: Approval Request Comment [Feature/regressing bug #]: this bug (1028869) [User impact if declined]: Network conditions can allow the Websocket connection between the Hello client and PushServer to stay alive even when the PushServer is actually off-line or unreachable. In this case, the user will not be able to notice that they are un-reachable for inbound Hello calls. While this condition can be cleared by restart the browser, this bug adds the PushServer ping capability which will detect the loss of the PushServer connection and trigger the connection retry logic which will re-establish the PushServer connection when the PushServer next becomes available. This will improve the overall responsiveness of the Hello client. [Describe test coverage new/current, TBPL]: Patch is currently on mozilla-central. Automated tests exist to test PushServer handler regression in the client and to the new functionality added by this bug. Manual testing has been performed for various network issues that can be encountered. [Risks and why]: Low. A full set of unit and functional tests already exist to test the PushServer handler module of the Hello client. If this patch fails to notice or re-establish a connection to the Hello PushServer, the user is in no worse shape than the current functionality and a browser restart will reconnect to the PushServer if it is available.

Comment on attachment 8549011 [details] [diff] [review] Part 1: Add ping and ack operations to PushHandler Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: [Describe test coverage new/current, TBPL]: [Risks and why]: [String/UUID change made/needed]: Approval Request Comment [Feature/regressing bug #]: this bug (1028869) [User impact if declined]: Network conditions can allow the Websocket connection between the Hello client and PushServer to stay alive even when the PushServer is actually off-line or unreachable. In this case, the user will not be able to notice that they are un-reachable for inbound Hello calls. While this condition can be cleared by restart the browser, this bug adds the PushServer ping capability which will detect the loss of the PushServer connection and trigger the connection retry logic which will re-establish the PushServer connection when the PushServer next becomes available. This will improve the overall responsiveness of the Hello client. [Describe test coverage new/current, TBPL]: Patch is currently on mozilla-central. Automated tests exist to test PushServer handler regression in the client and to the new functionality added by this bug. Manual testing has been performed for various network issues that can be encountered. [Risks and why]: Low. A full set of unit and functional tests already exist to test the PushServer handler module of the Hello client. If this patch fails to notice or re-establish a connection to the Hello PushServer, the user is in no worse shape than the current functionality and a browser restart will reconnect to the PushServer if it is available. [String/UUID change made/needed]: none

Comment on attachment 8549012 [details] [diff] [review] Part 2: xpcshell test updated with ping/restore Approval Request Comment [Feature/regressing bug #]: this bug (1028869) [User impact if declined]: This bug adds specific unit tests for the two new features added by this bug: ping and ack operations with the PushServer. It should be uplifted along with Part 1 of this bug. [Describe test coverage new/current, TBPL]: Adds unit tests for the ping and ack PushServer operations. [Risks and why]: Low. Uplifting the unit test additions should help reduce the risk of Part 1 uplift. [String/UUID change made/needed]: none

https://hg.mozilla.org/releases/mozilla-aurora/rev/3953437a1bcd https://hg.mozilla.org/releases/mozilla-aurora/rev/e86a967b9875

https://hg.mozilla.org/releases/mozilla-beta/rev/fc47c7a95f85 https://hg.mozilla.org/releases/mozilla-beta/rev/b653be6b040a