Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Behaviour of extension message ports and bfcache #474

Open
oliverdunk opened this issue Oct 23, 2023 · 7 comments
Open

Behaviour of extension message ports and bfcache #474

oliverdunk opened this issue Oct 23, 2023 · 7 comments
Labels
follow-up: firefox Needs a response from a Firefox representative implemented: chrome Implemented in Chrome inconsistency Inconsistent behavior across browsers supportive: safari Supportive from Safari

Comments

@oliverdunk
Copy link
Member

Back/forward cache (bfcache) is an optimization in browsers where pages are suspended but cached upon navigation so that they can be more quickly restored if a user decides to return.

If a page that is entering the cache has an open message port to another part of the extension, it is unclear how this should be handled. For example, it could be kept alive with messages buffered, kept alive with messages dropped, or immediately closed. Alternatively, it could be dropped but only if a message is received.

We discussed this at TPAC and our current thinking is to immediately drop ports. This may require more work for some developers than alternatives, but is hopefully consistent behaviour that reduces the risk of extensions behaving in unexpected ways.

Opening this issue to track any further discussion and rollout.
@oliverdunk oliverdunk added inconsistency Inconsistent behavior across browsers supportive: chrome Supportive from Chrome supportive: safari Supportive from Safari follow-up: firefox Needs a response from a Firefox representative labels Oct 23, 2023
@oliverdunk
Copy link
Member Author

@zombie / @Rob--W: The conclusion we came to at TPAC was based on the understanding that the current Firefox behaviour is to immediately close ports. After some testing, however, it seems like the port actually remains open indefinitely and messages are dropped while the page is suspended. Does this match what you are seeing, and does it change your thoughts here?

@Rob--W Rob--W mentioned this issue Nov 9, 2023
Merged
@dotproto dotproto mentioned this issue Nov 13, 2023
Closed
@Rob--W Rob--W mentioned this issue Nov 23, 2023
Merged
@oliverdunk
Copy link
Member Author

We are moving forward with this change in Chrome: https://developer.chrome.com/blog/bfcache-extension-messaging-changes. Given the potential for breakage and the fact that this is not currently the behavior in Firefox, we are doing so cautiously and with warnings in DevTools to try and give time for extensions to adapt.

@xeenon
Copy link
Collaborator

xeenon commented Mar 3, 2024

WebKit issue tracking this: https://webkit.org/b/270413

@Jack-Works
Copy link

Currently, we're listening to port.onDisconnected to reconnect (not pageshow event). Will our code continue to work?

@oliverdunk
Copy link
Member Author

Currently, we're listening to port.onDisconnected to reconnect (not pageshow event). Will our code continue to work?

Good question! In Chrome, this depends on where you are listening to the event:

  • In the background context, the event does fire, including with a new error message starting in Chrome 124 (commit).
  • In the content script, we agreed with other browsers not to fire an event. There are some open questions around when that would fire and pageshow seemed to be a reasonable solution. If we see this causing issues longer term, it's something that we could discuss if we want to change.

@Jack-Works
Copy link

In the content script, we agreed with other browsers not to fire an event. There are some open questions around when that would fire and pageshow seemed to be a reasonable solution. If we see this causing issues longer term, it's something that we could discuss if we want to change.

So this means, my port will not receive a disconnect event and the next message cannot be sent? I don't think it's intuitive to developers.

@jiexi jiexi mentioned this issue May 31, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this issue Nov 22, 2024
@jiexi
fix: Reset streams on BFCache events (#24950)
cc51a11 
## **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.
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this issue Nov 22, 2024
@jiexi
fix: Reset streams on BFCache events (#24950)
ad9a748 
## **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.
@oliverdunk
Copy link
Member Author

This change is shipping in Chrome so adding the implemented label.

@oliverdunk oliverdunk added implemented: chrome Implemented in Chrome and removed supportive: chrome Supportive from Chrome labels Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
follow-up: firefox Needs a response from a Firefox representative implemented: chrome Implemented in Chrome inconsistency Inconsistent behavior across browsers supportive: safari Supportive from Safari
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@xeenon @Jack-Works @oliverdunk