WIP: Undetermenistic internal state changes causes freezes and unexpected feature breakage all over

[greg-nagy]

We had some mysterious bugs before, and the common theme for them is the icon of the token is switching from the proper asset icon to the default placeholder. This change frequently happens on nondeterministic time intervals.

In previous situations, the send transaction failed because it went to the wrong address and swap was also affected. Do you have the exact context @Naxsun or @VladUXUI? It was a pool swap and a separate instance where the send tx seemed to go to the contract address.

The swap completely stopped working, and I had to reinstall the extension to get out of this.

https://user-images.githubusercontent.com/7690112/199694985-8859d6fc-3743-451c-931c-ac0f7b772721.mp4

Other signals for this problem are

• many events in our background script are triggered multiple times for the same events. This was one of the root causes of our performance issues because the background<>UI port communication was flooded. We have mitigated that issue by keeping the store size low and debouncing the redux persistence.
• the NFT list triggering refreshes

Theory for the root cause: Background updates redux store in the background > redux store change is propagated to the UI through port communication/webext-redux > when the changes are integrated into the UI proxy store a change event is generated > that doesn't affect the UI because react or selectors filter these changes BUT the async thunk is fired somehow > which is propagated to the background > goto step 1

I have a hunch that the root cause of this mystery bug is behind most of our user issues.

Affected functionality: all around the extension, performance, send, swaps.

[^1]: The recording was done on a dev build, but the issue is present in the 0.17.6 release build [^2]: Wenn video embed github?

[Shadowfiend]

Wenn video embed github?

About 17 months ago 😜

[Shadowfiend]

A quick remark on the hypothesis: I would be surprised if anything that happens in swap state management is easily ascribed to background script issues, because the swap page's frontend state management is so complicated and almost certainly has bugs.

[jagodarybacka]

fixed the video for u @greg-nagy 😄

[Naxsun]

@greg-nagy It's a while ago. What I was able to dig up quickly are issues that may be related, but not 100% sure. Does that help?

https://github.com/tallycash/extension/issues/1832 https://github.com/tallycash/extension/issues/2320

[greg-nagy]

@Naxsun thx, this helps!

also found the issue: https://github.com/tallycash/extension/issues/2403

I think this one is also related: https://github.com/tallycash/extension/issues/1898

[mhluongo]

I think this is a few independent issues...? It'd be good to break it out rather than a catch-all "high severity" bug that doesn't have clear reproduction steps.

I think you're trying to jump from a few issues to a solution, but we don't have evidence that they share a root cause. Instead, I think...

• Assets are one issue — we've known about it for a long time, and they can be tackled well and independently
• The swap UI logic is a mess — I've seen what looks like a race at least once, and the asset approval flow is also broken, to boot.
• General performance issues, some of which have shared causes, and some of which don't.

I have a hunch that the root cause of this mystery bug is behind most of our user issues.

I think we should avoid this sort of thinking in our issues. Let's get crisp here! In the meantime, I'm removing the high-severity label to keep from messing up sprint planning / miscommunicating our confidence here.

[greg-nagy]

thanks for pushing me to define better issues! You are correct. I went with my gut instinct here, and this issue is more of a pitch for a spike than an actionable task. I will collect more info and try to get to the root cause before ticketing the work. Let's treat this as a placeholder until then.

The issue is meant to be dedicated to UI rerenders caused by seemingly unrelated events. This behavior signals — at least to me — that somewhere, somehow, they are coupled together. Put it differently: What triggers the code path that leads to the flicker?

Not in scope for this issue: Concrete implementations fixes — swaps, asset merge

Seemingly related symptoms

• NFTsWallet.tsx Wallet.tsx page rerenders multiple times which causes rerenders of every component underneath in the state tree. (Unless they are using React.memoize. This comment is also a clue) Debug notes:
  • setup
  • hardcode all the NFT data in the component and remove all the selectors, dispatch etc
  • put a breakpoint to the return
  • add console.log("--- debug webext-redux PATCH_STATE_TYPE payload", JSON.stringify(message.payload, null, 2)); above this line in webext-redux + yarn build + yarn link
  • the rerender happens when we see this on the console from the webext-redux

    Store.js:123 --- debug webext-redux PATCH_STATE_TYPE payload [
    {
    "key": "assets",
    "change": "updated_array",
    "value": []
    }
    ]

Problems/working questions:

• why do we get updates with [] -> these should be filtered out as unnecessary noise?
• why does Wallet.tsx rerenders on all updates to assets and activities?
• why does Wallet.tsx rerenders without any meaningful state change?

[Shadowfiend]

The spurious updates are probably due to some of the gnarlier selectors like account totals that require a lot of info. Very likely some things that can be rendered conditionally to reduce triggers, and some selectors that might be able to be refined for better dependencies.

[Shadowfiend]

Also of note: deeper understanding of the patch stuff is 💯 worth it as well. Any patching that changes object identity is going to trigger updates. The empty array hunch feels sound---relaying empty arrays is not incorrect, but detecting when pre and post change arrays are both zero-length and suppressing those updates could definitely reduce invalidation of certain object trees in useful ways.

[greg-nagy]

To jot down before I forget Every main tab component is rerendering 3-5 times on every route change. This multiplies the rerenders coming from selectors.

The selector issue is likely to come from the fact that

• selectors memoization is using === by default
• webext-redux's change propagation logic is probably specific to shallowDiff
  • this check should be diff[0].values.length for deepDiff based on the shallowDiffStrat

  • I might lack the understanding of what a key update with an empty array as a value mean. Needs more digging

  • In the background we emit assets event even if we load all assets from cache and this results in the empty state changes

Still open questions

• Why do we get rerenders on seemingly unrelated changes.

  • Working theory is that we couple them together through selector chaining