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

Simplify MV3 initialization #16559

Merged
merged 5 commits into from
Nov 24, 2022
Merged

Simplify MV3 initialization #16559

merged 5 commits into from
Nov 24, 2022

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Nov 17, 2022

Explanation

The MV3 initialization logic was complicated and introduced difficult-to-reproduce race conditions when connections are made during initialization.

It seems that problems were encountered after the UI tried to connect before the background was initialized. To address this, the initialization step was delayed until after the first connection. That first connection was then passed into the initialization function, and setup properly after initialization had completed.

However, this special treatment is only given for the first connection. Subsequent connections that still occur during initialization would fail. This also results in the initialization being needlessly delayed, which is concerning given that our main performance goal is to speed it up.

Manual Testing Steps

Standard regression testing steps should apply here.

I think this might solve race conditions relating to connections during initialization, but I have not confirmed this, nor do I have steps to reproduce such problems.

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@Gudahtt Gudahtt force-pushed the init-changes branch 3 times, most recently from 7b728f7 to 1a025ef Compare November 17, 2022 19:13
@Gudahtt Gudahtt marked this pull request as ready for review November 17, 2022 19:55
@Gudahtt Gudahtt requested a review from a team as a code owner November 17, 2022 19:55
@Gudahtt Gudahtt requested a review from NidhiKJha November 17, 2022 19:55
@metamaskbot
Copy link
Collaborator

Builds ready [1a025ef]
Page Load Metrics (2115 ± 137 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint98178117199
domContentLoaded165828112095296142
load172028112115285137
domInteractive165828112095296142
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -103 bytes
  • ui: 0 bytes
  • common: 105 bytes

highlights:

storybook

app/scripts/background.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [7d5c09a]
Page Load Metrics (2148 ± 128 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint991791182010
domContentLoaded166624812127262126
load166625672148267128
domInteractive166624812127262126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 934 bytes
  • ui: 245 bytes
  • common: 336 bytes

highlights:

storybook

@metamaskbot
Copy link
Collaborator

Builds ready [e2b62ae]
Page Load Metrics (2086 ± 99 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint85173103199
domContentLoaded170224972070210101
load17022497208620799
domInteractive170224972070210101
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -50 bytes
  • ui: 0 bytes
  • common: 105 bytes

highlights:

storybook

The MV3 initialization logic was complicated and introduced race
difficult-to-reproduce race conditions when dapps connect during
initialization.

It seems that problems were encountered after the UI tried to connect
before the background was initialized. To address this, the
initialization step was _delayed_ until after the first connection.
That first connection was then passed into the initialization function,
and setup properly after initialization had begun.

However, this special treatment is only given for the first connection.
Subsequent connections that still occur during initialization would
fail. This also results in the initialization being needlessly delayed,
which is concerning given that our main performance goal is to speed it
up.
@metamaskbot
Copy link
Collaborator

Builds ready [28099cf]
Page Load Metrics (2216 ± 119 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint921697205343165
domContentLoaded169624702195243117
load169625272216249119
domInteractive169624702195243117
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -50 bytes
  • ui: 0 bytes
  • common: 105 bytes

highlights:

storybook

@metamaskbot
Copy link
Collaborator

Builds ready [e380ffb]
Page Load Metrics (2565 ± 258 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1072656366617296
domContentLoaded190942502552540260
load191542512565537258
domInteractive190942502552540260
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -50 bytes
  • ui: 0 bytes
  • common: 105 bytes

highlights:

storybook

Copy link
Contributor

@segun segun left a comment

Choose a reason for hiding this comment

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

LGTM

@Gudahtt Gudahtt merged commit 943453c into develop Nov 24, 2022
@Gudahtt Gudahtt deleted the init-changes branch November 24, 2022 13:32
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants