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.

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

fix: Reset streams on BFCache events #24950

Closed
wants to merge 20 commits into from

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented May 31, 2024

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

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: https://github.com/MetaMask/MetaMask-planning/issues/2582

Manual testing steps

Steps are for macOS. Using chrome 123 or newer

Testing with old BFCache strategy

  1. Close the entire chrome app
  2. run open /Applications/Google\ Chrome.app --args --disable-features=DisconnectExtensionMessagePortWhenPageEntersBFCache
  3. Visit http://www.brainjar.com/java/host/test.html
  4. Open console
  5. Enter await window.ethereum.request({method: 'eth_chainId'}), which should be responsive
  6. Visit chrome://terms/
  7. Use the back button to go back to the brainjar test page
  8. 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

bfcache.new.behavior.mov

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

targeted.prerender.still.working.correctly.mov

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@jiexi jiexi changed the title Reset streams on BFCache events fix: Reset streams on BFCache events May 31, 2024
@jiexi jiexi marked this pull request as ready for review May 31, 2024 18:50
@jiexi jiexi requested a review from a team as a code owner May 31, 2024 18:50
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.65%. Comparing base (d8e0cd1) to head (7ae24c3).
Report is 15 commits behind head on develop.

Current head 7ae24c3 differs from pull request most recent head 9856738

Please upload reports for the commit 9856738 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24950      +/-   ##
===========================================
+ Coverage    65.61%   65.65%   +0.04%     
===========================================
  Files         1372     1376       +4     
  Lines        54519    54569      +50     
  Branches     14282    14293      +11     
===========================================
+ Hits         35770    35822      +52     
+ Misses       18749    18747       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [dec9f77]
Page Load Metrics (137 ± 170 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6911587126
domContentLoaded9201232
load441680137354170
domInteractive9201232

@metamaskbot
Copy link
Collaborator

Builds ready [7ae24c3]
Page Load Metrics (51 ± 5 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint681058594
domContentLoaded9341252
load428051105
domInteractive9341252
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Aug 20, 2024
Copy link
Contributor

github-actions bot commented Sep 3, 2024

This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions.

@github-actions github-actions bot closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants