Report: add more robust handling of default property values stripped by LHR proto

[andrevandal]

FAQ

• Yes, my issue is not about variability or throttling.
• Yes, my issue is not about a specific accessibility audit (file with axe-core instead).

URL

https://investmentu.com

What happened?

I've set up an LHCI server with PSI auto collect.
But I'm getting one tiny issue related to https://googlechrome.github.io/lighthouse/viewer/.

When I click to open any report, the view complains:

Uncaught Error: Error rendering report: Cannot read property 'startsWith' of undefined

Debugging it, I "solved" this issue by setting the second parameter of this function to an empty string as default.

2021-08-05-10-39-05.mp4

2021-08-05-10-24-41.mp4

What did you expect?

The report should be shown properly.

What have you tried?

I've used local overwrites to debug and found the solution.

How were you running Lighthouse?

PageSpeed Insights

Lighthouse Version

8.0.0

Chrome Version

No response

Node Version

No response

Relevant log output

No response

[connorjclark]

Thanks for the report, we're looking into it now.

In the future, to save yourself extra effort when reporting bugs, what is most important (and is usually sufficient) is the stack trace of the error, which I was able to find in your video. Sharing here for others:

Repro: https://googlechrome.github.io/lighthouse/viewer/?psiurl=https://www.investmentu.com

[connorjclark]

details.url is somehow undefined. The problematic detail value comes from third-party-summary:

"entity": {
    "type": "link",
    "text": "PushCrew"
}

Looks good, right? Except remember: empty strings are dropped in LR because of proto shenanigans. https://github.com/GoogleChrome/lighthouse/blob/3ff45bb/core/lib/proto-preprocessor.js#L76

We should add a check in safelySetHref that the url is string. And do a full pass on all string values that are used to renderer stuff in the report renderer, making sure an empty/missing value won't explode things.

[connorjclark]

Reason why empty strings get dropped (not documented in our code so I asked shane): proto json encoder/decoder, by default, does not include fields if the value is the default for that type. This can be configured, but not on a per-type basis, so we must keep that option off. The issue is not "extra bloat"; but that not-present and the default value has different semantics in JS* (can someone confirm this?).

(I want to get a proper explanation so it can be added as a comment in prepareForProto)

We should add a check in safelySetHref that the url is string. And do a full pass on all string values that are used to renderer stuff in the report renderer, making sure an empty/missing value won't explode things.

Alternatively: should we replace empty strings in LR with something like '__LH__EMPTYSTRING__' and then reset to '' in renderer? would have to be on a breaking version.

[andrevandal]

thank you, dude!
I appreciate your effort!

I open a PR that works for me: #12869

It just prevents safelySetHref to break hehe

maybe it can help

[paulirish]

@derevandal thanks for the PR! Agree that'll definitely fix it. We will probably reach a bit deeper to try to fix the root issue, but we definitely appreciate you drafting that.

@connorjclark @brendankenny fixing the root issue feels like.. we address any of these situations. I think that's anything inside a proto Struct... which is just Audit.Details. And then within there its strings of '', numbers of 0, booleans of false, etc.
It does seem that we can allow our renderer to handle these dropped-defaults versions of LHR Audit.details or we do the bookmarked LH replacement thing that connor mentioned.
I bring up the larger scope issue in case we could catch some currently-silent bugs by tweaking our renderer types.

[brendankenny]

Yeah, it's definitely a bummer. Error handling actually came up in the original review for third-party-summary, but we only considered bad URLs (like that empty string), not undefined. The try/catch happened to handle undefined as well so we never noticed, but there's no test for undefined (because why would there be), which would have caught the url.startsWith('#') issue introduced in #12796.

Alternatively: should we replace empty strings in LR with something like '__LH__EMPTYSTRING__' and then reset to '' in renderer? would have to be on a breaking version.

Converting all string LHR properties to string|undefined would be a major regression (and if we did it by hand for only the properties where this is possible we risk this happening again), so maybe we really should consider this option.

Maaaybe there's something in jspb in the years since we got this working where we have some wiggle room to make this better?

[brendankenny]

I think that's anything inside a proto Struct... which is just Audit.Details. And then within there its strings of '', numbers of 0, booleans of false, etc.
It does seem that we can allow our renderer to handle these dropped-defaults versions of LHR Audit.details

we could do this (we do it automatically to strip out LH.IcuMessages and convert them all to strings, for instance), but that's almost every single value in audit details :/

[brendankenny]

for testing: proto-preprocessor strips empty strings, so we could take one of our tests that checks a report renders sample_v2 without error (is it just the viewer pptr test?) and run it with processForProto(sample_v2) as well (though we might need a different LHR with more empty strings, it looks like it wouldn't have hit this issue).

We could expand processForProto's stripping of default values to numbers and booleans, too

[connorjclark]

for testing ...

sample json doesn't include this particular case, and if there are others they are likely behind conditionals/fallbacks that we may not know to trigger. So it isn't full-proof, but it's still a good idea.

Maaaybe there's something in jspb in the years since we got this working where we have some wiggle room to make this better?

I'll double-check if we really can't disable the default-exclusion behavior for only strings.

[brendankenny]

I'll double-check if we really can't disable the default-exclusion behavior for only strings.

I guess it may not work for auditDetails in particular, since we have a lot of optional properties. I assume the JSON serializer won't be able to tell the difference between an optional string and a required empty string, since both will come as 0 bytes over the wire, so it needs to treat them both as '' or both as undefined. That would be fine as long as we're doing falsy checks, but it's still broken on a type level.

[connorjclark]

No immediate plans to fix root cause.

We would like to:

1. consider what analytics we have that will let us know about these renderer errors when they happen
2. consider how to use our staging traffic stress test to try out new renderer at-scale before releasing to prod