1120003 - direct calls code that touches network state hard to reason about, has issues

Status: RESOLVED FIXED

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

+++ This bug was initially created as a clone of Bug #1118061 +++

[Tracking Requested - why for this release]:

Given this amount of user training, I'm concerned that a substantial number of users may interpret every non-answer or busy as "the system is broken right now", which would effectively make the product seem extremely broken to those people, even when it's not.

In the short term, we need to audit all the possible callStateReasons for call termination and expose any appropriate reasons as "contact unavailable" rather than "something went wrong".  This includes the callStateReasons that come from the websocket, as well as the various other things that come from other parts of the client.

Medium-term (i.e. in another bug), we may want to stop overloading that variable with data sourced from different places.

One known issue is that the "closed" reason is sent by the server when someone clicks the "X" gadget in the call window.  The bug here _might_ be that the unload handler on that window isn't cleaning up the websocket and sending a better reason (presumably "reject").

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

> Medium-term (i.e. in another bug), we may want to stop overloading that
> variable with data sourced from different places.

Or at least namespace them to avoid collisions.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Adam, do these things seem like a reasonable approach to you?

* get rid of the unused websocket code
* split out a separate rest errno for user unavailable
* use the name USER_UNAVAILABLE rather than USER_UNKNOWN based on the speculation that it's since it's more opaque, at least unsophisticated spammers would be less likely to consider using it to test for the existence for accounts

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

bah, comment 2 in the wrong bug.  Please ignore!  I shall tag it off-topic to hide it.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Loop direct calls sometimes fail with a confusing/incorrect msg

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Loop direct calls sometimes fail with a confusing/incorrect msg

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Hey Mark, can you look over the changes that Dan and I put together here? Thanks! We want to abstract the FAILURE_REASONS but don't want to do it in the same patch because it will add extra complexity for the reviews and also code archaeology.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8550465 [details] [diff] [review]
Patch

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

From what I can tell, there's no actual functionality change in this patch - if that's right, then the patch title needs updating to mention that this is reworking the websocket and rest codes into constants. If I'm not right, please re-request review pointing out what I've missed.

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +376,3 @@
>              console.error("Failed to get outgoing call data", err);
>              var failureReason = "setup";
>              if (err.errno == 122) {

Shouldn't we change the 122 to REST_ERRNOS.USER_UNAVAILABLE as well?

::: browser/components/loop/content/shared/js/websocket.js
@@ +230,4 @@
>       * @param {Object} event The websocket onmessage event.
>       */
>      _onmessage: function(event) {
> +      var msgObject;

I think msgData would be clearer than msgObject.

::: browser/components/loop/test/standalone/webapp_test.js
@@ +20,5 @@
>        stubGetPermsAndCacheMedia,
>        fakeAudioXHR,
>        dispatcher,
> +      feedbackStore,
> +      WEBSOCKET_REASONS = loop.shared.utils.WEBSOCKET_REASONS;;

nit: double ;

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mark Banner (:standard8) from comment #8)

> From what I can tell, there's no actual functionality change in this patch -
> if that's right, then the patch title needs updating to mention that this is
> reworking the websocket and rest codes into constants.

That's right; will fix.

> ::: browser/components/loop/content/shared/js/conversationStore.js
> @@ +376,3 @@
> >              console.error("Failed to get outgoing call data", err);
> >              var failureReason = "setup";
> >              if (err.errno == 122) {
> 
> Shouldn't we change the 122 to REST_ERRNOS.USER_UNAVAILABLE as well?

Fixed, and this uncovered a bug where we weren't importing that either, so I've fixed that as well.

> ::: browser/components/loop/content/shared/js/websocket.js
> @@ +230,4 @@
> >       * @param {Object} event The websocket onmessage event.
> >       */
> >      _onmessage: function(event) {
> > +      var msgObject;
> 
> I think msgData would be clearer than msgObject.

Agred; fixed.

> 
> ::: browser/components/loop/test/standalone/webapp_test.js
> @@ +20,5 @@
> >        stubGetPermsAndCacheMedia,
> >        fakeAudioXHR,
> >        dispatcher,
> > +      feedbackStore,
> > +      WEBSOCKET_REASONS = loop.shared.utils.WEBSOCKET_REASONS;;
> 
> nit: double ;

Fixed.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: [landed fx-team] Hoist Loop REST errnos and websocket reasons, patch=jaws,dmose, r=Standard8

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/8ea496bb8402

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ea496bb8402

Comment 13

[Mark Banner (:standard8)]

The previous commit missed updating webapp.js, I've pushed a fix with rs=mikedeboer over irc:

https://hg.mozilla.org/integration/fx-team/rev/6a27852628ea

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Note that any uplifts will want both commits.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a27852628ea

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

The two patches in here are code cleanup.  They only need to be uplifted if something that depends on them gets uplifted.

I believe the investigation results are now captured in other bugs and documentation.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Does not seem important enough to track.