The workarounds in MozLoopService don't handle the websocket being disconnected, nor do they handle a push url expiry (if there is such a thing, which happens without disconnection). This means that a user could loose connection at some stage.

We need to handle this for mvp, so hitting in FF 33 timescale.

need to make sure we have stability we do not want to rely on the existing Push implementation in desktop - so need to continue to use our custom implementation. network API we are using will be available, but desktop PUSH is changing to evolving w3c spec.

Code changes to add retry functionality when connection to PushServer is lost. Patch to follow with updated xpcshell tests.

Comment on attachment 8454766 [details] [diff] [review] Part 1: Add retry logic to PushServer user agent Review of attachment 8454766 [details] [diff] [review]: ----------------------------------------------------------------- Generally looks good to me, most of the comments are trivial. I haven't got perms to review this, Justin Dolske is the person to ask this week. ::: browser/components/loop/MozLoopPushHandler.jsm @@ +32,5 @@ > pushUrl: undefined, > + // Set to true once the channelID has been registered with the PushServer. > + registered: false, > + > + _minRetryDelay_ms: (() => { nit: trailing whitespace @@ +41,5 @@ > + return 60000 // 1 minute > + } > + })(), > + > + _maxRetryDelay_ms: (() => { nit: trailing whitespace @@ +97,5 @@ > + // If a uaID has already been assigned, assume this is a re-connect > + // and send the uaID in order to re-synch with the > + // PushServer. If a registration has been completed, send the channelID. > + this._retryEnd(); > + let helloMsg = { messageType: "hello", I think it makes clearer reading to put the comment after the retryEnd before the let. @@ +159,5 @@ > break; > } > }, > > + _onRegister: function(msg) { nit: please document this function and arguments @@ +192,5 @@ > + * was received, calling asyncOpen() on the same interface will > + * trigger a "alreay open socket" exception even though the channel > + * is logically closed. > + */ > + _openSocket: function() { Due to bug 1038304, I think it would be useful to incorporate the Services.io.offline check (similar to that in initialize) in here. This is what was throwing me when I was testing it earlier. iirc I had put the check in initialize due to the bug, but then failed to file the core bug. Supporting offline mode also makes it easier to test, even if it isn't used that often. @@ +216,5 @@ > channelID: this.channelID > })); > + }, > + > + _retryOperation: function(delayedOp) { nit: please document this function and arguments @@ +228,5 @@ > + } > + this._timeoutID = setTimeout(delayedOp, this._retryDelay); > + }, > + > + _retryEnd: function() { nit: please document this function and arguments

Update based on feedback

Updated xpcshell unit test.

Comment on attachment 8456483 [details] [diff] [review] Part 1: Add retry logic to PushServer user agent Review of attachment 8456483 [details] [diff] [review]: ----------------------------------------------------------------- I don't really know Loop code, so I think you'd be better off with a review from another Loop developer. But feedback+ with a couple nits, and if your other reviewer is OK with the changes I'm ok with that review being sufficient for landing, as a module peer. ::: browser/components/loop/MozLoopPushHandler.jsm @@ +21,2 @@ > */ > +//XXX - do we need to handle non-response from the PushServer during the hello handshake? Nit -- if it's a question that needs answering, general preferred style it to either do it in this bug (and remove the XXX) or file a new bug on the issue and note the bug number here. (As had been done with the comment being removed.) @@ +43,5 @@ > + })(), > + > + _maxRetryDelay_ms: (() => { > + try { > + return Services.prefs.getIntPref("loop.retry_delay.limit") Unless these are extremely obscure, I'd suggest adding them to all.js/firefox.js so they're visible in about:prefs by default.

Comment on attachment 8456484 [details] [diff] [review] Part 2: Add additional tests for new functional paths Review of attachment 8456484 [details] [diff] [review]: ----------------------------------------------------------------- (ditto)

Comment on attachment 8456483 [details] [diff] [review] Part 1: Add retry logic to PushServer user agent Given Justin's comment, I can take on the review.

1) Added retry operation to the PushServer initial handshake. 2) Put retry timeouts into prefs

Comment on attachment 8458012 [details] [diff] [review] Part 1: Add retry logic to PushServer user agent Review of attachment 8458012 [details] [diff] [review]: ----------------------------------------------------------------- The additional changes here look good. r=Standard8 ::: browser/app/profile/firefox.js @@ +1520,1 @@ > #endif Just to note there's a little bit of bitrot here as the ifdefs have been removed.

Comment on attachment 8456484 [details] [diff] [review] Part 2: Add additional tests for new functional paths Review of attachment 8456484 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=Standard8 ::: browser/components/loop/test/xpcshell/test_looppush_initialize.js @@ +29,5 @@ > + }, > + function(version) { > + Assert.equal(version, 15, "Should have version number 15"); > + run_next_test(); > + }, nit: trailing whitespace. @@ +36,5 @@ > > + add_test(function test_reconnect_websocket() { > + MozLoopPushHandler.uaID = undefined; > + MozLoopPushHandler.pushUrl = undefined; //Do this to force a new registration callback. > + mockWebSocket.stop(); I think it would be either worth documenting that these tests rely on the callbacks set up in test_initalize_websocket, or redefine those in these tests. It took me a couple of minutes to work out how this was succeeding.

fix of bit-rotted patch

tagged with r=standard8

https://hg.mozilla.org/integration/mozilla-inbound/rev/a561499de7a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/89b195298ca3

Comment on attachment 8458349 [details] [diff] [review] Part 1: Add retry logic to PushServer user agent marking r=standard8

Comment on attachment 8458350 [details] [diff] [review] Part 2: Add additional tests for new functional paths marking r=standard8

https://hg.mozilla.org/mozilla-central/rev/a561499de7a4 https://hg.mozilla.org/mozilla-central/rev/89b195298ca3

Does this need QA testing or is it sufficiently covered by automation?

I believe that automation should cover the failure modes. It is sufficiently covered.

(In reply to Paul Kerr [:pkerr] from comment #24) > I believe that automation should cover the failure modes. It is sufficiently > covered. Thanks Paul, please needinfo? me if you change your mind.