1207300 - Start Conversation button doesn't respond

Status: RESOLVED FIXED

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

Description

[Ian Bicking (:ianbicking)]

When I open the Hello dialog, the Start Conversation button doesn't respond.

Using Dev Edition, 42.0a2 (2015-09-18)

Comment 1

[Mark Banner (:standard8)]

(In reply to Ian Bicking (:ianb) from comment #0)
> When I open the Hello dialog, the Start Conversation button doesn't respond.

Are any of your conversations listed?

Do you get any errors in the browser console?

My suspicion is that your connection to the push servers is down, and it might take 5 mins or so to recover. Restarting FF would probably fix it, but also likely loose the error state.

In 43, we've got a waiting state on startup that should have a better indication of if there's no connection at startup, though we do need to improve our indications further.

Comment 2

[Ian Bicking (:ianbicking)]

No conversations are listed, but I'm not sure I've created any in this profile.

No errors in the browser console.  I do see this recently:

  Loop:logInToFxA with fxAOAuthTokenData: false MozLoopService.jsm:1596
  Loop:hawkRequestInternal:  2 /fxa-oauth/params POST MozLoopService.jsm:602
  Loop:Stored a hawk session token for sessionType 2

I haven't restarted Firefox, but I have changed networks and the computer has gone to sleep and woken up, with no change over the course of the day.  So if the connection went down, it didn't come back up again.

I can still change my availability status and the console shows messages that the change was sent to the loop server.

Minutes later... I just completed a successful sign-in and Start Conversation started working.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Ian Bicking (:ianb) from comment #2)
> I haven't restarted Firefox, but I have changed networks and the computer
> has gone to sleep and woken up, with no change over the course of the day. 
> So if the connection went down, it didn't come back up again.

This strikes me as suspicious.  Mark, is it possible that after wakeup, the code needs to reconnect to the push server, but may not immediately realize it?

Comment 4

[Mark Banner (:standard8)]

Ok, I took a dig at this. I hadn't realised that Mac now manages the offline status (Services.io.offline) according to the network status. That's an aside, so let's break this down:

- Registration completed successfully

I can't see anything fundamentally wrong in this case. If the browser goes offline & then back online, we'll attempt to reconnect the push server. Communication with loop-server still works, so worst case the user just won't get push notifications until the reconnection happens.

We also won't tell them, which is another bug...

- Browser offline on startup, registration didn't complete

I'm guessing this is what hit Ian. When we startup offline, if you open the panel, then you'll now get the spinning loading indicator (you used to get nothing), and the start button is disabled.

That *never* recovers, even when the browser gets back online.

There's some weird interactions here, I think what's happening is:

* The panel does a getAll()
* This gets converted into causing registration
* There's an offline check in PushHandler#initialize that happens, but the return result is never checked.
* Some of the registration tries to continue, but obviously doesn't succeed
* The panel never gets a result from the getAll call.

Additionally, as that offline check doesn't have the return result being handled, then we don't display an error... and due to the issues with the initialization there's no way to kick us out of this mode without a restart.


I have a patch that removes the offline check from the push handler. This lets the push handler's initialisation complete, which in turn means the push handler sets up its timers to retry the connection. When the connection comes online, it can then detect that, and registration with the loop-server completes.

This isn't perfect, and there's no notification for the user, but at least we can get the user out of the really bad state - especially in the case of start up Firefox whilst offline, connect, and then after some time they use Hello.

I think what this does tell me, is that we need to take a serious look at that code and rework some of it with what we want from the UX in mind - currently its a bit of a mess, and there's different notifications coming from different places. I'll file a meta bug on this later.

Comment 5

[Mark Banner (:standard8)]

Attachment: If Firefox is started offline, then Loop never initialises correctly.

Here's the patch, this seems to work fine for all the cases I tried it in.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8667839 [details] [diff] [review]
If Firefox is started offline, then Loop never initialises correctly.

Review of attachment 8667839 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/modules/MozLoopPushHandler.jsm
@@ +390,3 @@
>  
>      if (this._initDone) {
> +      return;

It appears that the tests, although not the code, does check return values.

This suggests to me that if the tests are passing with this patch, that we need another test to get coverage over the above clause.

Assuming that you agree that it makes sense to keep the current test structure (I'm not actually sure whether or not it does), we probably should define this to return a boolean in the jsdoc and keep the existing returns at least.

Comment 7

[Mark Banner (:standard8)]

Attachment: If Firefox is started offline, then Loop never initialises correctly.

I don't think it makes sense to keep the return values since we never check them. The fact that init is already done doesn't actually matter.

I'd actually forgotten to check the tests as well.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8668094 [details] [diff] [review]
If Firefox is started offline, then Loop never initialises correctly.

Review of attachment 8668094 [details] [diff] [review]:
-----------------------------------------------------------------

Agreed that a spinoff bug is needed; thanks for the analysis and fix!  r=dmose.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

1 additional question, though: do we want a mochitest covering the fix you just made?

Comment 10

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #9)
> 1 additional question, though: do we want a mochitest covering the fix you
> just made?

I think that's going to be difficult to do - its going to require faking the push server as well as the loop-server. I think overall, it'd be worth investigating as a functional test once we've done additional work for whatever offline indications we end up with.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/41880e71001e

Comment 12

[Mark Banner (:standard8)]

I've filed bug 1210501 as a meta to track the follow-up work, and I've filed one for the UX work needed.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41880e71001e

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8668094 [details] [diff] [review]
If Firefox is started offline, then Loop never initialises correctly.

Approval Request Comment
[Feature/regressing bug #]: Has been an issue in Hello for a while now, certainly affects 42 and probably older.
[User impact if declined]: If Firefox is started when the browser is in offline mode (as recently happens with the newer network management detection), then Hello will be inaccessible even if the user goes online. The only way out of it is for the user to restart Firefox.
[Describe test coverage new/current, TreeHerder]: The registration processes have test coverage.
[Risks and why]: Low. Removes a check that currently does the wrong thing and isn't properly handled. This lets the code run that manages to do the right thing.
[String/UUID change made/needed]: None

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8668094 [details] [diff] [review]
If Firefox is started offline, then Loop never initialises correctly.

Improve the feature, has test, taking it.
Should be in 42 b4.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3fdb63e523f9

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/33a62058611e

Comment 18

[Andrei Vaida [:avaida]]

Ugh, removing qe-verify since this already has automated coverage.