metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard
verified publisher icon MetaMask

MV3: contentscript.js - re-activate streams when Service Worker terminates and then resets

Open digiwand opened this issue 3 years ago • 2 comments

Explanation

This PR disconnects and recreates the streams when the Service Worker resets. This PR reuses some patterns found here: https://github.com/MetaMask/metamask-extension/pull/14781.

The setup of the streams pretty much remains the same. The logic has been split into functions to be reused and to separate concerns. There is additional logic to destroy the streams and then reestablish them.

Todo:

  • troubleshoot handshake between contentscript stream and inpage stream
  • possibly reset streams created for inpage.js when Service Worker is re-activated
  • Get feedback on current work
  • Apply reset logic to setupPhishingStream()

More Information

  • Fixes #14845

Screenshots/Screencaps

Manual Testing Steps

  1. Run yarn start:mv3
  2. Open test-dapp
  3. Check that streams work by using dapp
  4. Terminate the app-init.js service worker
  5. Check that dapp continues to work

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 avatar Aug 05 '22 20:08 digiwand

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 05 '22 20:08 github-actions[bot]

Builds ready [e0ee038]
Page Load Metrics (1682 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint893331155125
domContentLoaded1561184216727636
load1574184216827938
domInteractive1561184216727636

metamaskbot avatar Aug 05 '22 21:08 metamaskbot

Builds ready [1e7ada7]
Page Load Metrics (1863 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint882681243718
domContentLoaded1730207918479445
load1731207918639646
domInteractive1730207918479445

metamaskbot avatar Aug 16 '22 16:08 metamaskbot

Builds ready [a78f894]
Page Load Metrics (1950 ± 114 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint93157114157
domContentLoaded162025201932236113
load165525201950238114
domInteractive162025201932236113

metamaskbot avatar Aug 16 '22 19:08 metamaskbot

Builds ready [cf9d25d]
Page Load Metrics (1826 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint903551215627
domContentLoaded15642279182117986
load15642279182617785
domInteractive15642279182117986

metamaskbot avatar Aug 17 '22 17:08 metamaskbot

Builds ready [2289602]
Page Load Metrics (2059 ± 131 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint962581294120
domContentLoaded167726422022282135
load167726422059273131
domInteractive167726422022282135

metamaskbot avatar Aug 18 '22 20:08 metamaskbot

Builds ready [23649db]
Page Load Metrics (2013 ± 101 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint922142231442212
domContentLoaded16892555197919493
load168925552013211101
domInteractive16892555197919493

metamaskbot avatar Aug 19 '22 08:08 metamaskbot

Builds ready [0e41b17]
Page Load Metrics (1768 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint901619193327157
domContentLoaded15572065174613163
load16232123176813967
domInteractive15572065174613163

metamaskbot avatar Aug 20 '22 05:08 metamaskbot

Builds ready [3a09691]
Page Load Metrics (1817 ± 76 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint90162118199
domContentLoaded15972173178616177
load16192173181715976
domInteractive15972173178616177

metamaskbot avatar Aug 20 '22 17:08 metamaskbot

Builds ready [53eec15]
Page Load Metrics (1660 ± 36 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint861707257464223
domContentLoaded1544178516467335
load1547180316607436
domInteractive1544178516467335

metamaskbot avatar Aug 21 '22 05:08 metamaskbot

Builds ready [f3bcd73]
Page Load Metrics (2064 ± 84 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1101954230396190
domContentLoaded17092318203017885
load17092318206417584
domInteractive17092318203017885

metamaskbot avatar Aug 22 '22 12:08 metamaskbot

Builds ready [2550c01]
Page Load Metrics (1735 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint963711325828
domContentLoaded15341987171812057
load15752092173512258
domInteractive15341987171712057

metamaskbot avatar Aug 22 '22 14:08 metamaskbot

Builds ready [290d2c5]
Page Load Metrics (1732 ± 44 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint96143113126
domContentLoaded1595185017167637
load1595192417329244
domInteractive1595185017167637

metamaskbot avatar Aug 22 '22 17:08 metamaskbot

Wow, this appears to be working great!

darkwing avatar Aug 22 '22 18:08 darkwing

Builds ready [14db9cc]
Page Load Metrics (1863 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint100174128189
domContentLoaded16492056183512058
load16702077186312962
domInteractive16492056183512058

metamaskbot avatar Aug 25 '22 16:08 metamaskbot

[out of scope] confirmed that the provider error is intermittent after seeing it pass here https://github.com/MetaMask/metamask-extension/pull/15494/commits/14db9cc4ff8a656f90bc2b96a08c38fc60d9feff

@jpuri @Gudahtt

Screenshot 2022-08-23 at 3 02 59 AM

digiwand avatar Aug 25 '22 17:08 digiwand

Builds ready [af89556]
Page Load Metrics (1746 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint881811262010
domContentLoaded1592201117248842
load1616203017469545
domInteractive1592201117248842

metamaskbot avatar Aug 25 '22 21:08 metamaskbot

Hey @digiwand : small suggestion using / in branch name creates trouble in running git commands. Try to avoid that.

jpuri avatar Aug 26 '22 09:08 jpuri

Hey @jyoti, thanks very much for bringing this up! I didn't know it was causing difficulties to work with. I'll avoid adding / in the branch names from here on

digiwand avatar Aug 26 '22 10:08 digiwand

Builds ready [23fe6e4]
Page Load Metrics (1832 ± 74 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint991856212378181
domContentLoaded16352211180715575
load16392238183215474
domInteractive16352211180715575

metamaskbot avatar Aug 26 '22 10:08 metamaskbot

@digiwand : service worker seems to always stay active after these changes even is extension is not used, do you plan to fix that.

I think we should keep service worker running only for connected dapps.

jpuri avatar Aug 26 '22 10:08 jpuri

@jpuri I agree we should only keep the service worker running for connected dapps, when the pop-up is open, and when the home.html page is open.

Let me double-check this...

digiwand avatar Aug 26 '22 15:08 digiwand

Hey @jpuri, good catch. I think this fixes it https://github.com/MetaMask/metamask-extension/commit/d259d04d8c5c9226c0b98bb7397fdb485dbdaa08 (merged commit)

digiwand avatar Sep 05 '22 10:09 digiwand

Builds ready [420c7bb]
Page Load Metrics (1799 ± 79 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint96168119178
domContentLoaded15912047177214268
load16092175179916579
domInteractive15912047177214268

metamaskbot avatar Sep 05 '22 16:09 metamaskbot

Overall behavior looks fine :) . I have seen a couple of console errors that might be worth to double-check:

  1. On the dev console, I'm seeing the error write after end on stream-utils at the moment of loading the Extension

https://user-images.githubusercontent.com/54408225/188630588-6f5af261-f95a-4c5f-a29d-dddb404dd0b4.mp4

  1. After Importing a Wallet, I see the error document is not defined on the dev console (this will also appear when I stop service worker and re-initiate - see point 4)

Screenshot from 2022-09-06 14-06-06

  1. When I Connect to Test Dapp and try to perfom a Send tx or to Deploy Contract, I see the error failed to parse tx data. This also happens when I Submit a tx.

https://user-images.githubusercontent.com/54408225/188631337-19c965cc-40dc-4e44-968a-e64427d10407.mp4

  1. When I stop the Service Worker, I see the following errors:
    • Duplicate script ID 'inpage'
    • Incorrect password
    • ReferenceError: document is not defined

https://user-images.githubusercontent.com/54408225/188632278-86412277-5403-4188-ab69-8ab0e71ff661.mp4

In this case, when I stop service worker MM is logged out and I need to log in again with the password. Is this the expected behaviour?

seaona avatar Sep 06 '22 12:09 seaona

@seaona Thank you for the thorough testing! I think there is some crossover testing of https://github.com/MetaMask/metamask-extension/pull/14781 and this PR, which is actually great you tested both. I believe none of these are blockers to this PR. This caught some out-of-scope issues, and I created one follow-up issue: https://github.com/MetaMask/metamask-extension/issues/15775

  1. [related to other service worker PR] This is actually testing service worker behavior for the home.html page and not the dapp pages. This PR re-activates streams for script/contentscript.js which is loaded in dapp pages which summons popup.html. Then https://github.com/MetaMask/metamask-extension/pull/14781 re-activates streams for script/ui.js which is loaded on our extension home.html page cc: @jpuri

  2. [unrelated] This document is not defined bug appears to be related to the Ledger iframe which will be supported in another PR https://github.com/MetaMask/metamask-extension/issues/14183. For now, I've been loading a wallet with no hard wallets associated with it.

  3. [I think out of scope] Nice catch. I'm seeing this issue on both home.html and popup.html. This may or may not be related to the service worker and streams. I created an issue ticket to handle this separately from this PR: https://github.com/MetaMask/metamask-extension/issues/15775

  4. When I stop the Service Worker, I see the following errors:

  • [non-blocker] Duplicate script ID 'inpage': I've also observed this on both home.html and dapp pages. I don't believe this is a blocker. This error appears to be more related to the service worker implementation than this PR - though, in a way, it may be related
  • [unrelated] Incorrect password: I believe this is being supported by https://github.com/MetaMask/metamask-extension/issues/15050
  • [unrelated] ReferenceError: document is not defined Please refer to # 2 relating to hard wallet support

digiwand avatar Sep 09 '22 07:09 digiwand

Builds ready [0d05cfc]
Page Load Metrics (1763 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint91170117189
domContentLoaded15422083173317684
load15422085176318689
domInteractive15422083173317684

metamaskbot avatar Sep 09 '22 11:09 metamaskbot

Hey @digiwand , I found an issue while working on task of retrying DAPP requests, if I add a bit of delay in resetStreamAndListeners method by adding a timeout, stream re-connection does not happen.

Reconnection should always happen irrespective of delay.

jpuri avatar Sep 13 '22 13:09 jpuri

I add a bit of delay in resetStreamAndListeners method by adding a timeout, stream re-connection does not happen.

To follow up from the call we just had discussing this: I suspect that this method of simulating a delayed startup is flawed. I would expect the method resetStreamAndListeners to execute synchronously without any delay even if the service worker was slow to start up - just that we'd see a timeout disconnect event after the fact. But I'm not confident in that guess.

Needs further testing. Suggested next steps for testing that was to add an artificial delay during service worker initialization, before streams were setup. No need to block this PR though.

Gudahtt avatar Sep 13 '22 20:09 Gudahtt