Start Conversation button doesn't respond

RESOLVED FIXED in Firefox 42

Status

Hello (Loop)
Client
P1
normal
Rank:
18
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ianbicking, Assigned: standard8)

Tracking

unspecified
mozilla44
Points:
3

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed, firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

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

Comment 1

3 years ago
(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.
Flags: needinfo?(ianb)
(Reporter)

Comment 2

3 years ago
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.
Flags: needinfo?(ianb)
(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?
Flags: needinfo?(standard8)
(Assignee)

Comment 4

3 years ago
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.
Flags: needinfo?(standard8)
(Assignee)

Comment 5

3 years ago
Created attachment 8667839 [details] [diff] [review]
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.
Attachment #8667839 - Flags: review?(dmose)
(Assignee)

Updated

3 years ago
Iteration: --- → 44.1 - Oct 5
Points: --- → 3
Rank: 18
Priority: -- → P1
(Assignee)

Updated

3 years ago
Assignee: nobody → standard8
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.
Attachment #8667839 - Flags: review?(dmose) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8668094 [details] [diff] [review]
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.
Attachment #8667839 - Attachment is obsolete: true
Attachment #8668094 - Flags: review?(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.
Attachment #8668094 - Flags: review?(dmose) → review+
1 additional question, though: do we want a mochitest covering the fix you just made?
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 12

3 years ago
I've filed bug 1210501 as a meta to track the follow-up work, and I've filed one for the UX work needed.
https://hg.mozilla.org/mozilla-central/rev/41880e71001e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Comment 14

3 years ago
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
Attachment #8668094 - Flags: approval-mozilla-beta?
Attachment #8668094 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
status-firefox42: --- → affected
status-firefox43: --- → affected
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.
Attachment #8668094 - Flags: approval-mozilla-beta?
Attachment #8668094 - Flags: approval-mozilla-beta+
Attachment #8668094 - Flags: approval-mozilla-aurora?
Attachment #8668094 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
QA Contact: bogdan.maris
(Assignee)

Updated

3 years ago
Blocks: 1188917
Ugh, removing qe-verify since this already has automated coverage.
Flags: qe-verify+
QA Contact: bogdan.maris
You need to log in before you can comment on or make changes to this bug.