Closed Bug 1027950 Opened 5 years ago Closed 5 years ago
Find My Device panel - don't use onerror to attach handlers
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.
Hey Bhavana, This is the same symptoms as bug 1027422, I just misdiagnosed it the first time :-P
Assignee: nobody → 6a68
blocking-b2g: --- → 2.0?
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
blocking-b2g: 2.0? → 2.0+
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?
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
Flags: needinfo?(6a68) → needinfo?(arthur.chen)
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".
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
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 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 on attachment 8443173 [details] [review] Github PR 20774 r=me with the nits addressed, thank you!
Attachment #8443173 - Flags: review?(arthur.chen) → review+
Gaia-try looks good, merging. Master: https://github.com/mozilla-b2g/gaia/commit/b3324d031fe91b864090461ffcacc6ca605a2903  https://tbpl.mozilla.org/?tree=Gaia-Try&rev=0ec9122836fe5512acc779eb882edc1b8eb02d3e
You need to log in before you can comment on or make changes to this bug.