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

Fix Lighthouse timeout on pages with video players #6704

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

chosak
Copy link
Member

@chosak chosak commented Sep 27, 2021

Currently, Lighthouse will consistently timeout when run against any website page that uses our VideoPlayer module. This is because of a potential infinite loop in its JS code that can be triggered by a certain Lighthouse test. This commit fixes that bug and allows for successful audits of these pages with Lighthouse.

See here for an example of a failing Lighthouse test on one of these pages.

The reason for the failures is down in the Lighthouse code -- currently LH runs an audit called redirects-http that verifies that a request to a URL with HTTP always gets redirected to HTTPS. Instead of using a HEAD request or something similar to check this, it tries making the request in Chrome. To speed up what is an expected redirect, the request disables the loading of certain assets in Chrome, including images.

Image loading being disabled seems to trigger some error handling code we have in the VideoPlayer: if the video thumbnail image fails to load, we load a default thumbnail. Currently, though, this is implemented with the 'error' listener of the <img> tag, which means that if all image loads are disabled, there will be an infinite loop of errors:

  • Try loading the correct thumbnail; this fails so
  • Error handler tries to load the default thumbnail; this fails so
  • Error handler tries to load the default thumbnail; this fails so
  • Error handler tries to load the default thumbnail; this fails so
  • etc.

This causes Lighthouse to timeout when run in the console and essentially hang forever when run in the browser.

This commit changes the logic so that we don't try loading the default thumbnail if we've already tried and failed to do so once.

How to test this PR

To test this change, first verify that Lighthouse hangs against production:

npx lighthouse https://www.consumerfinance.gov/about-us/blog/economic-impact-payment-prepaid-card/ --only-audits=redirects-http

Then run a local server with this change (first build using yarn run gulp scripts) and confirm that it works properly:

npx lighthouse http://localhost:8000/about-us/blog/economic-impact-payment-prepaid-card/ --only-audits=redirects-http

Notes

Edit to add: it looks like redirects-http is scheduled to be removed in the next major version of Lighthouse.

Checklist

  • PR has an informative and human-readable title
    • PR titles are used to generate the change log in releases; good ones make that easier to scan.
    • Consider prefixing, e.g., "Mega Menu: fix layout bug", or "Docs: Update Docker installation instructions".
  • Changes are limited to a single goal (no scope creep)
  • Code follows the standards laid out in the CFPB development guidelines
  • Future todos are captured in comments and/or tickets

Currently, Lighthouse will consitently timeout when run against any
website page that uses our VideoPlayer module. This is because of a
potential infinite loop in its JS code that can be triggered by a
certain Lighthouse test. This commit fixes that bug and allows for
successful audits of these pages with Lighthouse.

See here [0] for an example of a failing Lighthouse test on one of these
pages [1].

The reason for the failures is down in the Lighthouse code -- currently
LH runs an audit called redirects-http that verifies that a request to a
URL with HTTP always gets redirected to HTTPS. Instead of using a HEAD
request or something similar to check this, it tries making the request
in Chrome. To speed up what is an expected redirect, the request
disables the loading of certain assets in Chrome, including images [2].

Image loading being disabled seems to trigger some error handling code
we have in the VideoPlayer: if the video thumbnail image fails to load,
we load a default thumbnail. Currently, though, this is implemented with
the 'error' listener of the <img> tag, which means that if _all_ image
loads are disabled, there will be an infinite loop of errors:

- Try loading the correct thumbnail; this fails so
- Error handler tries to load the default thumbnail; this fails so
- Error handler tries to load the default thumbnail; this fails so
- Error handler tries to load the default thumbnail; this fails so
- etc.

This causes Lighthouse to timeout when run in the console and
essentially hang forever when run in the browser.

This commit changes the logic so that we don't try loading the default
thumbnail if we've already tried and failed to do so once.

To test this change, first verify that Lighthouse hangs against
production:

  npx lighthouse https://www.consumerfinance.gov/about-us/blog/economic-impact-payment-prepaid-card/ --only-audits=redirects-http

Then run a local server with this change (first build using `yarn run
gulp scripts`) and confirm that it works properly:

  npx lighthouse http://localhost:8000/about-us/blog/economic-impact-payment-prepaid-card/ --only-audits=redirects-http

[0] https://github.com/cfpb/cfgov-lighthouse/runs/3715284303?check_suite_focus=true
[1] https://www.consumerfinance.gov/about-us/blog/economic-impact-payment-prepaid-card/
[2] https://github.com/GoogleChrome/lighthouse/blob/a691b25aff50f5b0790f661c16469b1a3a134294/lighthouse-core/config/default-config.js#L179
Copy link
Member

@anselmbradford anselmbradford left a comment

Choose a reason for hiding this comment

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

Works as described and looks like an elegant solution. Full-stack Andy 🦸‍♂️

@chosak chosak merged commit 8f8c632 into main Sep 28, 2021
@chosak chosak deleted the fix/lighthouse-videos-hang branch September 28, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants