Skip to content

Commit

Permalink
fix: Fix background popup client connection count (#23583)
Browse files Browse the repository at this point in the history
## **Description**

Currently it is possible to open more than one extension popup (the
window that opens when you click on the extension icon from the browser
toolbar), but background currently only considers it possible to have
one open. This results in a bug where if two or more popups are open and
one is closed, any ongoing gas fee polling will stop for all popups not
just the one recently closed. This PR fixes this by changing the
variable that tracks popup connection counts from a boolean to an int.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23583?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Open two browser windows
2. Open one extension popup
3. In that popup, get to the confirmation screen for sending a
transaction
4. The gas fee should be updating periodically
5. In the other window, open an extension popup
6. The gas fee should be updating periodically
7. Close any popup
- NOTE: The gas fee will change to a very low number before updating
back to the correct amount. This seems to be a separate bug
8. The gas fee should be updating periodically still in the popup still
open

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**


https://github.com/MetaMask/metamask-extension/assets/918701/466440b6-7c8b-4990-985a-5cdd0c0fe9ec


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **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.
  • Loading branch information
jiexi authored Mar 25, 2024
1 parent b4356b9 commit b3d93fa
Showing 1 changed file with 6 additions and 6 deletions.
12 changes: 6 additions & 6 deletions app/scripts/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ log.setLevel(process.env.METAMASK_DEBUG ? 'debug' : 'info', false);
const platform = new ExtensionPlatform();
const notificationManager = new NotificationManager();

let popupIsOpen = false;
let openPopupCount = 0;
let notificationIsOpen = false;
let uiIsTriggering = false;
const openMetamaskTabsIDs = {};
Expand Down Expand Up @@ -571,7 +571,7 @@ export function setupController(

const isClientOpenStatus = () => {
return (
popupIsOpen ||
openPopupCount > 0 ||
Boolean(Object.keys(openMetamaskTabsIDs).length) ||
notificationIsOpen
);
Expand Down Expand Up @@ -657,9 +657,9 @@ export function setupController(
controller.setupTrustedCommunication(portStream, remotePort.sender);

if (processName === ENVIRONMENT_TYPE_POPUP) {
popupIsOpen = true;
openPopupCount += 1;
endOfStream(portStream, () => {
popupIsOpen = false;
openPopupCount -= 1;
const isClientOpen = isClientOpenStatus();
controller.isClientOpen = isClientOpen;
onCloseEnvironmentInstances(isClientOpen, ENVIRONMENT_TYPE_POPUP);
Expand Down Expand Up @@ -922,15 +922,15 @@ async function triggerUi() {
const currentlyActiveMetamaskTab = Boolean(
tabs.find((tab) => openMetamaskTabsIDs[tab.id]),
);
// Vivaldi is not closing port connection on popup close, so popupIsOpen does not work correctly
// Vivaldi is not closing port connection on popup close, so openPopupCount does not work correctly
// To be reviewed in the future if this behaviour is fixed - also the way we determine isVivaldi variable might change at some point
const isVivaldi =
tabs.length > 0 &&
tabs[0].extData &&
tabs[0].extData.indexOf('vivaldi_tab') > -1;
if (
!uiIsTriggering &&
(isVivaldi || !popupIsOpen) &&
(isVivaldi || openPopupCount === 0) &&
!currentlyActiveMetamaskTab
) {
uiIsTriggering = true;
Expand Down

0 comments on commit b3d93fa

Please sign in to comment.