982969 - Canceling sign-in flow should fire oncancel(), not onerror()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect)

Description

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

If the user cancels the sign-in flow, we should fire the oncancel() callback registered by the RP as part of the request() call (if any), not the onerror() callback.

Presently, we are firing errors like: {"error":"UI_ERROR","details":{"id":"{6db97549-ab00-7042-aca8-d1af39e88dc2}","error":"DIALOG_CLOSED_BY_USER"}}

More about request() and oncancel(): https://developer.mozilla.org/en-US/docs/Web/API/navigator.id.request

Comment 1

[Sam Penrose]

This looks like several small changes to a series of files to add oncancel to the various message and callback APIs. Also, it does not look like a blocker to me. I'm going to take it but assign it to the nice-to-haves, at the top of my personal queue. Jed could you let me know if this should remain a blocker for landing pref-ed off?

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I don't think it's a blocker for landing preffed-off, no.

Comment 3

[Sam Penrose]

I have now reversed position about 135° from comment 1. Firefox Accounts on FirefoxOS is basically a big set of multiplexers and de-multiplexers for IPC, concurrency, and semantic API channels. The mozFxAccounts*Event is single-channel. Promise-based APIs, notably in services/fxaccounts, are two-channel a la Unix return codes: resolve and reject. "Cancel" is kind of a sibling to that pair, not exactly reject aka "error" but not exactly not error either. We have roughly a third (fractional not ordinal), the receiving third, of a cancel channel implemented via:

  toolkit/identity/FirefoxAccounts.jsm:doCancel()
  dom/identity/DOMIdentity.jsm:doCancel()
    -- "cancel" multiplexed onto single-channel here --
  dom/identity/nsDOMIdentity.js:_onCancelRequestCallback()

but it's currently unused in favor of the error channel; hence this bug. I propose that rather than try to land the sending third:

  in the Gaia System App, replace error() here with cancel():
    apps/system/fxa/js/fxam_manager.js
    48:      window.parent.FxAccountsUI.error(error);
  [a couple more before we leave Gaia]

and an intermediate third in Gecko augmenting the current (Promise) error channel:

  b2g/components/FxAccountsUIGlue.js:onContentEvent()
    if (msg.error) ...
  services/fxaccounts/FxAccountsManager.jsm:_uiRequest()
    error => {return this._error(ERROR_UI_ERROR, error);}
  toolkit/identity/FirefoxAccounts.jsm:request()
    error => // now doError(), could branch to existing doCancel()

we instead grasp one of these horns:

  1) Sniff the Gaia error string in toolkit/identity/FirefoxAccounts.jsm and jump to the already-implemented-but-currently-unused cancel channel there. Yuk.

  2) Stay in the error channel until we get back to nsDOMIdentity.js, and sniff the Gaia string there. Also yuk, but gets my vote. We can delete the existing cancel channel, which doesn't serve any purpose in non-chrome Gecko that I can think of.

Magic strings from modules we're not supposed to know about stink. But rewriting a Promise-based cross-application library's API and control flow to avoid sniffing strings sucks worse IMO. Jed? Drive-by comments from others?

Comment 4

[Sam Penrose]

Attachment: 982969-detect-cancel-in-error.patch

This patch implements grasping the second horn of yuk. If we're comfortable with that direction, I'll take the onCancel path out of DOMIdentity.js and toolkit/identity/FirefoxAccounts.jsm

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8407084 [details] [diff] [review]
982969-detect-cancel-in-error.patch

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

I'm still pondering this gnarly situation.  But for the patch in question, don't remove the oncancel message handler.

More to come ...

::: dom/identity/nsDOMIdentity.js
@@ -505,5 @@
>          if (this._rpWatcher.onready) {
>            this._rpWatcher.onready();
>          }
>          break;
> -      case "Identity:RP:Watch:OnCancel":

Actually, you don't want to remove this.  Persona uses it.

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I'm beginning to agree with your 3π/4 radians reversal.

Agreed: We could add code to handle oncancel in fxam_ui.js, fxam_manager.js, system/js/fxa_ui.js, so that fxa_iac_client.js would ultimately have success, error, and cancel callbacks for each flow.  But when it comes time to shout a mozFxAccountsRPContentEvent down to gecko, FxAccountsUIGlue.js distills those messages into promise acceptances and rejections, and the rest of fxa speaks in terms of promises as well.

So we can either rewrite all of that or, as you suggest, figure out how to map 'cancel' into a promise rejection.  So let's say when 'cancel' happens, we continue to reject with the DIALOG_CLOSED_BY_USER string.

The only place where we have 'oncancel' in the BrowserID protocol is as an optional parameter to request() [1].  So we only have to deal with this in one part of the DOM API.

Is it possible then that all we need to do is test for that string in the FirefoxAccounts toolkit module [2], like so?

    // handle promise rejection for getAssertion, i.e., request
    error => {
      log.error("get assertion failed: " + JSON.stringify(error));
      if (error == "DIALOG_CLOSED_BY_USER") {
        return this.doCancel(aRPId);
      }
      this.doError(aRPId, error);
    }

[1] https://developer.mozilla.org/en-US/docs/Web/API/navigator.id.request#Syntax
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/identity/FirefoxAccounts.jsm#154

Comment 7

[Sam Penrose]

Yes, although I think we'd still want to rewrite the message reception in nsDOMIdentity to convert a cancel-without-handler to an error, no?

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

How about if in the place where we catch "Identity:RP:Watch:OnCancel", if (!this._onCancelRequestCallback), we print a warning in the console that the app isn't handling oncancel.

If they don't care enough about cancel events to listen for them, I don't think we should force them down their error path.  That could cause UI to surface or make other disruptive things happen, to prevent which they in turn would have to know about the magic DIALOG_CLOSED_BY_USER string, and so the plague would spread.

Comment 9

[Sam Penrose]

Attachment: 982969-detect-cancel-in-error.patch

This patch follows your suggestion. I'd be curious to know if you see log output from FirefoxAccounts.jsm. I was not able to (and I tried replacing the local LogUtils). Thanks Jed!

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #6)
 
> Is it possible then that all we need to do is test for that string in the
> FirefoxAccounts toolkit module [2], like so?
> 
>     // handle promise rejection for getAssertion, i.e., request
>     error => {
>       log.error("get assertion failed: " + JSON.stringify(error));
>       if (error == "DIALOG_CLOSED_BY_USER") {
>         return this.doCancel(aRPId);
>       }
>       this.doError(aRPId, error);
>     }
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/API/navigator.id.request#Syntax
> [2]
> https://mxr.mozilla.org/mozilla-central/source/toolkit/identity/
> FirefoxAccounts.jsm#154

FWIW I believe this is the way to solve this issue.

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

> FWIW I believe this is the way to solve this issue.

Thanks for chiming in, ferjm.  It's always reassuring to hear from you.

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8421497 [details] [diff] [review]
982969-detect-cancel-in-error.patch

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

Looks good to me, Sam.  Thanks for your work on this!

::: b2g/components/FxAccountsMgmtService.jsm
@@ +79,5 @@
>      }
>  
> +    if (msg.error) {
> +      self._onReject(msg.id, msg.error);
> +      return

nit - missing a semicolon

::: toolkit/identity/FirefoxAccounts.jsm
@@ +241,5 @@
>  
>    doError: function doError(aRpCallerId, aError) {
>      let rp = this._rpFlows.get(aRpCallerId);
>      if (!rp) {
> +      log.warn("doError found no rp to go with callerId " + aRpCallerId + "\n");

I know it was already there, but I don't think we need the trailing newline in the log call.  Looks like a dump() was morphed to log.warn() at some point.  Quite possible it's my handiwork :)

Comment 13

[Sam Penrose]

Attachment: 982969-detect-cancel-in-error.patch

https://tbpl.mozilla.org/?tree=Try&rev=f61036895c05

Nits fixed; thanks Jed!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/249b516e02d3

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/249b516e02d3