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

MV3: Update service worker restart logic and keep-alive logic for dapp support #16075

Merged
merged 76 commits into from
Nov 14, 2022
Merged

MV3: Update service worker restart logic and keep-alive logic for dapp support #16075

merged 76 commits into from
Nov 14, 2022

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Oct 4, 2022 •
edited
Loading

Explanation

This PR:

  • update keep alive logic to run for 45 minutes for connected dapps
  • reconnects dapp streams after the service worker reconnects

Details of Implementation:

  • narrows the scope of the keep-alive logic to run only after (most) JSON-RPC requests are sent and for 45 minutes
  • re-activates the service worker after inactivity upon a JSON-RPC request
  • updates destroy / set up stream logic so that stream is set up after extension is ready
  • send a message to all the tabs from the background to inform the dapp content script that the background is ready and to set up the streams. This isn't ideal. We'd like to only send this message to tabs that are dapps. We plan to revisit this logic in a future PR as we don't yet have another, clear, way to query or filter out tabs with dapps.

Other important notes regarding this PR:

Side note:
While I was out for 2 weeks, @jpuri assisted by adding some commits. I have returned with more commits.

More Information

Fixes: #15986
Fixes: #15802

Screenshots/Screencaps

Manual Testing Steps

Test Service Worker is not kept alive on non-dapp pages:

  • Start the extension using MV3 build (yarn start:mv3)
  • Be sure not to have any dapp pages, home.html, or the popup running
  • View non-dapp website
  • Inspect the app-init.js service worker and terminate the service worker
  • Observe the service worker stops running

Test Service Worker is kept alive on dapp pages:

  • Start the extension using MV3 build (yarn start:mv3)
  • View a dapp page
  • If you are not already connected, connect to the dapp
  • Inspect the app-init.js service worker and terminate the service worker
  • Observe the service worker is kept alive

Test Service Worker can be reactivated after inactivity:

  • Start the extension using MV3 build (yarn start:mv3)
  • View a dapp page
  • Stop the service worker. Two ways:
    • Do not interact with dapp and wait 45 minutes for the service worker
    • Disconnect from the dapp (disconnect accounts) and manually terminate the service worker
  • Observe service worker is not running
  • Interact with the dapp to send a JSON-RPC request (currently, this may take a few clicks)
  • Observe the service worker is running again
    (note: There might be a delay to re-activate. @jpuri is working on a task to fix this. More details to come)

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 this to the MV3 Implementation milestone Oct 4, 2022
@digiwand digiwand requested a review from jpuri October 4, 2022 16:11
@digiwand digiwand requested a review from a team as a code owner October 4, 2022 16:11
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

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 mentioned this pull request Oct 4, 2022
Closed
11 tasks
@hilvmason hilvmason added the PR - P1 identifies PRs deemed priority for Extension team label Oct 4, 2022
@metamaskbot
Copy link
Collaborator

Builds ready [c6f042e]
Page Load Metrics (2343 ± 109 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint89142110167
domContentLoaded170828452330219105
load170828682343227109
domInteractive170828452330219105

ryanml
ryanml reviewed Oct 5, 2022
View reviewed changes
app/scripts/background.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [eff5234]
Page Load Metrics (2343 ± 143 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint851861102110
domContentLoaded200531432318285137
load200532152343298143
domInteractive200531432318285137

jpuri
jpuri reviewed Oct 10, 2022
View reviewed changes
app/scripts/contentscript.js Outdated Show resolved Hide resolved
jpuri
jpuri reviewed Oct 10, 2022
View reviewed changes
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for minor feedbacks.

@jpuri jpuri dismissed a stale review via 206837b November 10, 2022 19:06
@garrettbear garrettbear dismissed a stale review via 206837b November 10, 2022 19:13
@digiwand
Copy link
Contributor Author

digiwand commented Nov 10, 2022 •
edited
Loading

@seaona re: This is what I see manually testing FireFox on MV2: missing provider

Thanks for manually checking this! It was indeed a bug. Paired with @jpuri earlier today to fix this: 858f5d4

@digiwand digiwand dismissed a stale review via d979dcf November 10, 2022 20:41
@Gudahtt Gudahtt dismissed a stale review via d979dcf November 11, 2022 14:19
@digiwand digiwand dismissed a stale review via 858f5d4 November 11, 2022 15:35
@digiwand digiwand dismissed a stale review via 67c5533 November 11, 2022 16:08
@digiwand digiwand dismissed a stale review via 76bc0f6 November 11, 2022 16:09
@metamaskbot
Copy link
Collaborator

Builds ready [76bc0f6]
Page Load Metrics (2173 ± 119 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint8912810094
domContentLoaded163625232140247118
load165625962173247119
domInteractive163625232140247118

highlights:

storybook

@digiwand digiwand dismissed a stale review via 58db96f November 11, 2022 17:47
@digiwand digiwand dismissed a stale review via db28cf8 November 11, 2022 18:10
@metamaskbot
Copy link
Collaborator

Builds ready [db28cf8]
Page Load Metrics (2209 ± 125 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint922161122612
domContentLoaded166324922183253121
load168925472209260125
domInteractive166324922183253121

highlights:

storybook

Gudahtt
Gudahtt approved these changes Nov 11, 2022
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

brad-decker
brad-decker approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great documentation, @digiwand Thank you! LGTM

@FrederikBolding FrederikBolding dismissed a stale review via db28cf8 November 14, 2022 10:50
@digiwand digiwand requested a review from jpuri November 14, 2022 12:42
@digiwand digiwand mentioned this pull request Nov 14, 2022
Merged
8 tasks
@digiwand
Copy link
Contributor Author

digiwand commented Nov 14, 2022 •
edited
Loading

Thanks everyone for the thorough reviews and time to discuss the numerous nuances of this PR! After @jpuri reviews, we can merge. Got the ok from @jpuri and merged! 🥳🎉

I added jest tests for the new browser-runtime utils here (new PR): #16483

cc: @brad-decker @jpuri @Gudahtt @legobeat

@PeterYinusa PeterYinusa dismissed a stale review via db28cf8 November 14, 2022 14:35
@Gudahtt Gudahtt dismissed a stale review via db28cf8 November 14, 2022 14:43
@digiwand digiwand merged commit a87c175 into develop Nov 14, 2022
@digiwand digiwand deleted the mv3-dapp-sendMessage-after-SW-inactivity branch November 14, 2022 17:18
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@brad-decker brad-decker brad-decker approved these changes

@legobeat legobeat legobeat approved these changes

@ryanml ryanml Awaiting requested review from ryanml

@seaona seaona Awaiting requested review from seaona

@jpuri jpuri Awaiting requested review from jpuri

Assignees
No one assigned
Labels
needs-qa Label will automate into QA workspace PR - P1 identifies PRs deemed priority for Extension team
Projects
None yet
Milestone
MV3 Implementation
10 participants
@digiwand @metamaskbot @brad-decker @seaona @jpuri @adonesky1 @Gudahtt @ryanml @legobeat @hilvmason