1002414 - Loop Service needs to handle push server disconnects / expiry (Push connection 'silently' drops, unable to receive incoming call)

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

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

Comment 2

[:shell escalante]

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.

Comment 4

[Paul Kerr [:pkerr]]

Attachment: Part 1: Add retry logic to PushServer user agent

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

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[Paul Kerr [:pkerr]]

Attachment: Part 1: Add retry logic to PushServer user agent

Update based on feedback

Comment 7

[Paul Kerr [:pkerr]]

Attachment: Part 1: Add retry logic to PushServer user agent

Comment 8

[Paul Kerr [:pkerr]]

Attachment: Part 2: Add additional tests for new functional paths

Updated xpcshell unit test.

Comment 9

[Justin Dolske [:Dolske]]

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 10

[Justin Dolske [:Dolske]]

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 11

[Mark Banner (:standard8)]

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.

Comment 12

[Paul Kerr [:pkerr]]

Attachment: Part 1: Add retry logic to PushServer user agent

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

Comment 13

[Mark Banner (:standard8)]

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 14

[Mark Banner (:standard8)]

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.

Comment 15

[Paul Kerr [:pkerr]]

Attachment: Part 1: Add retry logic to PushServer user agent

fix of bit-rotted patch

Comment 16

[Paul Kerr [:pkerr]]

Attachment: Part 1: Add retry logic to PushServer user agent

Comment 17

[Paul Kerr [:pkerr]]

Attachment: Part 2: Add additional tests for new functional paths

Comment 18

[Paul Kerr [:pkerr]]

tagged with r=standard8

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

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

marking r=standard8

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

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

marking r=standard8

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[u279076]

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

Comment 24

[Paul Kerr [:pkerr]]

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

Comment 25

[u279076]

(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.