Report app manifest errors #2349
Conversation
|
a dbw_tester bad manifest might not be a terrible idea |
There was a problem hiding this comment.
I'd like to check this out on some bad manifests before we merge. I'm still stuck in the #1666 mindset, that we'll be dropping more detailed error messages from our parser because we'll never try parsing it. I don't know what the protocol considers critical errors though.
at first glance it appears to only be used for JSON errors, but maybe there are errors higher up the chain that are also labeled critical? If not, not sure how we were labelling JSON parse errors as "unable to retrieve"... |
lighthouse-core/gather/driver.js
Outdated
| // We're not reading `response.errors` however it may contain critical and noncritical | ||
| // errors from Blink's manifest parser: | ||
| // https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError | ||
| // handle errors |
There was a problem hiding this comment.
maybe make this explicit and say // Handle JSON syntax errors or something?
There was a problem hiding this comment.
sg. also detailed what issues these deal with.
lighthouse-core/gather/driver.js
Outdated
| // handle errors | ||
| if (response.errors) { | ||
| response.errors.forEach(error => { | ||
| log.warn('driver', 'Manifest error', error.message); |
There was a problem hiding this comment.
not sure if we want to warn on all of these (if it's not a critical warning the manifest is still functional, and we don't warn on something like all uncaught exceptions), but don't feel strongly :)
There was a problem hiding this comment.
i prefer warning on all for now. can quiet it if its annoying. But right now the repeated debugStrings in the report are the more annoying thing.
lighthouse-core/gather/driver.js
Outdated
| }); | ||
| const criticalError = response.errors.find(error => error.critical); | ||
| if (criticalError) { | ||
| return reject(new Error(`Manifest error: ${criticalError.message}`)); |
There was a problem hiding this comment.
while you're in here, do you want to flatten this promise? Rather than wrapping, can just return this.sendCommand()... and these return reject(new Error())s can just be throw new Error()
There was a problem hiding this comment.
patrick did this mostly in #2549
paulirish
force-pushed
the
reportmanifesterrrors
branch
from
9b0bb61
to
e2129d7
Compare
paulirish
force-pushed
the
reportmanifesterrrors
branch
from
e2129d7
to
3d42be2
Compare
|
updated. @brendankenny ptal |
| throw new Error(`Manifest error: ${criticalError.message}`); | ||
| } | ||
| } | ||
| // Handle failed manifest request or 404 |
There was a problem hiding this comment.
this doesn't usually work (or even get this far), fyi. DevTools will happily try to parse a 404 response like "Cannot GET /no-manifest.json" and return a JSON.parse error instead of the fact that it 404ed
There was a problem hiding this comment.
ah! good call. thanks.
| // Possible critical issues: invalid JSON (location reported) or manifest root is an array | ||
| const criticalError = response.errors.find(error => error.critical); | ||
| if (criticalError) { | ||
| throw new Error(`Manifest error: ${criticalError.message}`); |
There was a problem hiding this comment.
We really don't want errors like these reported to Sentry, but this does seem like a valid thing to display as a fatal error for an audit, I guess we should bring back ExpectedError?
fixes #2348
If the manifest has JSON errors, it currently fails with something like this:
But we do have more useful errors coming from protocol we could share... for example.
This PR implements that.
Also driveby to remove superfluous word "required"