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

MV3: Update contentscript streams x service worker logic #15952

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Sep 23, 2022

Explanation

Currently, we are keeping the service worker alive for all pages open.

Instead of always keeping the service worker alive using setInterval, we will create the interval when we know the dapp is connected to permitted accounts. This has two occurrences:

  1. On load - We can observe the permitted accounts for the specific origin from metamask_getProviderState. To do this, we observe the request from pageChannel to capture the chunk id. Then we observe extensionStream to find the response with the matching chunk id because the response will not include the RPC method name.
  2. (TODO) On manual connect - Observe when a PermissionController account caveat has been updated. If PermissionController#getPermittedAccountsByOrigin returns accounts, then emit an event to the contentScript to create the keep alive interval.

In addition:

  • I removed the listeners for the page streams and reset the page streams when the extension disconnects. We need this to support the service worker being down.

Todo:

  • handle case when the keep-alive interval is not running, the service worker dies, and then the user attempts to communicate with the dapp. This will include destroying the extension streams when the service worker dies. See FIXME This is a temporary early return. comment in resetStreamAndListeners().
  • handle "On manual connect" logic described in # 2 above
  • add keep alive logic for phishing pages
  • turn off keep alive logic on disconnect
  • merge with Jyoti's PR

More Information

Fixes #15802

Screenshots/Screencaps

Before

After

Manual Testing Steps

Be sure to launch MV3 build: yarn start:mv3

Verify Service Worker stays alive while dapp is connected

  1. Open dapp that is connected with at least one account
  2. Terminate app-init.js service worker
  3. Observe that the service worker stays alive

Verify Service Worker stops running after in-activity if dapp is not connected

  1. Open dapp that is not connected to any accounts
  2. Terminate app-init.js service worker
  3. Observe that the service worker is not running

Verify Service Worker will start up after being idle
TODO

Pre-Merge Checklist

  • PR template is filled out
  • IF this PR fixes a bug, a test that would have caught the bug has been added
  • PR is linked to the appropriate GitHub issue
  • PR has been added to the appropriate release Milestone

+ If there are functional changes:

  • Manual testing complete & passed
  • "Extension QA Board" label has been applied

@digiwand digiwand added the MV3 label Sep 23, 2022
@digiwand digiwand added this to the MV3 Implementation milestone Sep 23, 2022
@digiwand digiwand requested a review from a team as a code owner September 23, 2022 06:36
@digiwand digiwand requested review from segun and removed request for a team September 23, 2022 06:36
@github-actions
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.

@digiwand digiwand marked this pull request as draft September 23, 2022 06:36
@hilvmason hilvmason added PR - P1 identifies PRs deemed priority for Extension team and removed MV3 labels Sep 23, 2022
@darkwing darkwing added the MV3 label Sep 26, 2022
@digiwand digiwand removed the MV3 label Sep 26, 2022
@digiwand
Copy link
Contributor Author

digiwand commented Oct 4, 2022

See #16075

@digiwand digiwand closed this Oct 4, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2022
@digiwand digiwand deleted the mv3-contentscript-update-init-streams-logic branch June 1, 2023 12:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR - P1 identifies PRs deemed priority for Extension team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants