refactor: make `inpage.js` inject itself into the MAIN world (manifest v2 only) by davidmurdoch · Pull Request #22524 · MetaMask/metamask-extension

davidmurdoch · 2024-01-12T17:07:45Z

Description

I'm working on a new build process that is primary focused on compilation speed and simplicity. The current manifest v2 way of injecting inpage.js makes that task harder.

Affects manifest v2 only.

This PR changes the way inpage.js is injected into the MAIN world by decoupling inpage.js from contentscript.js.

This new way is very similar to the old way, except inpage.js now injects itself into the document (MAIN world).

One potential issue with this new way is that contentscript.js used to conditionally inject inpage.js into the MAIN world, but with this change the inpage.js file is just another "content_script". This might not be a problem though, as inpage.js itself checks all of the same conditions contentscript.js does, i.e., they both check shouldInjectProvider(), but the difference is that only inpage.js decides itself if it will inject a provider into the MAIN world, whereas before it was up to both contentscript.js AND inpage.js to agree to inject the provider.

To reiterate why I want to make this change: the current system requires custom logic specifically for contentscript.js and inpage.js, and inpage.js MUST complete compilation before contentscript.js can be start to be compiled. Additionally, contentscript.js needs special treatment of fs.readFileSync in order to inline inpage.js as text into itself. This complicates and slows down the build system.

One thing this PR does NOT do is speed up the build, as I didn't change the order of compilation. inpage.js is still compiled before contentscript.js, even though contentscript.js no longer depends on inpage.js. I a) didn't know how to make this change, and b) was afraid it'd complicate an already perhaps dubious PR.

Some things to look out for in testing: will this new way behave if the page is not an HTML doctype, is an XML/PDF document, doesn't have a documentElement, is on the blocked domains list, etc.

Related issues

Fixes:

Manual testing steps

Go to any web page not on out block list
Open devtools in your browser
Type window.ethereum and press in the Console
A Proxy object should be logged (not undefined)

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-01-12T17:07:58Z

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.

davidmurdoch · 2024-01-12T20:13:51Z

Added team-security label to check out the reintroduction of web_accessible_resources.

app/scripts/contentscript.js

weizman

Given some of my notes, we should discuss how to proceed

…ected as script.src into tabs

…nifest v2)

…needed there

…-extension into refactor-inject-inpage

codecov · 2024-02-05T19:19:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (65f5b81) 68.45% compared to head (9ae02ae) 68.45%.
Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #22524   +/-   ##
========================================
  Coverage    68.45%   68.45%           
========================================
  Files         1089     1089           
  Lines        42902    42902           
  Branches     11428    11428           
========================================
  Hits         29368    29368           
  Misses       13534    13534

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

brad-decker · 2024-02-06T15:29:44Z

@davidmurdoch they are precise but not accurate, if in general over the course of many builds the value is higher then it represents an actionable issue.

development/build/scripts.js

app/scripts/contentscript.js

metamaskbot · 2024-02-06T17:36:00Z

Builds ready [e5f2b68]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (798 ± 25 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	138	111	11	5
		domContentLoaded	12	27	19	4	2
		load	722	901	798	53	25
		domInteractive	12	27	19	4	2

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

kumavis

the history of inpage.js goes back to the beginning of time so it's possible browsers have changed their behavior that motivated the current setup. I believe our reasonings included:

inpage.js execution timing. our current setup was the only way we found to run before page code did
ability to modify the page globalThis to add the api. I thought contentscript couldn't do this, only modify the DOM.
clear separation of security sensitive (content script) and non-sensitive (inpage.js)
bundle factoring inpage should be a single bundle and not factored into multiple files

some of your changes here may not have affected the above

davidmurdoch · 2024-02-06T18:01:46Z

@kumavis

inpage.js execution timing. our current setup was the only way we found to run before page code did

its the same set up, but inpage.js now injects itself.

ability to modify the page globalThis to add the api. I thought contentscript couldn't do this, only modify the DOM.

inpage.js now modifies the DOM to inject itself into the DOM so it can modify globalThis

clear separation of security sensitive (content script) and non-sensitive (inpage.js)

they are now more separated, as contentscript.js doesn't depend on inpage.js now(and visa versa)

bundle factoring inpage.js should be a single bundle and not factored into multiple files

no changes here

development/build/scripts.js

metamaskbot · 2024-02-06T20:47:02Z

Builds ready [a83bff8]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (807 ± 38 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	76	159	110	22	11
		domContentLoaded	9	55	23	13	6
		load	712	956	807	79	38
		domInteractive	9	55	23	13	6

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

development/build/utils.js

weizman

looks good, few notes to discuss ☝️

Co-authored-by: weizman <weizmangal@gmail.com>

metamaskbot · 2024-02-07T16:25:45Z

Builds ready [2861767]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (732 ± 17 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	78	120	93	9	5
		domContentLoaded	9	19	15	2	1
		load	654	801	732	36	17
		domInteractive	9	19	15	2	1

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2024-02-07T17:08:15Z

Builds ready [9ae02ae]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (810 ± 25 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	81	171	103	20	9
		domContentLoaded	9	40	16	7	3
		load	729	897	810	51	25
		domInteractive	9	40	16	7	3

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

development/build/utils.js

davidmurdoch added the team-extension-client label Jan 12, 2024

davidmurdoch requested a review from a team as a code owner January 12, 2024 17:07

davidmurdoch added the team-application-security Application security team label Jan 12, 2024

weizman reviewed Jan 14, 2024

View reviewed changes

app/scripts/contentscript.js Outdated Show resolved Hide resolved

weizman reviewed Jan 14, 2024

View reviewed changes

app/scripts/contentscript.js Outdated Show resolved Hide resolved

weizman reviewed Jan 14, 2024

View reviewed changes

app/scripts/contentscript.js Outdated Show resolved Hide resolved

weizman suggested changes Jan 14, 2024

View reviewed changes

davidmurdoch marked this pull request as draft January 14, 2024 19:56

davidmurdoch added 4 commits February 2, 2024 16:54

add inpage.js to manifest's web_accessible_resources so it can be inj…

af7abed

…ected as script.src into tabs

inject inpage.js into tab as script.src instead of textContent (in ma…

c473353

…nifest v2)

use_dynamic_url in v3

1126291

let's give this a whirl

ea04f8c

davidmurdoch force-pushed the refactor-inject-inpage branch from 2ea8dfa to ea04f8c Compare February 2, 2024 22:41

davidmurdoch added 5 commits February 2, 2024 17:44

revert adding inpage to web_accessible_resources as it's no longer …

dde9913

…needed there

.

7049e29

Merge branch 'develop' into refactor-inject-inpage

861261d

lint fix

5547e46

Merge branch 'refactor-inject-inpage' of github.com:MetaMask/metamask…

f2940fc

…-extension into refactor-inject-inpage

davidmurdoch changed the title ~~refactor: inject inpage.js as a script src in manifest v2~~ refactor: make inpage.js inject itself into the MAIN world (manifest v2 only) Feb 3, 2024

don't be a dum dum (include all browser targets)

0072a28

davidmurdoch force-pushed the refactor-inject-inpage branch 2 times, most recently from 2358cc2 to 0072a28 Compare February 5, 2024 19:01

davidmurdoch added 2 commits February 5, 2024 14:01

.

79f6835

Merge branch 'develop' into refactor-inject-inpage

51b44f6

davidmurdoch changed the title ~~refactor: make inpage.js inject itself into the MAIN world (manifest v2 only)~~ refactor: make inpage.js inject itself into the MAIN world (manifest v2 only) Feb 5, 2024

Merge branch 'develop' into refactor-inject-inpage

752a443

ignore inpage in source map validation

a6f18d8

Merge branch 'develop' into refactor-inject-inpage

fe07fc6

davidmurdoch commented Feb 6, 2024

View reviewed changes

development/build/scripts.js Show resolved Hide resolved

davidmurdoch commented Feb 6, 2024

View reviewed changes

app/scripts/contentscript.js Show resolved Hide resolved

Merge branch 'develop' into refactor-inject-inpage

e5f2b68

kumavis reviewed Feb 6, 2024

View reviewed changes

kumavis requested changes Feb 6, 2024

View reviewed changes

development/build/scripts.js Show resolved Hide resolved

development/build/scripts.js Outdated Show resolved Hide resolved

development/build/scripts.js Show resolved Hide resolved

davidmurdoch mentioned this pull request Feb 6, 2024

process.env.* vars should be read once at the top of the file #22831

Open

davidmurdoch added 3 commits February 6, 2024 14:01

refactor self injection and make it its own function

89e9317

Merge branch 'develop' into refactor-inject-inpage

23c56bb

oh yeah. this rule is surely super important

a83bff8

davidmurdoch requested a review from kumavis February 6, 2024 21:20

weizman reviewed Feb 7, 2024

View reviewed changes

development/build/utils.js Outdated Show resolved Hide resolved

weizman reviewed Feb 7, 2024

View reviewed changes

development/build/utils.js Outdated Show resolved Hide resolved

weizman reviewed Feb 7, 2024

View reviewed changes

davidmurdoch and others added 2 commits February 7, 2024 10:56

Update development/build/utils.js

5b9aeae

Co-authored-by: weizman <weizmangal@gmail.com>

Update development/build/utils.js

2861767

Co-authored-by: weizman <weizmangal@gmail.com>

Merge branch 'develop' into refactor-inject-inpage

9ae02ae

weizman approved these changes Feb 7, 2024

View reviewed changes

brad-decker approved these changes Feb 8, 2024

View reviewed changes

kumavis approved these changes Feb 8, 2024

View reviewed changes

development/build/utils.js Show resolved Hide resolved

davidmurdoch merged commit ba3a20f into develop Feb 8, 2024

davidmurdoch deleted the refactor-inject-inpage branch February 8, 2024 19:22

github-actions bot locked and limited conversation to collaborators Feb 8, 2024

metamaskbot added the release-11.12.0 Issue or pull request that will be included in release 11.12.0 label Feb 8, 2024

Conversation

davidmurdoch commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Jan 12, 2024

Uh oh!

davidmurdoch commented Jan 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weizman left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brad-decker commented Feb 6, 2024

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Feb 6, 2024

Uh oh!

kumavis left a comment

Choose a reason for hiding this comment

Uh oh!

davidmurdoch commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Feb 6, 2024

Uh oh!

Uh oh!

Uh oh!

weizman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 7, 2024

Uh oh!

metamaskbot commented Feb 7, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidmurdoch commented Jan 12, 2024 •

edited

Loading

codecov bot commented Feb 5, 2024 •

edited

Loading

davidmurdoch commented Feb 6, 2024 •

edited

Loading

weizman left a comment •

edited

Loading