Skip to content

Commit f638960

Browse files
gauthierpetetingauthierpetetin
authored
fix(premature close): gracefully end extension stream muxes to prevent "Premature close" errors (#37400)
## **Description** This PR is an AI-generated fix aiming to fix the #1 error in Sentry (3.8M occurrences/month) by adding graceful shutdown handlers to extension context streams, preventing "ERR_STREAM_PREMATURE_CLOSE" errors. The error was initially discussed in this [Slack thread](https://consensys.slack.com/archives/CTQAGKY5V/p1761904147325279?thread_ts=1761646886.397379&cid=CTQAGKY5V). #### Disclaimer Since we haven't found a reliable way to manually reproduce these "Premature close" errors, there's no "before/after proof" that this PR fixes it. ### Problem When underlying transport streams (e.g., `WindowPostMessageStream`, `ExtensionPortStream`) close or end abruptly—during page navigation, tab closure, or extension disconnection—the `ObjectMultiplex` instances in **extension context** do not gracefully shut down. This results in "Premature close" errors being thrown by the stream pipeline. ### Solution Added graceful shutdown handlers to **extension context** streams that: 1. **Listen for transport termination**: Attach `once('close')` and `once('end')` listeners to underlying transport streams 2. **Proactively close muxes**: When transport terminates, check if mux is still open (`!destroyed && !writableEnded`) and call `mux.end()` before pipeline error detection kicks in 3. **Prevent error propagation**: By closing gracefully, we avoid "ERR_STREAM_PREMATURE_CLOSE" errors in the pipeline ### Key Design Decision: Page Context vs Extension Context **Why handlers are ONLY in extension context streams:** | Context | Files | Needs Handlers? | Reason | |---------|-------|-----------------|--------| | **Page Context** | `inpage.js` | ❌ **NO** | Browser automatically destroys the entire script execution context on navigation. Adding handlers can actually CAUSE disconnection errors during rapid navigation. | | **Extension Context** | `provider-stream.ts`<br>`phishing-stream.ts`<br>`cookie-handler-stream.ts` | ✅ **YES** | Extension context persists across page navigations. Muxes remain active when transports disconnect, causing "Premature close" errors without explicit cleanup. | This approach mirrors the solution in [#33470](#33470) for CAIP streams. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/37400?quickstart=1) ## **Changelog** CHANGELOG entry: Fixed "Premature close" stream errors in extension context by adding graceful shutdown handlers ## **Related issues** Fixes: #26337 Fixes: #35241 Fixes: #21078 ## **Manual testing steps** (disclaimer: the error is hard to reproduce, even with these manual testing steps) 1. Open a dapp page connected to MetaMask (e.g., https://metamask.github.io/test-dapp/) 2. Rapidly navigate between pages multiple times or close/reopen tabs 3. Open DevTools Console and check for errors 4. Open Extension Background Console: `chrome://extensions` → MetaMask → "service worker" 5. Verify no "Premature close" errors appear during page navigation 6. Test deep link navigation: Navigate to `chrome-extension://{extension-id}/home.html#link?u=/buy` and verify it redirects cleanly without errors **Expected**: Clean shutdowns with no "Premature close" errors in console ## **Screenshots/Recordings** ### **Before** - "Premature close" errors appear in extension background console during page navigation - Sentry reports 3.8M occurrences per month ### **After** - Extension context streams shut down gracefully - No "Premature close" errors during normal operations - E2E tests pass (including deep-link tests that navigate rapidly) ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). 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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Add graceful shutdown handlers to extension-context multiplexers for provider, phishing, and cookie-handler streams, plus tests; clarify no handlers in page-context in inpage. > > - **Streams (extension context)**: > - **Graceful shutdown handlers**: End muxes on underlying transport `close`/`end` to avoid `ERR_STREAM_PREMATURE_CLOSE` in: > - `app/scripts/streams/provider-stream.ts` (`pageMux`, `extensionMux`) > - `app/scripts/streams/phishing-stream.ts` (`phishingPageMux`, `phishingExtMux`) > - `app/scripts/streams/cookie-handler-stream.ts` (`cookieHandlerPageMux`, `cookieHandlerMux`) > - **Logging/notifications**: Preserve existing pipeline callbacks; continue logging via `logStreamDisconnectWarning` and sending `METAMASK_STREAM_FAILURE` where applicable. > - **Inpage (page context)**: > - `app/scripts/inpage.js`: Add documentation explaining why graceful shutdown handlers are not added in page context. > - **Tests**: > - Add filter tests to assert unconditional logging on pipeline completion/error: > - `provider-stream.filter.test.ts` > - `phishing-stream.filter.test.ts` > - `cookie-handler-stream.filter.test.ts` > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit e72ec5e. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 709e4d7 commit f638960

File tree

7 files changed

+593
-0
lines changed

7 files changed

+593
-0
lines changed

‎app/scripts/inpage.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,38 @@ if (shouldInjectProvider()) {
6767
});
6868

6969
const mux = new ObjectMultiplex();
70+
71+
/**
72+
* Note: We do NOT add graceful shutdown handlers (close/end/beforeunload) to the mux
73+
* in this file, unlike in the background stream files (provider-stream.ts, etc.).
74+
*
75+
* This is intentional because:
76+
*
77+
* 1. CONTEXT DIFFERENCE:
78+
* - inpage.js runs in PAGE CONTEXT (web pages)
79+
* - Background streams run in EXTENSION CONTEXT (persistent background)
80+
*
81+
* 2. AUTOMATIC CLEANUP:
82+
* - When a page navigates/unloads, the browser automatically destroys the entire
83+
* script execution context, including all streams and event listeners
84+
* - No explicit cleanup is needed - the browser handles it naturally
85+
*
86+
* 3. AVOIDING PREMATURE DISCONNECTION:
87+
* - Adding handlers that call mux.end() or connectionStream.end() can actually
88+
* CAUSE disconnection errors when pages navigate to external URLs
89+
* - Tests showed that explicit handlers in page context trigger "Disconnected from
90+
* MetaMask background" errors during rapid navigation scenarios (e.g., deep links)
91+
*
92+
* 4. DIFFERENT ERROR SOURCE:
93+
* - "Premature close" errors in page context are typically harmless - they occur
94+
* during normal page navigation and don't indicate a real problem
95+
* - The critical "Premature close" issues (3.8M/month in Sentry) come from the
96+
* BACKGROUND streams that persist across page loads
97+
*
98+
* For context on the "Premature close" issue, see:
99+
* - https://github.com/MetaMask/metamask-extension/issues/26337
100+
* - https://github.com/MetaMask/metamask-extension/issues/35241
101+
*/
70102
pipeline(metamaskStream, mux, metamaskStream, (error) => {
71103
let warningMsg = `Lost connection to "${METAMASK_EIP_1193_PROVIDER}".`;
72104
if (error?.stack) {

‎app/scripts/streams/cookie-handler-stream.filter.test.ts‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
import { jest } from '@jest/globals';
2+
import {
3+
initializeCookieHandlerSteam,
4+
setupCookieHandlerExtStreams,
5+
} from './cookie-handler-stream';
6+
7+
// Set env before any imports
8+
process.env.PHISHING_WARNING_PAGE_URL = 'https://example.com/phishing';
9+
10+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
11+
(global as any).mockPipeline = jest.fn();
12+
jest.mock('readable-stream', () => ({
13+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
14+
pipeline: (...args: unknown[]) => (global as any).mockPipeline(...args),
15+
}));
16+
17+
jest.mock('@metamask/object-multiplex', () => {
18+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
19+
return function mockObjectMultiplex(this: any) {
20+
return {
21+
destroyed: false,
22+
writableEnded: false,
23+
setMaxListeners: () => {
24+
// empty on purpose
25+
},
26+
ignoreStream: () => {
27+
// empty on purpose
28+
},
29+
createStream() {
30+
return {} as unknown;
31+
},
32+
end() {
33+
this.writableEnded = true;
34+
},
35+
};
36+
};
37+
});
38+
39+
jest.mock('@metamask/post-message-stream', () => ({
40+
WindowPostMessageStream: class {
41+
once(_event: string, _fn: () => void) {
42+
// empty on purpose
43+
}
44+
},
45+
}));
46+
47+
jest.mock('extension-port-stream', () => ({
48+
ExtensionPortStream: class {
49+
once(_event: string, _fn: () => void) {
50+
// empty on purpose
51+
}
52+
},
53+
}));
54+
55+
jest.mock('webextension-polyfill', () => ({
56+
/* eslint-disable-next-line @typescript-eslint/naming-convention */
57+
__esModule: true,
58+
default: {
59+
runtime: {
60+
connect: () => ({
61+
onDisconnect: {
62+
addListener: () => {
63+
// empty on purpose
64+
},
65+
},
66+
}),
67+
onMessage: {
68+
addListener: () => {
69+
// empty on purpose
70+
},
71+
},
72+
sendMessage: () => Promise.resolve(),
73+
},
74+
},
75+
}));
76+
77+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
78+
(global as any).mockLogCalls = [] as unknown[];
79+
jest.mock('./stream-utils', () => ({
80+
logStreamDisconnectWarning: (...args: unknown[]) => {
81+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
82+
(global as any).mockLogCalls.push(args);
83+
},
84+
}));
85+
86+
describe('cookie-handler-stream logging filter', () => {
87+
beforeEach(() => {
88+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
89+
(global as any).mockPipeline.mockClear();
90+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
91+
(global as any).mockLogCalls = [];
92+
});
93+
94+
it('page mux pipeline: logs unconditionally on both completion and error', () => {
95+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
96+
const callsBefore = (global as any).mockPipeline.mock.calls.length;
97+
initializeCookieHandlerSteam();
98+
// The first pipeline call after init is the page mux pipeline (setupCookieHandlerStreamsFromOrigin)
99+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
100+
const pageMuxCall = (global as any).mockPipeline.mock.calls[callsBefore];
101+
const cb = pageMuxCall[pageMuxCall.length - 1] as (err?: Error) => void;
102+
103+
// Clean completion (no error) - still logs
104+
cb(undefined);
105+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
106+
expect((global as any).mockLogCalls.length).toBe(1);
107+
108+
// Error case - logs again
109+
cb(new Error('boom'));
110+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
111+
expect((global as any).mockLogCalls.length).toBe(2);
112+
});
113+
114+
it('extension mux pipeline: logs unconditionally on both completion and error', () => {
115+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
116+
const before = (global as any).mockPipeline.mock.calls.length;
117+
setupCookieHandlerExtStreams();
118+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
119+
const cb = (global as any).mockPipeline.mock.calls[before][
120+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
121+
(global as any).mockPipeline.mock.calls[before].length - 1
122+
] as (err?: Error) => void;
123+
124+
// Clean completion (no error) - still logs
125+
cb(undefined);
126+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
127+
expect((global as any).mockLogCalls.length).toBe(1);
128+
129+
// Error case - logs again
130+
cb(new Error('boom'));
131+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
132+
expect((global as any).mockLogCalls.length).toBe(2);
133+
});
134+
});

‎app/scripts/streams/cookie-handler-stream.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,32 @@ function setupCookieHandlerStreamsFromOrigin(origin: string): void {
4545
cookieHandlerPageMux = new ObjectMultiplex();
4646
cookieHandlerPageMux.setMaxListeners(25);
4747

48+
/**
49+
* Graceful shutdown handler for the cookie handler page mux.
50+
*
51+
* WHY THIS IS NEEDED:
52+
* This code runs in EXTENSION CONTEXT (content script), not page context.
53+
* When the page navigates or closes, the underlying transport terminates,
54+
* but the extension-side mux persists. Without this handler, the pipeline
55+
* throws "ERR_STREAM_PREMATURE_CLOSE" errors.
56+
*
57+
* See provider-stream.ts for detailed explanation, and:
58+
* - https://github.com/MetaMask/metamask-extension/issues/26337
59+
* - https://github.com/MetaMask/metamask-extension/issues/35241
60+
*/
61+
const endCookieHandlerPageMuxIfOpen = () => {
62+
if (
63+
!cookieHandlerPageMux.destroyed &&
64+
!cookieHandlerPageMux.writableEnded
65+
) {
66+
cookieHandlerPageMux.end();
67+
}
68+
};
69+
70+
// Attach handlers to detect when the underlying transport terminates
71+
cookieHandlerPageStream.once?.('close', endCookieHandlerPageMuxIfOpen);
72+
cookieHandlerPageStream.once?.('end', endCookieHandlerPageMuxIfOpen);
73+
4874
pipeline(
4975
cookieHandlerPageMux,
5076
cookieHandlerPageStream,
@@ -80,6 +106,21 @@ export const setupCookieHandlerExtStreams = (): void => {
80106
cookieHandlerMux = new ObjectMultiplex();
81107
cookieHandlerMux.setMaxListeners(25);
82108

109+
/**
110+
* Graceful shutdown handler for the cookie handler extension mux.
111+
* See the comment in provider-stream.ts for detailed explanation of why these
112+
* handlers are necessary in extension context but not in page context.
113+
*/
114+
const endCookieHandlerMuxIfOpen = () => {
115+
if (!cookieHandlerMux.destroyed && !cookieHandlerMux.writableEnded) {
116+
cookieHandlerMux.end();
117+
}
118+
};
119+
120+
// Attach handlers to detect when the underlying transport terminates
121+
cookieHandlerExtStream?.once?.('close', endCookieHandlerMuxIfOpen);
122+
cookieHandlerExtStream?.once?.('end', endCookieHandlerMuxIfOpen);
123+
83124
pipeline(
84125
cookieHandlerMux,
85126
cookieHandlerExtStream,

‎app/scripts/streams/phishing-stream.filter.test.ts‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
import { jest } from '@jest/globals';
2+
3+
// Set env before mocks
4+
process.env.PHISHING_WARNING_PAGE_URL = 'https://example.com/phishing';
5+
6+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
7+
(global as any).mockPipeline = jest.fn();
8+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
9+
(global as any).mockLogCalls = [] as unknown[];
10+
11+
jest.mock('readable-stream', () => ({
12+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
13+
pipeline: (...args: unknown[]) => (global as any).mockPipeline(...args),
14+
}));
15+
16+
jest.mock('@metamask/object-multiplex', () => {
17+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
18+
return function mockObjectMultiplex(this: any) {
19+
return {
20+
destroyed: false,
21+
writableEnded: false,
22+
setMaxListeners: () => {
23+
// empty on purpose
24+
},
25+
ignoreStream: () => {
26+
// empty on purpose
27+
},
28+
createStream() {
29+
return {} as unknown;
30+
},
31+
end() {
32+
this.writableEnded = true;
33+
},
34+
};
35+
};
36+
});
37+
38+
jest.mock('@metamask/post-message-stream', () => ({
39+
WindowPostMessageStream: class {
40+
once(_event: string, _fn: () => void) {
41+
// empty on purpose
42+
}
43+
},
44+
}));
45+
46+
jest.mock('extension-port-stream', () => ({
47+
ExtensionPortStream: class {
48+
once(_event: string, _fn: () => void) {
49+
// empty on purpose
50+
}
51+
},
52+
}));
53+
54+
jest.mock('webextension-polyfill', () => ({
55+
/* eslint-disable-next-line @typescript-eslint/naming-convention */
56+
__esModule: true,
57+
default: {
58+
runtime: {
59+
connect: () => ({
60+
onDisconnect: {
61+
addListener: () => {
62+
// empty on purpose
63+
},
64+
},
65+
}),
66+
onMessage: {
67+
addListener: () => {
68+
// empty on purpose
69+
},
70+
},
71+
},
72+
},
73+
}));
74+
75+
jest.mock('../../../shared/modules/browser-runtime.utils', () => ({
76+
checkForLastError: () => null,
77+
}));
78+
79+
jest.mock('./stream-utils', () => ({
80+
logStreamDisconnectWarning: (...args: unknown[]) => {
81+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
82+
(global as any).mockLogCalls.push(args);
83+
},
84+
}));
85+
86+
describe('phishing-stream logging filter', () => {
87+
beforeEach(() => {
88+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
89+
(global as any).mockPipeline.mockClear();
90+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
91+
(global as any).mockLogCalls = [];
92+
});
93+
94+
it('initPhishingStreams (page mux): logs unconditionally on both completion and error', async () => {
95+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
96+
const callsBefore = (global as any).mockPipeline.mock.calls.length;
97+
// @ts-expect-error - TypeScript requires .js extension for ESM but Jest cannot resolve it
98+
const { initPhishingStreams } = await import('./phishing-stream');
99+
initPhishingStreams();
100+
// The first pipeline call is the page mux pipeline (setupPhishingPageStreams)
101+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
102+
const pageMuxCall = (global as any).mockPipeline.mock.calls[callsBefore];
103+
const cb = pageMuxCall[pageMuxCall.length - 1] as (err?: Error) => void;
104+
105+
// Clean completion (no error) - still logs
106+
cb(undefined);
107+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
108+
expect((global as any).mockLogCalls.length).toBe(1);
109+
110+
// Error case - logs again
111+
cb(new Error('boom'));
112+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
113+
expect((global as any).mockLogCalls.length).toBe(2);
114+
});
115+
116+
it('extension mux pipeline: logs unconditionally on both completion and error', async () => {
117+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
118+
const before = (global as any).mockPipeline.mock.calls.length;
119+
// @ts-expect-error - TypeScript requires .js extension for ESM but Jest cannot resolve it
120+
const { setupPhishingExtStreams } = await import('./phishing-stream');
121+
setupPhishingExtStreams();
122+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
123+
const cb = (global as any).mockPipeline.mock.calls[before][
124+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
125+
(global as any).mockPipeline.mock.calls[before].length - 1
126+
] as (err?: Error) => void;
127+
128+
// Clean completion (no error) - still logs
129+
cb(undefined);
130+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
131+
expect((global as any).mockLogCalls.length).toBe(1);
132+
133+
// Error case - logs again
134+
cb(new Error('boom'));
135+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
136+
expect((global as any).mockLogCalls.length).toBe(2);
137+
});
138+
});

0 commit comments

Comments
 (0)