Skip to content

fix: prevent approval popup from being force-closed by dapp polling (ADN-757)#819

Open
HeesungB wants to merge 2 commits intomainfrom
fix/ADN-757
Open

fix: prevent approval popup from being force-closed by dapp polling (ADN-757)#819
HeesungB wants to merge 2 commits intomainfrom
fix/ADN-757

Conversation

@HeesungB
Copy link
Copy Markdown

Summary

Three related fixes for approval-popup teardown that caused dapps to see TRANSACTION_REJECTED when the user had actually approved (or had not acted at all).

  1. Primarymessage-handler.ts no longer tears down open popups for read-only requests (GET_ACCOUNT, GET_NETWORK). The dapp's background polling was force-closing in-flight approval popups.
  2. Race guardcommon.ts now ensures sendResponse fires at most once and cleans up all popup listeners together, plus the popup awaits chrome.runtime.sendMessage before window.close(). Prevents the earlier race where a successful popup response could be overridden by the onRemoved fallback.
  3. Forced-close disambiguation — When a popup is closed by the extension itself (not by the user), the dapp now receives UNEXPECTED_ERROR instead of the user-rejection message. This keeps the rejection signal truthful for future force-close paths (e.g. consecutive DO_CONTRACT requests).

Root cause

While a DO_CONTRACT popup was open, the dapp (e.g. Gnoswap) kept polling GET_ACCOUNT every few seconds. On each poll, MessageHandler.requestHandler ran removePopups() before dispatching the handler, which closed the approval popup. The popup's onRemoved listener in common.ts then sent the default closeMessage (TRANSACTION_REJECTED) back to the dapp — producing a "user rejected" result even though the user had done nothing, or had actually approved.

Timing evidence from instrumentation:

  • Failing key 51fd0e5b…: dapp GET_ACCOUNT at t=50997ms → popup onRemoved at t=51017ms (~20ms gap)
  • Failing key 2c108cd1…: dapp GET_ACCOUNT at t=124156ms → popup onRemoved at t=124173ms (~17ms gap)

Changes

packages/adena-extension/src/inject/message/message-handler.ts

  • Introduce NON_POPUP_REQUEST_TYPES = { GET_ACCOUNT, GET_NETWORK }.
  • Skip existsPopups() / removePopups() for these read-only requests. Popup-opening requests (DO_CONTRACT, SIGN_*, ADD_ESTABLISH, etc.) preserve the existing behavior.

packages/adena-extension/src/inject/message/methods/common.ts

  • Replace anonymous listeners with named onRemovedListener / onMessageListener / onTabsUpdatedListener and add a cleanup() that removes all three. Prevents listener leaks across popup sessions.
  • Add a sendResponseOnce guard so sendResponse can never fire twice for the same request (defends against races between popup success message and onRemoved fallback).
  • Add a module-scoped programmaticallyClosedPopups: Set<number>. removePopups() records each window id it is about to close. When onRemovedListener fires, if the id is in the set, respond with UNEXPECTED_ERROR instead of the default closeMessage. This prevents future forced-close paths (e.g. consecutive DO_CONTRACT requests) from being reported as user rejections.

packages/adena-extension/src/pages/popup/wallet/approve-transaction-main/index.tsx

  • onClickCloseResult: await chrome.runtime.sendMessage(response) before window.close() so the success response actually reaches the background before the popup tears down.

Test plan

  • Build the extension and load unpacked.
  • On Gnoswap, open a swap approval popup and wait >5s before confirming (to let background GET_ACCOUNT polling fire). Confirm the popup stays open and the transaction succeeds. Dapp should receive TRANSACTION_SUCCESS.
  • Repeat with multiple swaps in succession — each should yield its own TRANSACTION_SUCCESS.
  • Close the popup with the X button mid-review — dapp should receive TRANSACTION_REJECTED (unchanged behavior).
  • Fire a second DO_CONTRACT while a first popup is still open — previous request should now receive UNEXPECTED_ERROR (not a user rejection).
  • SIGN_AMINO / SIGN_TX popups behave the same way (same createPopup path).
  • GET_ACCOUNT / GET_NETWORK continue to respond normally.

Closes ADN-757

…sful tx

A race between the popup's success response and chrome.windows.onRemoved
could cause the dapp to receive TRANSACTION_REJECTED even though the
transaction was broadcast successfully. Guard sendResponse to fire at
most once, clean up listeners after the first response, and await
chrome.runtime.sendMessage in the popup before closing the window.
DApps such as Gnoswap poll GET_ACCOUNT/GET_NETWORK every few seconds while
an approval popup is open. Each incoming request previously ran removePopups()
unconditionally, tearing down the in-flight popup and making the dapp receive
TRANSACTION_REJECTED even though the user had done nothing or had actually
approved.

- message-handler: skip existsPopups/removePopups for read-only request types
  (GET_ACCOUNT, GET_NETWORK).
- common: track window ids closed programmatically and, when onRemoved fires
  for such a popup, respond with UNEXPECTED_ERROR instead of the default
  closeMessage. Keeps the user-rejection signal truthful for future forced
  close paths.
Copy link
Copy Markdown
Member

@jinoosss jinoosss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants