1027950 - Find My Device panel - don't use onerror to attach handlers

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Original problem (fixed by bug 1021428): we were attaching button click listener inside mozId.watch() onready callback. If onerror fires instead of onready, say, due to the phone being offline, the button would never work. This was the root cause of bug 1021428.

I fixed that bug incorrectly, adding a button listener inside the onerror callback as well as the onready callback. It's possible for onerror to fire more than once, for instance, if the user opens the fxa dialog, then closes it. If a button handler is added each time onerror fires, we might attach a ton of handlers.

I previously thought the problem was FxA firing onready() repeatedly, which would be incorrect (bug 1027422), but it's actually firing onerror() repeatedly, which is correct.

Solution: 
The simplest fix is to just naively wire up the button listener.

navigator.mozId.watch() doesn't depend on remote resources, so it should always load very quickly. The button won't work until watch() has returned, but if the user clicks before watch() has returned, nothing bad will happen. The user will probably just wait a second and click the button again, and then it'll work.

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Bhavana,

This is the same symptoms as bug 1027422, I just misdiagnosed it the first time :-P

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github PR 20774

Hi Arthur,

This is a small fix for a regression caused by the patch for bug 1021429. It's blocking but doesn't involve strings. Do you have time to take a look?

Thanks very much,

Jared

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Although onready returns quickly and seems no harm for now but this assumption adds uncertainty and makes the implementation less robust. Using `onclick` might be an easier option, what do you think?

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi Arthur,

I'm not quite sure what you mean in comment 3. The 'onready' in this patch is a callback fired by the firefox accounts navigator.mozId.watch() method, not a DOM event. The overall effect of the patch actually is to move the click handler outside of the 'onready' callback, ensuring it's always attached exactly once. Does this make sense?

Thanks!

Jared

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

I meant we could use "loginButton.onclick = self._onLoginClick.bind(self);" in "onready" and "onerror". This also ensures that there is only one handler attached. In addition to that, the handler is guaranteed to be called after "onready" and "onerror".

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Arthur,

Yeah, so "onerror" is called every time you open, then close, the fxa dialog. This is the source of the current bug: I misunderstood when it was invoked.

We could certainly do something more complex than naively wire up the button. I could either wait for a promise/deferred to resolve before wiring up the button, or use an object flag to implement a hacky "only fire once" implicit promise.

My thought here was, it's just UI code, and navigator.mozId doesn't invoke any code hosted on the network--it's in the gecko layer. So, worst case, the user clicks the button, nothing happens, they click again (a few seconds later), and the overlay is displayed.

I'm happy to go with something fancier, if you'd rather - I just wanted to be clear that implementing "attach only when ready, and exactly once" semantics is a bit involved in this case.

Thanks for the feedback! Let me know how you'd like me to proceed.

Jared

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Jared,

My intention was to avoid the unresponsiveness when clicking buttons. I understand that navigator.mozId.watch() should return very quickly, but it may change when the gecko code changes or depends on the running devices. It also has potential risks on calling to "onLoginClick" before ready. Although we don't call to something relying on the ready of navigator.mozId.watch now but it may introduce potential bugs in the future.

Therefore, I would suggest to disable the button by default so that the users know it is not ready and won't get frustrated by unresponsiveness (sorry I did not point this out). And when `onready` and `onerror`, enable the button and attach the handler. Does that make sense?

Arthur

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8443173 [details] [review]
Github PR 20774

Hey Arthur,

Thanks for explaining your concerns with this patch. I've updated it to disable the button until watch() returns, and I've also added some tests.

Mind taking a look at the updated patch?

Thanks,

Jared

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8443173 [details] [review]
Github PR 20774

r=me with the nits addressed, thank you!

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Gaia-try looks good[1], merging.

Master: https://github.com/mozilla-b2g/gaia/commit/b3324d031fe91b864090461ffcacc6ca605a2903



[1] https://tbpl.mozilla.org/?tree=Gaia-Try&rev=0ec9122836fe5512acc779eb882edc1b8eb02d3e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/0266c326c5ba573457f3c10de3e94305a7fdfbbe