Description

This PR addresses a problem which had the following surface level symptoms: when the UI is closed, the extension continues to send network requests to the provider.

Upon some investigation, we discovered that these requests were the result of polling initiated from the account tracker. And then upon further investigation, we discovered that the callback in the outstream 'end' listener in setupControllerConnection of metamask-controller.js was not being called when the UI was closed.

This problem was introduced with #24533. That PR updates the object-multiplex dependency to v2.0.0. The major change in that update is the update of the readable-stream dependency from a v2 version to a v3 version. With that update, we see new premature-close errors in the background. It seems that these errors are still present in v2, but they are handled differently, resulting in an 'error' event instead of an 'end' event. We still have not established a definitive understanding of why this is happening, and follow up investigation will be needed.

However, this PR still puts forward a fix that safely resolves the issue. The solution added here is to make use of the readableStream.finished function. The callback passed to this function will be called when the stream is ended. This function has the same behaviour as the outstream.on('end' listener had prior to the aforementioned PR.

Instead of fully replacing the existing listener, we have added the readableStream.finished call in addition to the existing listener. The reason for this is explained in a code comment:

The presence of both of the below handlers may be redundant. After upgrading metamask/object-multiples to v2.0.0, which included an upgrade of readable-streams from v2 to v3, we saw that the outStream.on('end' handler was almost never being called. This seems to related to how v3 handles errors vs how v2 handles errors; there are "premature close" errors in both cases, although in the case of v2 they don't prevent outStream.on('end' from being called. At the time that this comment was committed, it was known that we need to investigate and resolve the underlying error, however, for expediency, we are not addressing them at this time. Instead, we can observe that readableStream.finished preserves the same functionality as we had when we relied on readable-stream v2. Meanwhile, the outStream.on('end' handler was observed to have been called at least once. As we do not yet fully understand the conditions under which both of these are being called, we include both handlers at the risk of redundancy. Logic has been added to the handler to ensure that calling it more than once does not have any affect.

To achieve "Logic has been added to the handler to ensure that calling it more than once does not have any affect.", the PR adds a unique id to each stream passed to setupControllerConnection, and uses a Set to ensure the handler code only runs once per stream.

This PR then adds automated tests. The added unit tests are focused on validating both pre-existing and new behaviour of setupControllerChanged. The added e2e tests focus on the inititially mentioned surface level symptoms: it confirms that eth_call and eth_getBalance calls, the calls coming from the account-tracker initiated polling, are not continued after the UI is closed. This e2e test fails on current develop but passes on this branch.

MetaMask team members can see more details in this thread: https://consensys.slack.com/archives/CTQAGKY5V/p1721653870583869

Related issues

Fixes: #26002

Manual testing steps

To observe the bug on develop:

1. Install and onboard
2. Open the service worker dev console and go to the network tab
3. Observe eth_call and eth_getBalance requests to infura
4. Close the UI, observe that the requests from step 3 continue

On this branch, in step 4, after closing the UI, those requests will not continue.

Screenshots/Recordings

Before

After

Pre-merge author checklist

• I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
• I've completed the PR template to the best of my ability
• I’ve included tests if applicable
• I’ve documented my code using JSDoc format if applicable
• I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

• I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
• I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.