[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 [email protected] 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.

Screen.Recording.2023-02-21.at.9.54.21.PM.mov

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
• 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)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• 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?

2023-02-22_15-57-10.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

App/src/components/createOnyxContext.js

Line 33 in 33408b8

const propsToPass = {

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:

Screen.Recording.2023-02-23.at.18.40.15.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.

[koko57]

Not really - I've applied these changes in Onyx
and now I need to apply the changes in the App mentioned in this comment: #15321 (comment)

I'm a bit confused with the onClear method - we should apply this one in the app, but in the Onyx code we're subscribe to the ON_CLEAR type of data, but we're never sending this kind of message to the Broadcast. I think it should be sent here https://github.com/BeeMargarida/react-native-onyx/blob/c72152c7f894474b01655e09fc8a8ff1c5c99b9b/lib/Onyx.js#L1395 instead of sending CLEAR message again.

For the 3rd change - the changes have been already applied, for the 4th - I wonder if this point is still valid.

I've already tested the changes in the app - they were working but for HT accounts signing out was really slow. I'll test it once again with the app changes.

If everything works fine I will create a PR to Onyx and a PR to App.

[koko57]

Unfortunately, the more I test this solution the less I'm convinced to apply it - when signing out the Leader tab correctly redirects to sign-in page but the other tabs are "frozen" for a long time (especially for HT accounts). When changing Storage.removeItems to Storage.clear the screen for non-leader tabs goes blank. Ana's changes were made a long time ago, before batched updates were introduced, I think I need to figure out a new solution now and maybe I need to consult it with the performance team.

For now we can safely merge Chris PR with bumping the version.

[roryabraham]

We are also starting to look more closely at using SQLite's wasm build on the web, and there are probably some SQLite features we could use to synchronize writes across tabs. The SQLite wasm build also depends on OPFS, which they told us operates much more predictably across multiple parallel instances than IndexedDB

[koko57]

@roryabraham I've came up with a few possible solutions so far, tested them, but none of them seem to be working decently. I'm still thinking of any new solution and tweaking the code. I saw that bumping Onyx caused a few other issues: #33554 (comment) What are the next steps then until I find a working solution? Should we revert the PR with the Broadcast + moving AtiveClientManager to Onyx?

[chrispader]

@roryabraham Should we revert the PR with the Broadcast + moving AtiveClientManager to Onyx?

hey @roryabraham and @koko57. I'm currently working on bumping Onyx in E/App again after the revert and was therefore working on fixing the regressions that the previous bump caused.

As mentioned here, this feature/PR caused at least 1-2 regressions.

I've started working on fixes for these regressions in Expensify/react-native-onyx#455 (comment), though i'm thinking it might be better, to revert Expensify/react-native-onyx#382 and exclude it from the bump and just bump Onyx with my changes from Expensify/react-native-onyx#437 and all the other prettier PRs and small fixes.

cc @mountiny

[chrispader]

One issue with Expensify/react-native-onyx#382 was definitely, that the wrong parameters were passed to multiSet in subscribeToEvents.

The other changes i made to fix #34575, as when the other (non-leader) tabs weren't updated e.g. on Onyx.clear, and therefore froze... @koko57 is this something you're planning to fix with a different solution?

In general, do we need to call functions like scheduleSubscriberUpdate, broadcastUpdate and scheduleNotifyCollectionSubscribers on all tabs? Since we want all tabs to receive the updates and re-render the UI accordingly.

Also, i experienced issues with Onyx in other (non-leader) tabs, because the cache is stale in non-leader tabs... We'll also have to think about a solution for this i guess

[chrispader]

We are also starting to look more closely at using SQLite's wasm build on the web, and there are probably some SQLite features we could use to synchronize writes across tabs. The SQLite wasm build also depends on OPFS, which they told us operates much more predictably across multiple parallel instances than IndexedDB

I'm also going to be working on this feature. Should we prioritize the SQLite wasm build, before putting more work into fixing Expensify/react-native-onyx#382? cc @roryabraham

[koko57]

@chrispader thanks for taking care of this one! I also think it would be better to revert Expensify/react-native-onyx#382, apply only your changes and bump the version.

[chrispader]

Created a PR to revert these changes here: Expensify/react-native-onyx#458

[koko57]

We are also starting to look more closely at using SQLite's wasm build on the web, and there are probably some SQLite features we could use to synchronize writes across tabs. The SQLite wasm build also depends on OPFS, which they told us operates much more predictably across multiple parallel instances than IndexedDB

I'm also going to be working on this feature. Should we prioritize the SQLite wasm build, before putting more work into fixing Expensify/react-native-onyx#382? cc @roryabraham

@roryabraham @chrispader I'm waiting on your decision on what should be prioritized

[mountiny]

SQLite sounds more important to me

[roryabraham]

I apologize for the lackluster communication on my part. I think that since Expensify/react-native-onyx#382 caused multiple regressions, we should revert it. @chrispader, you should focus on SQLite-wasm over the the cross-tab synchronized writes.

It's not 100% clear to me if SQLite-wasm will fix the multi-tab concurrency issues, but it feels like maybe it could. Do multiple tabs share the same database? This seems like something we can discuss further with SQLite and try to get a solid plan for.

So let's revert Expensify/react-native-onyx#382 then put this on HOLD for SQLite-wasm

[chrispader]

I apologize for the lackluster communication on my part. I think that since Expensify/react-native-onyx#382 caused multiple regressions, we should revert it. @chrispader, you should focus on SQLite-wasm over the the cross-tab synchronized writes.

It's not 100% clear to me if SQLite-wasm will fix the multi-tab concurrency issues, but it feels like maybe it could. Do multiple tabs share the same database? This seems like something we can discuss further with SQLite and try to get a solid plan for.

Got it. Going to start research and implementation tmrw and keep you guys updated in #34289

So let's revert Expensify/react-native-onyx#382 then put this on HOLD for SQLite-wasm

Revert PR already in the works 👍

[roryabraham]

Putting this on HOLD for #34289. One idea that's maybe crazy or maybe just makes sense is to use bedrock to synchronize SQLite across tabs. I'm really not too sure how the data is shared between tabs in SQLite or how it might need to be replicated.

I think using leader election to ensure that only one tab is doing writes makes sense at a high level, not sure what went wrong with out implementation because it was pretty thoroughly tested. Maybe when adding it to E/App we left the old ActiveClientManager in place and the two were conflicting with eachother?

[kacper-mikolajczak]

It's not 100% clear to me if SQLite-wasm will fix the multi-tab concurrency issues, but it feels like maybe it could. Do multiple tabs share the same database? This seems like something we can discuss further with SQLite and try to get a solid plan for.

For keeping the database synchronised, we could create a single instance of it in separate context (e.g. Worker) as a single source of truth and then access it from multiple tabs. I am not sure if SharedWorker API is sufficient for our needs.

It would be cool to have the sync layer OOTB with SQLite.wasm, but let me know what you think of above as kind of an alternative 👍

CC: @roryabraham @chrispader

[koko57]

Maybe when adding it to E/App we left the old ActiveClientManager in place and the two were conflicting with each other?

No, unfortunately it wasn't that. Even with applying ActiveClientManager from Onyx the issues are still reproducible.

[chrispader]

I gave an update on the SQLite WASM idea in #34289 (comment).

I'm currently also investigating if the extra worker thread could be directly used to sync multiple tabs, or if we need an extra worker or some other logic to keep tabs in sync...

[melvin-bot]

This issue has not been updated in over 15 days. @trjExpensify, @sobitneupane, @koko57, @roryabraham, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[trjExpensify]

Still held, Melv.

[melvin-bot]

@trjExpensify, @sobitneupane, @koko57, @roryabraham, @tienifr, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

[roryabraham]

This issue this was on HOLD for was closed as not planned for now. Not sure if multi-tab support is worth fixing now or if we should just leave this as closed