Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chainChanged emits on page load #110

Closed
mqklin opened this issue Sep 16, 2020 · 14 comments · Fixed by #117
Closed

chainChanged emits on page load #110

mqklin opened this issue Sep 16, 2020 · 14 comments · Fixed by #117
Assignees
Labels
bug Something isn't working

Comments

@mqklin
Copy link

mqklin commented Sep 16, 2020

Describe the bug
Sometimes chainChanged message emits on every page load. So if I have this line in my code: ethereum.on('chainChanged', (_chainId) => window.location.reload()); then user gets endless page refresh loop.

To Reproduce
I couldn't reproduce this, but one of our user shared his screen and I saw this endless page refresh loop. I added console.log('chainChanged'); to that event callback, and console showed this log every time before page refreshed.

Expected behavior
chainChanged event should be sent only on chain change, not on page load

Browser details

  • OS: OS X
  • Browser: last Chrome
  • MetaMask Version: 8.0.8
@danfinlay
Copy link
Contributor

Thanks for the report!

@mehranhydary
Copy link

Hi! I ran into this issue as well... has anyone solved this?

@mqklin
Copy link
Author

mqklin commented Oct 17, 2020

@mehranhydary to fix this I store lastChainId in localStorage and on chainChained check if it's the same:

      (async () => {
        try {
          const chainId = await web3.eth.getChainId();
          StoredLastEthereumChain.set(`0x${chainId.toString(16)}`); // save to localStorage

          web3.currentProvider.on(
            'chainChanged',
            chainId => {
              const lastChainId = StoredLastEthereumChain.get(); // get from localStorage
              if (String(chainId) !== String(lastChainId)) {
                window.location.reload();
              }
            },
          );
        }
        catch (e) {
          // handle exception
        }
      })();

@wbt
Copy link

wbt commented Nov 17, 2020

I am also seeing this issue, on MetaMask 8.1.3 on Windows 10 on Chrome 86.0.4240.183 (Official Build) (64-bit). The reload loop is long but not infinite; it's usually on the order of "several" and rarely into "dozens" of refreshes or few/none. It makes for pretty terrible UX.

@wbt
Copy link

wbt commented Nov 17, 2020

I should probably clarify that my computer is running very slowly today, and I suspect this is actually what interrupts the loop: if the computer is running slowly enough that the event handler (code from start of OP) does not get attached before the chainChanged event fires, the page stays loaded. That also explains the pseudorandom number of reloads.

@blakewest
Copy link

I ran into the same issue. I had to use the localStorage approach, which works, but is gross.

@wbt
Copy link

wbt commented Nov 23, 2020

The localStorage solution above runs somewhat slowly because of the call to getChainId, slower than the pretty fast timer on producing this warning:

MetaMask: MetaMask will soon stop reloading pages on network change.
For more information, see: https://docs.metamask.io/guide/ethereum-provider.html#ethereum-autorefreshonnetworkchange
Set 'ethereum.autoRefreshOnNetworkChange' to 'false' to silence this warning.

One way around this is to only store the chain ID in the chainChanged event handler callback, which means one extra refresh when first loading the dapp and fewer warnings thereafter. However, if you have multiple tabs open, if you actually do change networks the localStorage will be updated in only one of them and already updated in all the rest, missing refreshes that should happen, so sessionStorage should be used instead.

Hopefully it can be fixed soon in MetaMask!

@wbt
Copy link

wbt commented Nov 23, 2020

FYI, it looks like the chainChanged event is emitted here, which looks like it's in the InPageProvider constructor, in a conditional if ('chainId' in state && state.chainId !== this.chainId). Above that, this.chainId is set to null.
Within the conditional but before the event emission, there is this.chainId = state.chainId || null.

The way I read it, if state.chainId is not null, the first half of the conditional will necessarily be true and the second half will also be true, because at that point in execution this.chainId === null and (!null !== null). Therefore, the conditional will evaluate true and be entered. Then, the this.chainId will be set to the same as state.chainId and that value will be emitted with the event, obscuring the fact that this.chainId was null until just prior to the event emission.

I suspect the issue may have been introduced in this commit, but am not familiar enough with the context/flow/intended behavior of the InPageProvider constructor to give a sure fix. Hopefully the above can guide someone who is more familiar with that block of code than I... @rekmarks perhaps? He would at least have a better idea than I of what he was aiming to accomplish with that commit.

@rekmarks rekmarks self-assigned this Nov 23, 2020
@rekmarks
Copy link
Member

Thank you for pinging me @wbt. I'm going to fix this ASAP, and it should be included in the same release as all of these changes: MetaMask/metamask-extension#8077

I'll leave this open until we've merged the fix.

@rekmarks rekmarks transferred this issue from MetaMask/metamask-extension Nov 24, 2020
@rekmarks rekmarks added the bug Something isn't working label Nov 24, 2020
rekmarks added a commit that referenced this issue Dec 7, 2020
It was reported in #110 that `chainChanged` (and by extension, `networkChanged`) is sometimes emitted for the first time after page load, causing a reload loop for those handling chain changes by reloading the page. When the `chainId` is set on initialization, the chain has not changed, and `chainChanged` should not be emitted. The same can be said for `accountsChanged`, which currently would emit under the same circumstances.

This PR ensures that `chainChanged`, `networkChanged`, and `accountsChanged` are only emitted after the provider has been initialized.
@zimmah
Copy link

zimmah commented Dec 26, 2020

Since which version is this fixed? Because I'm still having this issue. (not sure if I have the latest version, BTW I use the build-in wallet by brave that's based on metamask)

@rekmarks
Copy link
Member

rekmarks commented Dec 26, 2020

We haven't shipped this yet due to the holidays, but it will be included in version 9 of the MetaMask extension.

Edit: Expect it early in the New Year.

@mqklin
Copy link
Author

mqklin commented Dec 28, 2020

The localStorage solution above runs somewhat slowly because of the call to getChainId, slower than the pretty fast timer on producing this warning:

@wbt I moved window.ethereum.autoRefreshOnNetworkChange = false; line before async function and don't see it anymore.

@laygir
Copy link

laygir commented Jan 25, 2022

I'm having this issue with Brave but not with Chrome.

Brave v1.34.81 Chromium: 97.0.4692.99
MetaMask v10.8.1

@Sheriik
Copy link

Sheriik commented Feb 13, 2023

Having this issue on android with TrustWallet browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants