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

Add SES lockdown and Sentry to all pages #10013

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Dec 7, 2020

When the SES lockdown was added in #9729, the lockdown and the Sentry initialization were migrated from the main bundle into separate modules, which were run as separate <script> tags. These extra tags were accidentally omitted for home.html and notification.html. As a result Sentry was not initialized on these pages, so any errors thrown on them would not be collected. They also do not benefit from the SES lockdown.

The SES lockdown and Sentry initialization modules have been added to both pages where they were missing.

When the SES lockdown was added in #9729, the lockdown and the Sentry
initialization were migrated from the main bundle into separate
modules, which were run as separate `<script>` tags. These extra tags
were accidentally omitted for `home.html` and `notification.html`. As
a result Sentry was not initialized on these pages, so any errors
thrown on them would not be collected. They also do not benefit from
the SES lockdown.

The SES lockdown and Sentry initialization modules have been added to
both pages where they were missing.
@rekmarks rekmarks requested review from EtDu and kumavis December 7, 2020 21:39
@metamaskbot
Copy link
Collaborator

Builds ready [7b1ee52]
Page Load Metrics (586 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint489060115
domContentLoaded3327225858239
load3337245868239
domInteractive3317215858239

@Gudahtt
Copy link
Member Author

Gudahtt commented Dec 7, 2020

Steps to reproduce:

  1. Introduce an error somewhere in the application UI. Usually I add a new Error("test") to the render function of the Home component.
  2. Build the extension using yarn dist (Sentry errors aren't reported in dev builds)
  3. Load the page with the error and open the network tab of dev tools
  4. Reload the page, and unlock if necessary. You should see the error page.

On develop, there will be no network call to report the error to Sentry. With this PR, there will be a network call.

@Gudahtt Gudahtt marked this pull request as ready for review December 7, 2020 21:54
@Gudahtt Gudahtt requested a review from a team as a code owner December 7, 2020 21:54
@Gudahtt Gudahtt requested a review from danjm December 7, 2020 21:54
@Gudahtt Gudahtt merged commit 7349801 into develop Dec 7, 2020
@Gudahtt Gudahtt deleted the add-sentry-and-ses-lockdown-to-all-pages branch December 7, 2020 22:16
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 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.

3 participants