Skip to content

Commit

Permalink
fix: Reset streams on BFCache events (#24950)
Browse files Browse the repository at this point in the history
## **Description**

Previously, Chrome's BFCache (Back Forward) strategy was to evict a page
from cache if it received a message over any connected port streams. The
port stream would remain open and the background script would NOT
receive an onDisconnect. As long as the cached page did not receive a
message, the port would still function when the page became active
again. This was problematic because MetaMask was likely to send a
message to the still connected cached page at some point due to the
nature of notifications, which would evict the page from cache,
neutralizing the performance benefit of the BFCache for the end user.

Now, Chrome's BFCache strategy is to trigger an onDisconnect for the
background script, but NOT the cached page. The port stream is invalid
despite not being closed on the cached page side. This is problematic
because we do not listen for events relevant to when a BFCached page
becomes active and thus do not reset the invalid stream.

To address both strategies, we now listen for the `pageshow` and
`pagehide` events. When a page is entering a BFCached state, we
preemptively end the port stream connection (even if the user is on an
older version of chrome that would have kept it alive). When a BFCached
page is restored to an active state, we establish a port stream
connection. We know the port stream must be restored/reset because we
were the ones responsible for preemptively ending it earlier in the
lifecycle. Combining these two changes allows us to handle both the old
and new BFCache strategies without having to target them by versions
separately.

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

## **Related issues**

See:
https://developer.chrome.com/blog/bfcache-extension-messaging-changes?hl=en
See: #13373
See: https://web.dev/articles/bfcache (useful links at bottom)
See: w3c/webextensions#474
Fixes: MetaMask/MetaMask-planning#2582

## **Manual testing steps**
Steps are for macOS. Using chrome 123 or newer

**Testing with old BFCache strategy**
1. Close the entire chrome app
1. run `open /Applications/Google\ Chrome.app --args
--disable-features=DisconnectExtensionMessagePortWhenPageEntersBFCache`
1. Visit `http://www.brainjar.com/java/host/test.html`
1. Open console
1. Enter `await window.ethereum.request({method: 'eth_chainId'})`, which
should be responsive
1. Visit `chrome://terms/`
1. Use the back button to go back to the brainjar test page
1. Enter `await window.ethereum.request({method: 'eth_chainId'})`, which
should be responsive

**Testing with the new BFCache strategy**
Repeat the steps above, but use `--enable-features` instead of `disable`
 
MetaMask Behavior should look the same regardless of browser's BFCache
strategy

## **Screenshots/Recordings**

BFCache Behavior


https://github.com/MetaMask/metamask-extension/assets/918701/efeb1591-5fde-44c8-b0a3-3573dfb97806


Prerender Behavior (to show affected chromium browsers still reset
streams correctly)


https://github.com/MetaMask/metamask-extension/assets/918701/7461bf64-b5b0-4e70-96d5-416cf5bf6b7c



## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [ ] 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/develop/.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.
  • Loading branch information
jiexi authored Nov 22, 2024
1 parent b38244b commit ad9a748
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 8 deletions.
16 changes: 16 additions & 0 deletions app/scripts/contentscript.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { getIsBrowserPrerenderBroken } from '../../shared/modules/browser-runtime.utils';
import shouldInjectProvider from '../../shared/modules/provider-injection';
import {
destroyStreams,
initStreams,
onDisconnectDestroyStreams,
setupExtensionStreams,
} from './streams/provider-stream';
import {
isDetectedPhishingSite,
Expand Down Expand Up @@ -33,6 +35,20 @@ const start = () => {
);
});
}

window.addEventListener('pageshow', (event) => {
if (event.persisted) {
console.warn('BFCached page has become active. Restoring the streams.');
setupExtensionStreams();
}
});

window.addEventListener('pagehide', (event) => {
if (event.persisted) {
console.warn('Page may become BFCached. Destroying the streams.');
destroyStreams();
}
});
}
};

Expand Down
32 changes: 24 additions & 8 deletions app/scripts/streams/provider-stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ let legacyExtMux: ObjectMultiplex,

let extensionMux: ObjectMultiplex,
extensionChannel: Substream,
extensionPort: browser.Runtime.Port,
extensionPort: browser.Runtime.Port | null,
extensionStream: PortStream | null,
pageMux: ObjectMultiplex,
pageChannel: Substream;
Expand Down Expand Up @@ -65,7 +65,7 @@ const setupPageStreams = () => {
// The field below is used to ensure that replay is done only once for each restart.
let METAMASK_EXTENSION_CONNECT_SENT = false;

const setupExtensionStreams = () => {
export const setupExtensionStreams = () => {
METAMASK_EXTENSION_CONNECT_SENT = true;
extensionPort = browser.runtime.connect({ name: CONTENT_SCRIPT });
extensionStream = new PortStream(extensionPort);
Expand Down Expand Up @@ -226,19 +226,35 @@ const onMessageSetUpExtensionStreams = (msg: MessageType) => {
return undefined;
};

/**
* Ends two-way communication streams between browser extension and
* the local per-page browser context.
*/
export function destroyStreams() {
if (!extensionPort) {
return;
}
extensionPort.onDisconnect.removeListener(onDisconnectDestroyStreams);

destroyExtensionStreams();
destroyLegacyExtensionStreams();

extensionPort.disconnect();
extensionPort = null;

METAMASK_EXTENSION_CONNECT_SENT = false;
}

/**
* This listener destroys the extension streams when the extension port is disconnected,
* so that streams may be re-established later when the extension port is reconnected.
*
* @param [err] - Stream connection error
*/
export const onDisconnectDestroyStreams = (err: unknown) => {
export function onDisconnectDestroyStreams(err: unknown) {
const lastErr = err || checkForLastError();

extensionPort.onDisconnect.removeListener(onDisconnectDestroyStreams);

destroyExtensionStreams();
destroyLegacyExtensionStreams();
destroyStreams();

/**
* If an error is found, reset the streams. When running two or more dapps, resetting the service
Expand All @@ -251,7 +267,7 @@ export const onDisconnectDestroyStreams = (err: unknown) => {
console.warn(`${lastErr} Resetting the streams.`);
setTimeout(setupExtensionStreams, 1000);
}
};
}

/**
* Initializes two-way communication streams between the browser extension and
Expand Down
65 changes: 65 additions & 0 deletions test/e2e/provider/bfcache.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
const { strict: assert } = require('assert');
const {
withFixtures,
defaultGanacheOptions,
DAPP_URL,
openDapp,
} = require('../helpers');
const FixtureBuilder = require('../fixture-builder');

const triggerBFCache = async (driver) => {
await driver.executeScript(`
window.addEventListener('pageshow', (event) => {
if (event.persisted) {
window.restoredFromBFCache = true
}
});
`);

await driver.driver.get(`chrome://terms/`);

await driver.driver.navigate().back();

const restoredFromBFCache = await driver.executeScript(
`return window.restoredFromBFCache`,
);

if (!restoredFromBFCache) {
assert.fail(new Error('Failed to trigger BFCache'));
}
};

describe('BFCache', function () {
it('has a working provider stream when a dapp is restored from BFCache', async function () {
await withFixtures(
{
dapp: true,
fixtures: new FixtureBuilder().build(),
ganacheOptions: defaultGanacheOptions,
title: this.test.fullTitle(),
},
async ({ driver }) => {
await openDapp(driver, undefined, DAPP_URL);

const request = JSON.stringify({
jsonrpc: '2.0',
method: 'eth_chainId',
params: [],
id: 0,
});

const initialResult = await driver.executeScript(
`return window.ethereum.request(${request})`,
);
assert.equal(initialResult, '0x539');

await triggerBFCache(driver);

const bfcacheResult = await driver.executeScript(
`return window.ethereum.request(${request})`,
);
assert.equal(bfcacheResult, '0x539');
},
);
});
});

0 comments on commit ad9a748

Please sign in to comment.