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

core: avoid fatal errors when collecting base artifacts #13312

Merged
merged 13 commits into from
Nov 10, 2021
Merged
8 changes: 5 additions & 3 deletions lighthouse-core/audits/installable-manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ const UIStrings = {
* for the current page encloses the scope and start URL from the manifest. */
'no-matching-service-worker': `No matching service worker detected. You may need to reload the page, or check that the scope of the service worker for the current page encloses the scope and start URL from the manifest.`,
/**
* @description Error message explaining that the manifest does not contain a suitable icon.
* @example {192} value0
*/
* @description Error message explaining that the manifest does not contain a suitable icon.
* @example {192} value0
*/
'manifest-missing-suitable-icon': `Manifest does not contain a suitable icon - PNG, SVG or WebP format of at least {value0}\xa0px is required, the sizes attribute must be set, and the purpose attribute, if set, must include "any".`,

/**
Expand Down Expand Up @@ -93,6 +93,8 @@ const UIStrings = {
'manifest-location-changed': `Manifest URL changed while the manifest was being fetched.`,
/** Warning message explaining that the page does not work offline. */
'warn-not-offline-capable': `Page does not work offline. The page will not be regarded as installable after Chrome 93, stable release August 2021.`,
/** Error message explaining that Lighthouse failed while detecting a service worker, and directing the user to try again in a new Chrome. */
'protocol-timeout': `Lighthouse could not determine if there was a service worker. Please try with a newer version of Chrome.`,
};
/* eslint-enable max-len */

Expand Down
35 changes: 29 additions & 6 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -422,21 +422,44 @@ class GatherRunner {
static async populateBaseArtifacts(passContext) {
const status = {msg: 'Populate base artifacts', id: 'lh:gather:populateBaseArtifacts'};
log.time(status);

const baseArtifacts = passContext.baseArtifacts;

// Copy redirected URL to artifact.
baseArtifacts.URL.finalUrl = passContext.url;

// Fetch the manifest, if it exists.
baseArtifacts.WebAppManifest = await WebAppManifest.getWebAppManifest(
passContext.driver.defaultSession, passContext.url);
try {
baseArtifacts.WebAppManifest = await WebAppManifest.getWebAppManifest(
passContext.driver.defaultSession, passContext.url);
} catch (err) {
log.error('GatherRunner WebAppManifest', err);
baseArtifacts.WebAppManifest = null;
}

if (baseArtifacts.WebAppManifest) {
baseArtifacts.InstallabilityErrors = await InstallabilityErrors.getInstallabilityErrors(
passContext.driver.defaultSession);
try {
if (baseArtifacts.WebAppManifest && !(baseArtifacts.WebAppManifest instanceof Error)) {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
baseArtifacts.InstallabilityErrors = await InstallabilityErrors.getInstallabilityErrors(
passContext.driver.defaultSession);
}
} catch (err) {
log.error('GatherRunner InstallabilityErrors', err);
baseArtifacts.InstallabilityErrors = {
errors: [
{
errorId: 'protocol-timeout',
Copy link
Member

@adamraine adamraine Nov 9, 2021

Choose a reason for hiding this comment

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

Protocol timeouts are probably the only errors we will encounter, but the message doesn't say anything about protocol timeouts. WDYT about a more generic name with lh- prefix like lh-collection-error?

Copy link
Collaborator Author

@connorjclark connorjclark Nov 9, 2021

Choose a reason for hiding this comment

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

The error code is exactly correct as to why this error would occur (what else would cause an error to be thrown here)? The user doesn't need to know anything about the details, "protocol" wouldn't mean anything to them.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about future code in this catch block that could emit a different error. Since we do this for any generic error, seems like it should have a generic name 🤷 . It's a nit though, so we can land without.

errorArguments: [],
},
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
],
};
}

baseArtifacts.Stacks = await Stacks.collectStacks(passContext.driver.executionContext);
try {
baseArtifacts.Stacks = await Stacks.collectStacks(passContext.driver.executionContext);
} catch (err) {
log.error('GatherRunner Stacks', err);
baseArtifacts.Stacks = [];
}

// Find the NetworkUserAgent actually used in the devtoolsLogs.
const devtoolsLog = baseArtifacts.devtoolsLogs[passContext.passConfig.passName];
Expand Down
3 changes: 3 additions & 0 deletions shared/localization/locales/en-US.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions shared/localization/locales/en-XL.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.