[HOLD #34289] [$2000] The app crashes when the user is logged into multiple tabs and logs out of one of the tabs

[techievivek]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

• Open newDot in 2 different tabs.
• Log out from one of the tabs and observe that in other tab you can see the app crashes.

Expected Result:

The user should have been logged out in the other tab

Actual Result:

The app actually crashes.

Workaround:

No workaround and this needs to be fixed ASAP.

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: v1.2.74-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL: Issue reported by: @hungvu193 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674651616948819, https://expensify.slack.com/archives/C9YU7BX5M/p1676990369948349

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016764b6d950f13143
• Upwork Job ID: 1628628211642437632
• Last Price Increase: 2023-04-12

[MelvinBot]

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

Triggered auto assignment to @roryabraham (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[johncschuster]

Hm... I may have jumped the gun here. I just tried reproducing the behavior in an incognito window, and was not able to reproduce it.

@techievivek, what am I doing wrong here?

https://user-images.githubusercontent.com/5579997/220772607-df35f26b-8642-400d-834b-c42415289f4b.mp4

[roryabraham]

@johncschuster The problem is probably that you were using an incognito window, so the other site is not sharing the same IndexedDB database. When we have the site running in multiple non-incognito tabs, those tabs are both sharing the same data. The same is not true when the site is running in a normal window and in an incognito window. That's why if you're signed in in a normal window and open the site in a new tab, it will pop right up. But if you open it in an incognito window, you'll have to log in again.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~016764b6d950f13143

[MelvinBot]

Current assignee @johncschuster is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

[MelvinBot]

Current assignee @roryabraham is eligible for the External assigner, not assigning anyone new.

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When we log into 2 tabs in normal browser (not cognito) and log out in 1 tab, the other tab will crash.

What is the root cause of that problem?

When we logout, we clear the network state momentarily and network become undefined, while many components in our app expects it to be defined, causing the crash when a property like network.isOffline is accessed.

What changes do you think we should make in order to solve the problem?

In https://github.com/Expensify/App/blob/33408b80acfaf0e9b54aa59fe6fd247e188bab37/src/components/createOnyxContext.js#L33 We can set add a new defaultValue for createOnyxContext, if the value of the OnyxKey is undefined then we'll fallback to that defaultValue.

For example we can do:

if (propsToPass[propName] === undefined && defaultValue) {
                        propsToPass[propName] = defaultValue;
                    }

And we can set {} or {isOffline: false} as the defaultValue of network so that when Onyx clears the network value and reinitialize it again, it will not cause the crash.

What alternative solutions did you explore? (Optional)

NA

[tienifr]

I managed to dig deeper and found the actual root cause which is in react-native-onyx

Proposal 2

Please re-state the problem that we are trying to solve in this issue.

When we log into 2 tabs in normal browser (not cognito) and log out in 1 tab, the other tab will crash.

What is the root cause of that problem?

The root cause is in react-native-onyx, here https://github.com/Expensify/react-native-onyx/blob/93bb6ee9e84d1980041088c5f5d17bf70abbb148/lib/Onyx.js#L1115

We have the defaultKeyValuePairs and keyValuesToPreserve which we must maintain in the state at all cost, as explained here https://github.com/Expensify/react-native-onyx/blob/93bb6ee9e84d1980041088c5f5d17bf70abbb148/lib/Onyx.js#L1060.

The things that are happening is:

1. We logout in 1 tab
2. We call Storage.clear() which clears everything from the storage, and sync to all other tabs that those values are cleared https://github.com/Expensify/react-native-onyx/blob/f6312e843d3369b2e241ca232ddb984e7e2d40f5/lib/storage/WebStorage.js#L49
3. One of those key that was cleared (and synced) happens to be network, which is a required field for most of the pages in the app to function
4. The other tab receive the sync network becomes null, and crash
5. Then we Storage.multiSet() to set the defaultKeyValuePairs and keyValuesToPreserve and sync those values, but now it's too late since the other tab already crashes.

What changes do you think we should make in order to solve the problem?

To fix this, we need to change how we clear the values:

1. Get all the keys in storage
2. Get the keys that are actually supposed to be cleared (the ones in defaultKeyValuePairs and keyValuesToPreserve should not be cleared)
3. Clear those keys (and the cleared keys will be synced correctly in other tabs)
4. Call Storage.multiSet() to set the defaultKeyValuePairs and keyValuesToPreserve (which will be synced correctly in other tabs)

As we can see the required keys like network are never cleared (as we intended to) and will not cause the crash.

What alternative solutions did you explore? (Optional)

NA

Result

Working well after the fix:

https://user-images.githubusercontent.com/113963320/220896150-ed9b199b-5af8-4e79-bdf9-37dedc6e1aca.mov

[fedirjh]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The problem we are trying to solve is that the app is crashing instead of logging out the user in another tab, as expected.

What is the root cause of that problem?

Onyx.clear is failing to reset the initial value of the network key in the 2nd tab in Onyx cache layer :

1. Onyx.clear is called in tab 1
2. Onyx.clear generates key-value pairs to preserve
3. Storage.clear is called to clear the storage
4. raiseStorageSyncEvent is called to notify other tabs about the changes , bug occurs in this step
5. Storage.multiSet is called to resets the generated key-value pairs in step 2

The bug : raiseStorageSyncEvent is called to notify other tabs about the changes before resetting the generated key-value pairs in step 5. Since the raiseStorageSyncEvent retrieves the value from the storage in this line , which was cleared in step 3, all values will be null in step 4. The result is that the network key being removed from Onyx cache in the 2nd tab, causing the app to crash.

            Storage.getItem(onyxKey)
                .then(value => onStorageKeyChanged(onyxKey, value));

What changes do you think we should make in order to solve the problem?

The bug can be fixed by ensuring that step 4 is called after resetting the key-value pairs in step 5. We can pass a callback to Storage.clear that will be executed to reset key-value pairs before the sync event.

In Onyx.js

Onyx.clear = () => {
  // Onyx clear logic

  return Storage.clear(() => Storage.multiSet([...defaultKeyValuePairs, ...keyValuesToPreserve]));
};

In

        this.clear = (callback) => {
            let allKeys;

            // They keys must be retreived before storage is cleared or else the list of keys would be empty
            return Storage.getAllKeys()
                .then((keys) => {
                    allKeys = keys;
                })
                .then(() => Storage.clear())
                .then(() = callback()) // add this line
                .then(() => {
                    // Now that storage is cleared, the storage sync event can happen which is a more atomic action
                    // for other browser tabs
                    _.each(allKeys, raiseStorageSyncEvent);
                });
        };

What alternative solutions did you explore? (Optional)

Solution 2 we can pass the keys to preserve to the Storage.clear method and only sync the keysToSync , keys to preserve will be synced with multiSet

In Onyx.js

const keysToPreserve = _.union(keysToPreserve, defaultKeys); // add this line
return Storage.clear(keysToPreserve) // update this line
                .then(() => Storage.multiSet([...defaultKeyValuePairs, ...keyValuesToPreserve]));

In WebStorage.js

        this.clear = (keysToPreserve) => { // update this line
            let allKeys;

            // They keys must be retreived before storage is cleared or else the list of keys would be empty
            return Storage.getAllKeys()
                .then((keys) => {
                    allKeys = keys;
                })
                .then(() => Storage.clear())
                .then(() => {

                    const keysToSync = _.difference(allKeys, keysToPreserve); // add this line

                    // Now that storage is cleared, the storage sync event can happen which is a more atomic action
                    // for other browser tabs
                    _.each(keysToSync, raiseStorageSyncEvent); // update this line
                });
        };

[MelvinBot]

📣 @fedirjh! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[sobitneupane]

@tienifr Thanks for the proposal.

Your proposal looks good and well-explained. I think the better solution will be to call Storage.clear() > Storage.multiset() > raiseStorageSyncEvent as proposed by @fedirjh.

Please let me know if you disagree.

[tienifr]

@sobitneupane thanks for the review!

Both approaches are quite similar and the difference is only in the implementation detail. I agree the sequence Storage.clear() > Storage.multiset() > raiseStorageSyncEvent seems more understandable, but I don't prefer it because:

• It actually still clears the required-to-be-preserved keys, and the only thing that makes it work is that it only syncs to the other tabs after it already resets the keys (sounds workaround-ish). What if there's another process that happens to read the required-to-be-preserved keys after Storage.clear() but before Storage.multiset() finishes executing? It might cause unexpected behaviors.
• It will trigger the sync for the required-to-be-preserved keys twice which are unnecessary, once when multiSet is called and once after the clearing finishes (in the line _.each(allKeys, raiseStorageSyncEvent);)

[sobitneupane]

@fedirjh Thanks for the proposal.

Your proposal looks good.

@roryabraham The proposed change should be made in react-native-onyx. The proposal is to change order in Onyx.clear

🎀👀🎀 C+ reviewed

[sobitneupane]

@tienifr Thanks for your response. The first issue you mentioned doesn't look probable.

[tienifr]

I think part of the reason why this bug happens is because we're commenting The only keys that should not be cleared are... but we actually implement We actually still clear it but we'll restore it immediately no worries. So IMO we should avoid having that same approach in the fix.

[tienifr]

@sobitneupane Just curious if you see any other benefits from @fedirjh's proposal other than that it might look cleaner when implemented?

[sobitneupane]

@tienifr I think the approach suggested in this proposal is the better way to solve the issue. I asked you if you think otherwise, but the reasons for your disagreement with the proposal doesn't look strong to me. But I am okay to go with another proposal if @roryabraham thinks otherwise.

[fedirjh]

Get the keys that are actually supposed to be cleared (the ones in defaultKeyValuePairs and keyValuesToPreserve should not be cleared)

@tienifr your proposed solution was implemented in the past. This PR https://github.com/Expensify/react-native-onyx/pull/220 introduced the changes to use localForage.clear method to fix this issue https://github.com/Expensify/App/issues/13884 . So I don’t think it’s wise to revert the logic here and use the old implementation , note that localForage.clear will remove all the keys.

It will trigger the sync for the required-to-be-preserved keys twice which are unnecessary, once when multiSet is called and once after the clearing finishes (in the line _.each(allKeys, raiseStorageSyncEvent);)

@sobitneupane i have included another option to fix this , I updated my proposal here

[hungvu193]

Just comment here the Pr, I think is the cause of this issue:

https://github.com/Expensify/App/pull/13384

[sobitneupane]

Thanks for the update @fedirjh. Do you think your alternative proposal is better than the initial one? Is there any significant difference in performance between the proposals?

[fedirjh]

Is there any significant difference in performance between the proposals?

I don't think there is any significant difference in performance between the proposals. However, the second approach will not require any changes to the SQLite provider.

I may ask @tgolen for his thoughts since he has some background.

[tgolen]

Thanks for pinging me on this! I looked at both proposals, and the one that I like the most is "Proposal 1" from @fedirjh. That one makes the most sense in my mind, and is related to this discussion (please read through this to make sure you understand it).

As I was reading the proposals, one question kept sticking out in my mind...

Why are we not just updating the code to better handle the network prop missing and preventing the crash?

This seems much more simple and doesn't need to change the internal working of how Onyx.clear() works.

[tienifr]

Why are we not just updating the code to better handle the network prop missing and preventing the crash?

@tgolen that can fix the issue as well. My first proposal here suggests one way to do it.

cc @sobitneupane

[tgolen]

Ah, OK. Great. I missed that proposal, sorry! Maybe we should do both? It seems harmful that something would cause the code to crash like that, so we fix the crash reasons... while we also fix the underlying issue of the storage event firing at an inopportune time.

[roryabraham]

Thanks for the discussion on this issue, everyone. Here's my interpretation.

Problem:

The problem has been well-established by @tienifr and @fedirjh, but here's a summary in my own words.

When we call Onyx.clear, on all platforms we completely clear the storage database, then add back the keys we want to keep. On web, this creates a problem because all subscribers are notified after the database is cleared but before the data we want to preserve is added back into the database.

As a result, other tabs see data that is never supposed to be cleared from Onyx (i.e: keysToPreserve and default key states) being cleared.

The part of the solution we can all agree on™

As @tgolen touched upon, this specific issue is occurring because the app crashes when the network Onyx data disappears. I think we can all agree that ideally the app should be refactored to handle that more gracefully such that it doesn't crash if the network data disappears.

I think @tienifr's proposal to provide a defaultValue to createOnyxContext when creating the withNetwork HOC sounds like a good way to address this 👍🏼

Addressing the root cause: @fedirjh's first proposal

My high-level summary of this proposed solution is to:

Patch WebStorage.clear to accept a callback so that we can include other side-effects in WebStorage.clear (in this case, setting the keys and values that we want to preserve) before notifying subscribers of the change.

Addressing the root cause: @tienifr's proposal

In short, this proposed solution is to prevent keys that we want to keep from being cleared from storage in the first place.

---

Overall, I think that I prefer @tienifr's proposal to address the root problem. I think he made some reasonable arguments in this comment

1. It addresses the problem in a platform-agnostic way
2. It reduces the potential for race conditions.
3. It seems more efficient – we only need to write to storage once to remove the keys we don't want, rather than writing to storage twice (once to clear and then once to add the keys we want back)
4. I think the end-result will be simpler and more intuitive. I'm worried that introducing this side-effect into WebStorage.clear will lead to other side-effects being added in different places in the storage layer, which is something I would prefer to avoid.

Given that, I'm going to assign this issue to @tienifr. Thanks for the input everyone 👍🏼

[MelvinBot]

📣 @tienifr You have been assigned to this job by @roryabraham! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[tienifr]

@sobitneupane @roryabraham The PR to address the root cause in react-native-onyx is ready for review.

I am creating a PR in this repository to add the default value after the PR in react-native-onyx is completed.

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr Huh... This is 4 days overdue. Who can take care of this?

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr 10 days overdue. I'm getting more depressed than Marvin.

[sobitneupane]

We are working on the PR. It should be merged pretty soon.

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sobitneupane]

PR on 'react-native-onyx' merged.

[tienifr]

@sobitneupane PR https://github.com/Expensify/App/pull/15556 is ready for review. Thanks

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr Huh... This is 4 days overdue. Who can take care of this?

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[MelvinBot]

@johncschuster, @sobitneupane, @roryabraham, @tienifr 12 days overdue. Walking. Toward. The. Light...

[sobitneupane]

@tienifr Any update?

[tienifr]

@sobitneupane I think we should wait this PR: https://github.com/Expensify/App/pull/15130 is merged. Because we're having the problem with "react-native-quick-sqlite": "^8.0.0-beta.2" on android.

[roryabraham]

https://github.com/Expensify/App/pull/15130 has been merged, so I think the next up is https://github.com/Expensify/App/pull/16531, then https://github.com/Expensify/App/pull/15556. Thanks for your persistence here!

[roryabraham]

Also, I looked back at https://github.com/Expensify/react-native-onyx/pull/237 and it went through a number of iterations, so I've decided to double the bounty here.

[MelvinBot]

Upwork job price has been updated to $2000

[johncschuster]

Note for me: It looks like the Upwork job has closed. I will need to recreate the job to pay out this issue. 👍

[MelvinBot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.