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

Disable console in contentscript #10040

Merged
merged 6 commits into from
Dec 14, 2020
Merged

Conversation

EtDu
Copy link
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 kumavis and a team as code owners December 10, 2020 09:36
@EtDu EtDu requested a review from danjm December 10, 2020 09:36
@github-actions
Copy link
Contributor

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
Collaborator

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

Copy link
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
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.

@brad-decker brad-decker removed the request for review from danjm December 10, 2020 16:22
Copy link
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
Copy link
Member

rekmarks commented Dec 10, 2020

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
Collaborator

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

@metamaskbot
Copy link
Collaborator

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

Copy link
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.

One problem discovered; looks good otherwise

app/scripts/disable-console.js Outdated Show resolved Hide resolved
@rekmarks rekmarks requested a review from Gudahtt December 14, 2020 18:54
Copy link
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
Collaborator

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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants