App: Signing in with `exitTo` doesn't work

I was able to reproduce it only with switching tab and coming back to the sign in tab. I’m not sure if others, who reproduced it, switched the tab in the process, but this flow has 100% reproducibility (thanks, @TheRohitSharma).

There was a post in the Slack about new steps for contributors:

1. Hired contributors post their solution proposals directly on the GitHub issue
2. Expensify reviews and provides feedback on proposal on GH issue
3. Contributors move forward with creating PRs

Even though I’m itching to write the pull request, I’ll post the solution first.

Steps to reproduce

1. Login to the web app
2. Navigate to any chat
3. Click “Sign Out”
4. Switch to other tab in the browser
5. Switch back to the Sign In page
6. Wait for 3 seconds (I know, right?)
7. Enter the email, press “Continue”
8. Confirm that sign in hangs and doesn’t continue

The root cause

This is a combination of two issues.

When ReportActionsView is mounted, it adds an event listener for the app visibility change (remember the tab switching?)

    componentDidMount() {
        this.visibilityChangeEvent = AppState.addEventListener('change', () => {
            if (this.props.isActiveReport && Visibility.isVisible()) {
                this.timers.push(setTimeout(this.recordMaxAction, 3000));
            }
        });

So on each app visibility change, the function recordMaxAction will be called, and it will send the request to the server to update last read action id.

And when ReportActionsView is unmounting, it should remove this listener:

    componentWillUnmount() {
        if (this.visibilityChangeEvent) {
            this.visibilityChangeEvent.remove();
        }

The problem is AppState.addEventListener doesn’t really return anything, so this.visibilityChangeEvent is undefined after subscription to the event. It means that componentWillUnmount doesn’t remove the listener.

The listener is still active after the sign out.

When user switches to other tab and returns back to the sign in, the event triggers. After 3 seconds timeout, it sends the request. But user is signed out, so server responds with 407 code. API request function calls handleExpiredAuthToken to try to relogin user.

function handleExpiredAuthToken(originalResponse, originalCommand, originalParameters, originalType) {
    // Prevent any more requests from being processed while authentication happens
    Network.pauseRequestQueue();
    isAuthenticating = true;

    Authenticate({
        useExpensifyLogin: false,
        partnerName: CONFIG.EXPENSIFY.PARTNER_NAME,
        partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
        partnerUserID: credentials.autoGeneratedLogin,
        partnerUserSecret: credentials.autoGeneratedPassword,
    })

This is the place where other issue happens. handleExpiredAuthToken pauses the network queue and tries to send Authenticate request, using saved credentials. But user is signed out. So credentials object is null. So accessing credentials.autoGeneratedLogin and credentials.autoGeneratedPassword throws an exception, which is not caught.

So handleExpiredAuthToken ends prematurely. But before it tried to send the request, it paused the Network Queue. And after the exception the queue is still on pause.

Now user enters the email and presses “Continue” button.

fetchAccountDetails request is added to the queue, but queue is on pause, so request will never be sent and “Continue” button forever hangs in loading state.

https://user-images.githubusercontent.com/130532/103470075-10e73680-4d76-11eb-8578-94283f4d5c95.mp4

The fix

The fix should include:

1. The null check for credentials object, or something like this:

    const authCredentials = credentials || {};

    Authenticate({
        useExpensifyLogin: false,
        partnerName: CONFIG.EXPENSIFY.PARTNER_NAME,
        partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
        partnerUserID: authCredentials.autoGeneratedLogin,
        partnerUserSecret: authCredentials.autoGeneratedPassword,
    })

1. Proper event listener removal in ReportActionsView:

    // Should have .bind(this) in constructor
    onVisibilityChange() {
        if (this.props.isActiveReport && Visibility.isVisible()) {
            this.timers.push(setTimeout(this.recordMaxAction, 3000));
        }
    }

    componentDidMount() {
        AppState.addEventListener('change', this.onVisibilityChange);
    }

    componentWillUnmount() {
        AppState.removeEventListener('change', this.onVisibilityChange);
    }

Prevention of future issues

I’m not sure about it yet.

1. I could put the request in handleExpiredAuthToken to try-catch block, but we already have the catch for the promise. So this approach would require some duplication of the catch code, because promise rejection will not be caught by try-catch.

2. I could wrap the whole function content with Promise.try(() => { ... }); and add the only one parent catch, which would handle both promise rejections and params errors. That would be cleaner, but looks a bit weird to me.

3. The cleanest way would be async/await with try/catch, but you don’t use it anywhere in your code. So that probably makes the №2 approach the winner.