core(trace-processor): ignore navigationStart with falsy document url

[adamraine]

Based on the description for #5917, it seems like we should be ignoring nav starts with falsy document url but the condition doesn't do that.

This fixed the third item in #13847 for me locally.

Will land after #13844

[brendankenny]

I bisected the issue, not sure why the range is so big:

You are probably looking for a change made after 981364 (known good), but no later than 981430 (first known bad).
CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.

https://chromium.googlesource.com/chromium/src/+log/064a42bec17f70ca7c19dd482690980d1899b92f..8946e416b5988c8776c26b1e0801e5b1fe29a309

For some reason I can't get the per-revision bisect to work, so if someone wants to take a stab be my guest (to repro, just yarn smoke redirects-client-paint-server)

[paulirish]

I bisected the issue, not sure why the range is so big:

i tried and got a slightly smaller range. (16 compared to your 66)

my commands

python bisect_builds.py --verbose -a mac -g 981365 -b 981430 --verify-range  --use-local-cache -c "%p --user-data-dir=/tmp/sdfl3k1 --no-first-run --remote-debugging-port=9222"
# along with making port=0 to port=9222 in smokehouse/…/cli.js
# and then of course `yarn smoke redirects-client-paint-server`

(i tried with -o and it didnt find builds so just got rid of it. god i hate those -o -r switches and the world's worst docs page)

anyway:
https://chromium.googlesource.com/chromium/src/+log/613d5a65cecfe6db5dbaf951f8dadc0671bc204a..81aef823bb74d322f94715791944034e74df5524

looking at it, i very much suspect it's Reland "Enable same-site bfcache by default" (I3002d4a7) · Gerrit Code Review https://chromium-review.googlesource.com/c/chromium/src/+/3439311

[brendankenny]

maybe some variation on

Suggested change

	if (!event.args.data?.documentLoaderURL) return false;
	// COMPAT: support pre-m67 test traces before `args.data` added to all navStart events.
	// TODO: remove next line when old test traces (progressive-app-m60.json) are updated.
	if (!event.args.data) return true;
	if (!event.args.data?.documentLoaderURL) return false;

?

I'm 100% good with updating well past m60 for a test trace, but we could fix the test failure to unblock CI and then update the tests? Maybe it would be a trivial update, but we'd want to make sure it exercises the same execution paths, and it's currently used in at least 227 tests, probably more that just aren't currently failing with NO_NAVSTART.

[brendankenny]

for future reference, CL from @deepanjanroy that added args.data to all navStart events: https://chromium-review.googlesource.com/975986

[adamraine]

Ope I just thought that the unit failures were all #13846. Didn't think to check.

[adamraine]

There were some test traces with event.args.data but no documentLoaderURL. I updated this to check if documentLoaderURL is specifically undefined.

[brendankenny]

haha, ok then, so when we need to update the trace tests just removing that one line will be where to start

[brendankenny]

looking at it, i very much suspect it's Reland "Enable same-site bfcache by default" (I3002d4a7) · Gerrit Code Review https://chromium-review.googlesource.com/c/chromium/src/+/3439311

Nice work! -o thing is super confusing, and the message If you would like to bisect on release builds, try running with -r option instead. Previous -o options is currently changed to -r option as continous official builds were added for bisect does not help (or explain itself well).

Change seems plausible. I couldn't resist but update @adamraine's https://crbug.com/1308805#c1 to maybe find out more :)

[brendankenny]

Will land after #13844

@adamraine possible to land first? Would feel better landing with green smoke tests.

Change seems good to me. Looking at the trace event generation:

dict.Add("documentLoaderURL", document_loader_ ? document_loader_->Url().GetString() : "");

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/document_load_timing.cc;l=129;drc=e5a38eddbdf45d7563a00d019debd11b803af1bb

we should have always been handling the empty string case, and if it doesn't have a document_loader, it seems like we're never going to want to treat it as a navStart of interest.

[adamraine]

@adamraine possible to land first? Would feel better landing with green smoke tests.

Ya

[paulirish]

is it worth touching this bit?

lighthouse/lighthouse-core/lib/tracehouse/trace-processor.js

Lines 469 to 470 in d71a587

	const navStartEvt = events.find(e => Boolean(e.name === 'navigationStart' &&
	e.args?.data?.isLoadingMainFrame && e.args.data.documentLoaderURL));

feels a little odd this is so similar but different.

[adamraine]

is it worth touching this bit?

Seems pretty simple to bring in _isNavigationStartOfInterest

[paulirish]

is it worth touching this bit?

Seems pretty simple to bring in _isNavigationStartOfInterest

totally yes. but the impls are different. and i havent been looking at this recently enough to know that they should or should not be different. so was hoping you'd know. :)

[adamraine]

totally yes. but the impls are different. and i havent been looking at this recently enough to know that they should or should not be different. so was hoping you'd know. :)

I think the only concern is if the regex is too strict for just determining the main frame. Unless we are concerned about trace processor working for non http/https/chrome pages, I think it's fine to reuse isNavigationStartOfInterest.

[paulirish]

totally yes. but the impls are different. and i havent been looking at this recently enough to know that they should or should not be different. so was hoping you'd know. :)

I think the only concern is if the regex is too strict for just determining the main frame. Unless we are concerned about trace processor working for non http/https/chrome pages, I think it's fine to reuse isNavigationStartOfInterest.

thanks yeah. when i wrote my last reply I didn't see that you already made the change.
so i was like "why is he saying it'd be fine but without trying it?" :)

anyway.. my bad.

your reasoning for reuse feels good, so i'm happy we got to DRY it up a tad.
thx