Skip to content

Disable console in contentscript#10040

Merged
rekmarks merged 6 commits intodevelopfrom
remove-console-contentscript
Dec 14, 2020
Merged

Disable console in contentscript#10040
rekmarks merged 6 commits intodevelopfrom
remove-console-contentscript

Conversation

@EtDu
Copy link
Copy Markdown
Contributor

@EtDu EtDu commented Dec 10, 2020

Logging in the content scripts surfaces to the external page console. SES/lockdown's logging of removals was appearing in the web console on every web page. This fix renders console useless inside of content scripts to avoid this.

@EtDu EtDu requested review from a team and kumavis as code owners December 10, 2020 09:36
@EtDu EtDu requested a review from danjm December 10, 2020 09:36
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 10, 2020

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.

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 10, 2020

Builds ready [8f88e2f]
Page Load Metrics (551 ± 29 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint336648105
domContentLoaded3456475506129
load3466475516129
domInteractive3456465506129

kumavis
kumavis reviewed Dec 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

  • contentscript lockdown logs 3x times via console.log
  • console.log only is silenced to explicitly address this (ok to expand to other console methods)
  • sentry console breadcrumb gathering should still work even if logging to console is ultimately disabled. they override console methods to collect breadcrumb then call original console method, our noop

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Dec 10, 2020

It looks like ses intentionally log this as a way to discover new APIs to add to their whitelist: https://github.com/Agoric/SES-shim/blob/1f98d31debe47a9947a5e1bfe74cc0c4b874864a/packages/ses/src/whitelist-intrinsics.js#L41 🤔 Makes sense, but I wish there was a way to disable it.

Gudahtt
Gudahtt reviewed Dec 10, 2020
View reviewed changes
@brad-decker brad-decker removed the request for review from danjm December 10, 2020 16:22
rekmarks
rekmarks reviewed Dec 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

In addition to what @Gudahtt pointed out, we have a number of log statements in contentscript.js that are useful for development in particular.

I'd also hate to lose this one: https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/contentscript.js/#L42

@rekmarks rekmarks mentioned this pull request Dec 10, 2020
Merged
@rekmarks
Copy link
Copy Markdown
Member

rekmarks commented Dec 10, 2020 •
edited
Loading

To follow up on my previous comment, if the content script blows up, the inpage script should (as of late) do enough console logging to hold us over in production.

@rekmarks rekmarks force-pushed the remove-console-contentscript branch from 8f88e2f to 316844e Compare December 11, 2020 16:46
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 11, 2020

Builds ready [316844e]
Page Load Metrics (551 ± 25 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint316947115
domContentLoaded4837025495124
load4857065515125
domInteractive4837015495124

Gudahtt
Gudahtt reviewed Dec 11, 2020
View reviewed changes
Gudahtt
Gudahtt reviewed Dec 11, 2020
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 14, 2020

Builds ready [31d1dbc]
Page Load Metrics (656 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint397154105
domContentLoaded5438676558440
load5458686568440
domInteractive5438676548440

Gudahtt
Gudahtt requested changes Dec 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One problem discovered; looks good otherwise

@rekmarks @Gudahtt
Update app/scripts/disable-console.js
a73b5b6 
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@rekmarks rekmarks requested a review from Gudahtt December 14, 2020 18:54
Gudahtt
Gudahtt approved these changes Dec 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 14, 2020

Builds ready [a73b5b6]
Page Load Metrics (623 ± 26 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint377952105
domContentLoaded5077256215326
load5097266235326
domInteractive5077256215326

@rekmarks rekmarks merged commit 69df19f into develop Dec 14, 2020
@rekmarks rekmarks deleted the remove-console-contentscript branch December 14, 2020 19:17
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kumavis kumavis kumavis left review comments

@rekmarks rekmarks rekmarks left review comments

@Gudahtt Gudahtt Gudahtt approved these changes

Assignees

@Gudahtt Gudahtt

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@EtDu @metamaskbot @Gudahtt @rekmarks @kumavis