974108 - Race condition in FxAccountsIACHelper

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

On Settings app startup, some fxa code is lazy loaded[1] in order to show the fxa account status inside the main settings panel.

Right now, the FxaMenu code only properly updates the main settings panel DOM on startup if I set a debugger, otherwise the getAccounts callback never fires. I suspect the immediate problem is that the FxAccountsIACHelper calls connect() with no callbacks at the end of its startup routine[2], but doesn't seem to keep track of connections vs requests, so that when FxaMenu calls getAccounts[3], the response is sent to the wrong (missing) callback?

Also, kgrandon mentioned that this code might be simpler if it were updated to use the evented IAC handler[4] instead of caching callbacks.

[1] https://github.com/6a68/gaia/blob/settings-app-WIP-rebased/apps/settings/js/firefox_accounts/menu_loader.js#L10-L13
[2] https://github.com/6a68/gaia/blob/settings-app-WIP-rebased/shared/js/fxa_iac_client.js#L180-L182
[3] https://github.com/6a68/gaia/blob/settings-app-WIP-rebased/apps/settings/js/firefox_accounts/menu.js#L18
[4] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/iac_handler.js

Comment 1

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

Attachment: github link

I've added the commit that fixes this to the branch[1] that holds the large settings app patch.

Kaze, do you mind reviewing along with the other code? Basically, with this additional commit applied, after the startup delay, the firefox accounts menu item in the main settings panel *does* correctly update the status on startup.

The specific commit is https://github.com/6a68/gaia/commit/ead0ba11. This attachment just links to the existing pull request. I didn't squash it into the gigantic settings commit, it's a separate commit on top.

Let me know if you'd like me to split it out into a totally separate pull request.

[1] https://github.com/6a68/gaia/tree/settings-app-WIP-rebased

Comment 2

[Fabien Cazenave [:kaze]]

Comment on attachment 8378727 [details] [review]
github link

LGTM, see coding style nitpicks on your PR.

(In reply to Jared Hirsch [:_6a68] from comment #1)
> Let me know if you'd like me to split it out into a totally separate pull
> request.

Yes please. Thanks for keeping it as a separate commit, but landing separate patches is saner and safer if we ever need to back-out one of them. Especially in this case, where the two patches don’t affect the same piece of code.

Clearing the r? flag, please ask for a review on a separate PR.

Comment 3

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

kaze: whoops! I thought I had unmarked r?, I must have only done that on 949051.

Yeah, this code is gnarly, it should be its own pull request.

The problem with the unit tests seems to be that
* fxa_iac_client.js gets sourced at the top, which triggers a connect() call
* we don't load the MockApp until inside the setup function

The old code mixed up connections, so it tolerated this. Now that we are properly queueing them, the MockApp isn't wired up in time, so the callbacks inside the MockApp are never called, so the tests are timing out.

Working on a fix still.

Comment 4

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

Attachment: github pull request 16541

Comment 5

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

By requiring the fxa_iac_client file *after* we setup the MockApp, and forcing mocha to wait till fxa_iac_client.js had loaded (by doing require('filename', done)), I've got the tests working with minimal changes. Yay!

Comment 6

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

Comment on attachment 8380127 [details] [review]
github pull request 16541

Please check my comment in github. And comments related to settings app were added in the pull request of bug 949051.

Comment 7

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

Comment on attachment 8380127 [details] [review]
github pull request 16541

Thanks for the great feedback, Arthur! Today was mostly meetings, but I did get the callback queueing fixed as requested.

Comment 8

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

Comment on attachment 8380127 [details] [review]
github pull request 16541

Looks good to me. Could you also add tests for the listener and callback queuing? Thanks!

Comment 9

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

Comment on attachment 8380127 [details] [review]
github pull request 16541

Tests added!

Comment 10

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

Comment on attachment 8380127 [details] [review]
github pull request 16541

The test seems not work. I've commented in github, could you check it? Thanks.

Comment 11

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

Hey Arthur - I'm at my wits' end with testing this fxa_iac_client.js file, I could use some input.

My changes to this code ensure that, during the period between the code loading and the port being ready, no messages/callbacks are dropped. And I'm sure this works, because exercising the menu/panel on the handset, the funky error cases aren't reproducible any longer.

The problem is that I can't figure out how to set up the unit tests to put the code in the pre-initialized state in a repeatable way. The current tests all insert a port and then start the testing, and although this helper does have reset() and init() methods, the init() method doesn't actually reset the port, and the connect() call is made *when the file loads*, not at init time, so that it's really tricky to separately (1) load the code and later (2) open a port.

I had really hoped to land this and the larger settings patch (bug 949051) by Monday, but writing the tests here seems to be holding everything up. I'd really appreciate any thoughts you have on the quickest way to get this landed :-\

Comment 12

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

lol, nothing like a good night's sleep to get some ideas around how to test this code.

clearing ni?, going to try to find a bit of time to hack later today

Comment 13

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

Hi Jared, I found the root cause that blocked the tests. Please see my comments in github and my modifications here (https://gist.github.com/crh0716/e3ec6f0881a5f878d964). Let me know if that makes sens to you, thanks!

Comment 14

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

:arthurcc Great suggestion! Playing with it now.

Comment 15

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

Comment on attachment 8380127 [details] [review]
github pull request 16541

Thanks a lot for your help with the tests, Arthur. Resubmitting :-)

Comment 16

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

Comment on attachment 8380127 [details] [review]
github pull request 16541

r=me. Thank you for the effort! Please squash the commits before merging.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

A Travis failure in test_persona_app.py test_persona_app.TestPersonaStandard.test_persona_standard_sign_in tells me this PR probably shouldn't be merged.

Comment 18

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

RyanVM: we had unrelated failures this afternoon due to a persona email server outage[1], would you mind restarting the Travis tests?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=986716

Comment 19

[Ryan VanderMeulen [:RyanVM]]

That failure was from a retrigger I did after Gaia was reopened (and after I merged your other PR).

Comment 20

[Ryan VanderMeulen [:RyanVM]]

But now it's green after another retrigger, so yay.

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

Comment 21

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

Awesome! Thanks Ryan :-)