Restrict the size of the permissions metadata store #8596

rekmarks · 2020-05-14T23:04:38Z

This PR bounds the size of the permissions metadata store to the number of domains with permissions.

Metadata is never deleted for domains with permissions
- This should be fine in the short to medium term
- This may be untenable in the long term, but we need to make some fundamental improvements to our JSON RPC implementation before we can address it in version 8.x
When rehydrating the store on boot, we delete the metadata of all domains without permissions, if any
At runtime, we keep track of newly added metadata origins via the _pendingSiteMetadata set
- If adding a new origin would ever grow this set beyond 100 in size, we delete the oldest entry in it, and if that oldest origin has no permissions, we also delete its metadata from the store
We do not delete metadata when permissions are removed, to prevent a degraded user experience
- We still delete the metadata of such domains on boot, but not otherwise
- This will be addressed when we can request permissions on demand in 8.x

rekmarks · 2020-05-14T23:06:05Z

app/scripts/controllers/permissions/index.js

+  /**
+   * Removes all known domains and their related permissions.
+   */
+  clearPermissions () {
+    this.permissions.clearDomains()
+    this.notifyAllDomains({
+      method: NOTIFICATION_NAMES.accountsChanged,
+      result: [],
+    })
+  }


This function was moved, because its placement didn't make any sense. It hasn't been changed.

This is the only formatting change in this PR.

Gudahtt · 2020-05-15T00:36:39Z

Do we ever want to trim metadata for sites with permissions? 🤔 I don't see the harm in keeping that. We use it in the UI anyway - we'd have to remove the permission too to avoid weird UI bugs.

metamaskbot · 2020-05-15T18:27:05Z

Builds ready [e2e75ea]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (626 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	32	97	50	21	10
		domContentLoaded	336	897	624	155	74
		load	341	898	626	154	74
		domInteractive	335	897	624	155	74

metamaskbot · 2020-05-15T18:35:13Z

Builds ready [8b4df94]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (584 ± 68 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	76	38	11	5
		domContentLoaded	339	896	582	141	67
		load	341	898	584	141	68
		domInteractive	339	895	582	141	67

app/scripts/controllers/permissions/index.js

metamaskbot · 2020-05-19T20:18:22Z

Builds ready [bec3dab]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (644 ± 31 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	90	42	15	7
		domContentLoaded	410	770	642	65	31
		load	411	771	644	65	31
		domInteractive	410	770	642	65	31

metamaskbot · 2020-05-20T21:12:22Z

Builds ready [bb8e211]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (600 ± 51 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	91	38	13	6
		domContentLoaded	345	702	598	106	51
		load	346	704	600	106	51
		domInteractive	345	702	598	106	51

metamaskbot · 2020-05-22T18:33:50Z

Builds ready [e96a18e]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (666 ± 34 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	83	40	11	5
		domContentLoaded	394	733	665	72	34
		load	396	734	666	72	34
		domInteractive	394	732	664	72	34

whymarrh

If there are 100 or more domains in the metadata store, we attempt to remove a domain without permissions before adding a new one

This only fails if there is no domain metadata for a domain without permissions

What is supposed to happen in this failure case?

app/scripts/controllers/permissions/index.js

rekmarks · 2020-05-26T17:26:26Z

@whymarrh all inline comments addressed in d3c7d89.

Regarding your question:

What is supposed to happen in this failure case?

The PR description was outdated; there is no failure case. The bullet point you referred to has been updated to read:

At runtime, we keep track of newly added metadata origins via the _pendingSiteMetadata set
- If adding a new origin would ever grow this set beyond 100 in size, we delete the oldest entry in it, and if that oldest origin has no permissions, we also delete its metadata from the store

rekmarks · 2020-05-26T17:29:22Z

@Gudahtt I didn't see your review until after responding to @whymarrh, will comment on your requested changes in turn.

metamaskbot · 2020-05-26T18:01:26Z

Builds ready [c3b7128]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (885 ± 72 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	37	133	66	28	13
		domContentLoaded	459	1165	883	150	72
		load	460	1169	885	151	72
		domInteractive	459	1165	882	150	72

Gudahtt

LGTM!

test/unit/app/controllers/permissions/permissions-controller-test.js

whymarrh

metamaskbot · 2020-05-26T21:05:12Z

Builds ready [87d1bf3]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (676 ± 52 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	87	45	17	8
		domContentLoaded	362	848	674	109	52
		load	364	850	676	109	52
		domInteractive	362	847	674	109	52

rekmarks commented May 14, 2020

View reviewed changes

rekmarks marked this pull request as ready for review May 15, 2020 18:12

rekmarks requested a review from a team as a code owner May 15, 2020 18:12

rekmarks changed the title ~~Trim permissions domain metadata~~ Restrict the size of the permissions metadata store May 15, 2020

Gudahtt requested changes May 19, 2020

View reviewed changes

app/scripts/controllers/permissions/index.js Outdated Show resolved Hide resolved

app/scripts/controllers/permissions/index.js Outdated Show resolved Hide resolved

app/scripts/controllers/permissions/index.js Outdated Show resolved Hide resolved

rekmarks force-pushed the trim-domain-metadata branch 2 times, most recently from d655755 to a8a3865 Compare May 19, 2020 16:18

rekmarks added 8 commits May 19, 2020 12:47

refactor add metadata functionality

449d281

further refactor of add metadata

3a774f3

add last updated time to metadata

aed035f

simplify trim metadata; never trim domains with metadata

9379c60

make function name consistent

f45cbca

inline popMetadata function

4899dfd

fixup! inline popMetadata function

5ef7ffe

trigger metadata deletion on cache instead of store size

bec3dab

rekmarks force-pushed the trim-domain-metadata branch from 047feba to bec3dab Compare May 19, 2020 19:47

simplify addDomainMetadata trimming logic

bb8e211

rekmarks requested a review from Gudahtt May 20, 2020 21:30

fixup! simplify addDomainMetadata trimming logic

e96a18e

whymarrh suggested changes May 26, 2020

View reviewed changes

Gudahtt reviewed May 26, 2020

View reviewed changes

clarify/remove comments

d3c7d89

add back store key comment

c3b7128

rekmarks requested review from whymarrh and Gudahtt May 26, 2020 17:50

Gudahtt previously approved these changes May 26, 2020

View reviewed changes

test/unit/app/controllers/permissions/permissions-controller-test.js Show resolved Hide resolved

test/unit/app/controllers/permissions/permissions-controller-test.js Outdated Show resolved Hide resolved

fixup! add back store key comment

87d1bf3

rekmarks dismissed Gudahtt’s stale review via 87d1bf3 May 26, 2020 20:32

Gudahtt approved these changes May 26, 2020

View reviewed changes

whymarrh approved these changes May 26, 2020

View reviewed changes

rekmarks merged commit e0b31aa into develop May 26, 2020

rekmarks deleted the trim-domain-metadata branch May 26, 2020 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict the size of the permissions metadata store #8596

Restrict the size of the permissions metadata store #8596

rekmarks commented May 14, 2020 •

edited

Loading

rekmarks May 14, 2020 •

edited

Loading

Gudahtt commented May 15, 2020

metamaskbot commented May 15, 2020

metamaskbot commented May 15, 2020

metamaskbot commented May 19, 2020

metamaskbot commented May 20, 2020

metamaskbot commented May 22, 2020

whymarrh left a comment

rekmarks commented May 26, 2020

rekmarks commented May 26, 2020 •

edited

Loading

metamaskbot commented May 26, 2020

Gudahtt left a comment

whymarrh left a comment

metamaskbot commented May 26, 2020

Restrict the size of the permissions metadata store #8596

Restrict the size of the permissions metadata store #8596

Conversation

rekmarks commented May 14, 2020 • edited Loading

rekmarks May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Gudahtt commented May 15, 2020

metamaskbot commented May 15, 2020

metamaskbot commented May 15, 2020

metamaskbot commented May 19, 2020

metamaskbot commented May 20, 2020

metamaskbot commented May 22, 2020

whymarrh left a comment

Choose a reason for hiding this comment

rekmarks commented May 26, 2020

rekmarks commented May 26, 2020 • edited Loading

metamaskbot commented May 26, 2020

Gudahtt left a comment

Choose a reason for hiding this comment

whymarrh left a comment

Choose a reason for hiding this comment

metamaskbot commented May 26, 2020

rekmarks commented May 14, 2020 •

edited

Loading

rekmarks May 14, 2020 •

edited

Loading

rekmarks commented May 26, 2020 •

edited

Loading