metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Published 20 hours ago • verified publisher icon MetaMask

Fix #14846 - Inject provider for MV3 via app-init

Open darkwing opened this issue 3 years ago • 13 comments

Supercedes https://github.com/MetaMask/metamask-extension/pull/15262

Explanation

If we use the src of <script> to inject our inpage provider, the code is async and won't be initialized immediately in the page. By using the new world: MAIN property for content scripts, we can ensure the script is injected at the correct time.

More Information

Manual Testing Steps

  1. Start via yarn start:mv3
  2. Refresh the extension in the Extension Manager, which ensures flushing cache
  3. Open the test dapp
  4. Type window.ethereum in the console, ensure it's there
  5. Use the test dapp buttons, ensure the popup appears

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

darkwing avatar Aug 03 '22 15:08 darkwing

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.

github-actions[bot] avatar Aug 03 '22 15:08 github-actions[bot]

One difference with this method is that we don't use shouldInjectProvider which does various checks for Doctype, host, etc. Not sure if we'll find that perfectly acceptable.

darkwing avatar Aug 03 '22 16:08 darkwing

One difference with this method is that we don't use shouldInjectProvider which does various checks for Doctype, host, etc. Not sure if we'll find that perfectly acceptable.

We can move that check within inpage.js itself. If it's run first-thing, even before the imports, it should let us stop execution early at least.

I'm not sure if that covers all the reasons we had that check though. I know the hard-coded domains listed there had incompatibilities with our inpage script (probably relying on globals or doing some sloppy message handling), so moving the check to inpage.js should do the trick there. But for the suffix check I'm less sure. I think that was to prevent injecting into non-web contexts like PDFs, but... maybe the contentscript just ignores those so we don't need that check anymore.

Gudahtt avatar Aug 03 '22 16:08 Gudahtt

Builds ready [053cd67]
Page Load Metrics (1672 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint811601072411
domContentLoaded15152069165114771
load15162069167214168
domInteractive15152069165114771

metamaskbot avatar Aug 03 '22 17:08 metamaskbot

Builds ready [f6bf5de]
Page Load Metrics (1693 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint92169109189
domContentLoaded1541192316919947
load15411943169310149
domInteractive1541192316919947

metamaskbot avatar Aug 03 '22 18:08 metamaskbot

Builds ready [c5afdfa]
Page Load Metrics (2205 ± 121 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1002519258519249
domContentLoaded173825632184237114
load173826822205252121
domInteractive173825632184237114

metamaskbot avatar Aug 03 '22 20:08 metamaskbot

Builds ready [5ed15fc]
Page Load Metrics (1789 ± 79 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint931711282010
domContentLoaded16052409176716178
load16212409178916479
domInteractive16052408176716178

metamaskbot avatar Aug 04 '22 18:08 metamaskbot

Builds ready [76c22ec]
Page Load Metrics (1831 ± 95 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint95142110136
domContentLoaded16062384179618086
load16062384183119795
domInteractive16062384179618086

metamaskbot avatar Aug 05 '22 15:08 metamaskbot

Builds ready [61a7ecf]
Page Load Metrics (1756 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint912411243517
domContentLoaded1604196617408943
load1622198917569746
domInteractive1604196617408943

metamaskbot avatar Aug 05 '22 19:08 metamaskbot

Builds ready [f0a4e3a]
Page Load Metrics (1738 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint87159114189
domContentLoaded15912097173212660
load15912097173812359
domInteractive15912097173212660

metamaskbot avatar Aug 07 '22 21:08 metamaskbot

Builds ready [61a7ecf]
Page Load Metrics (1655 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint90123104115
domContentLoaded1523194716519947
load1541194716559646
domInteractive1523194716519947

metamaskbot avatar Aug 08 '22 17:08 metamaskbot

Amazing! I've tested and overall looks great! :100: Just one thing, I noticed a slightly strange behaviour, that I am not sure if might be on the Dapp side (meaning Dapps should adapts something on their code?) or on our side. Basically, when I stop service worker and re-start again, after unlocking MM I can see both on MM and on the Dapp that I am still Connected to the Dapp. Even though, when I continue triggering actions on the Dapp nothing happens. It is not until I refresh the Dapp that I can continue operating normally. Is this the expected behaviour? @darkwing @naugtur

https://user-images.githubusercontent.com/54408225/183867630-4abdd2a3-357b-4b14-9f54-1c990f756f5b.mp4

seaona avatar Aug 10 '22 09:08 seaona

It's not expected. Something didn't reconnect. But I'm not familiar with that logic, I'll leave figuring it out to someone more qualified 😉 Thanks for finding it!

naugtur avatar Aug 10 '22 12:08 naugtur

@seaona @naugtur This is expected behavior at the moment because we need to reconnect the streams when the Service Worker restarts.

This is the issue related here: https://github.com/MetaMask/metamask-extension/issues/14845 I have a draft PR attempting to fix this: https://github.com/MetaMask/metamask-extension/pull/15494

digiwand avatar Aug 11 '22 09:08 digiwand

great! so for QA side this PR is looking good! @darkwing I will check the fix on the other PR once ready @digiwand Thank you all!!

seaona avatar Aug 11 '22 14:08 seaona

Builds ready [c46fc39]
Page Load Metrics (1866 ± 72 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint882219299582279
domContentLoaded16802228183713766
load17032355186615072
domInteractive16802228183713766

metamaskbot avatar Aug 15 '22 16:08 metamaskbot

Builds ready [c46fc39]
Page Load Metrics (1795 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint901891132512
domContentLoaded1595195717659244
load1631195717958239
domInteractive1595195717659244

metamaskbot avatar Aug 19 '22 17:08 metamaskbot