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

RESOLVED FIXED

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dmose, Assigned: jaws)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36-)

Details

Attachments

(1 attachment, 4 obsolete attachments)

+++ 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").
> 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.
No longer blocks: 1120001
Depends on: 1120001
No longer blocks: 1119992
Depends on: 1119992
OS: Mac OS X → All
Hardware: x86 → All
QA Contact: dmose
Posted patch WIP Patch (obsolete) — Splinter Review
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8549074 - Attachment is obsolete: true
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8550431 - Attachment is obsolete: true
Attachment #8550465 - Flags: review?(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 ;
Attachment #8550465 - Flags: review?(standard8) → review+
(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.
Keywords: leave-open
Attachment #8551941 - Attachment description: Hoist Loop REST errnos and websocket reasons, patch=jaws,dmose, r=Standard8 → [landed fx-team] Hoist Loop REST errnos and websocket reasons, patch=jaws,dmose, r=Standard8
Attachment #8551941 - Flags: review+
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
Blocks: 1124365
Blocks: 1124384
Note that any uplifts will want both commits.
backlog: --- → Fx38+
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Summary: direct calls sometimes tell the user "something went wrong" when they should say "caller unavailable", part 2 → direct calls code that touches network state hard to reason about, has issues
Iteration: --- → 38.1 - 26 Jan
Does not seem important enough to track.
You need to log in before you can comment on or make changes to this bug.