core(trace-processor): refactor processEvents and frameEvents

[paulirish]

As discovered in #14264 we were severely excluding events in our processEvents array. And also frameEvents, frameTreeEvents, and mainThreadEvents.

The primary changes here are in trace-processor and this pr contains mostly bug fixes. I envision a second PR which refactors the traceprocessor functions and flow a bit (and wouldn't adjust test results). I can do that here, too, but i'll leave that to the reviewer to opt-in to.

Observations & changes:

• about:blank => website can undergo a process switch. the twitter.com repro makes this super obvious.
• findMainFrameIds() returns the "starting" pid, but that will not be the pid of the primary navigation necessarily. that needs to be found afterwards.
• frameIds are consistent across x-process navigations. (always have been, AFAIK)
• the FrameCommittedInBrowser event is great and spells out the potentially-new processId of the renderer in the new navigation.
• I tried to support the usecase of 'multiple navigations in one trace' (eg in timespans). I'm not convinced we support that well right now.
• Trace events explicitly associated with a frame are generally tagged with either args.data.frame or args.frame. No consistency. Where I saw our code depending on one location, I expanded it to both.
• Removed trace minification

Followup work:

edit: filed trace processor - cleanup and fixes ☂️ · #14708

[paulirish]

diff-wise: the two FrameCommittedInBrowser items here are the new rows.
I added the frame identifier to spell out the tree.

[paulirish]

Turns out startedAfterNavstartTrace (which is already required in this test) is the same asset. I liked that name better.

[brendankenny]

Really nice!

• do we have a test trace that switches processes? I assume no but it would be good to have one
• one thing that gets us still is if somehow a pid gets re-used (however unlikely that is while tracing). Having just a map of pid->tid says nothing about the timing. Seems like we might need a temporal aspect to the tracking as well? Or just step though the trace, subsetting in chunks between any FrameCommittedInBrowser events. That might be what you mean by "determine the "inspected" pids/frames in one pass before doing all the subsetting", though?

[brendankenny]

Would there be an easy way to make sure we don't fall back to this in new traces? If somehow the thread_name event changed format it would be good if the change wasn't masked by the fallback still working.

On the other hand we don't want to throw for users if the fallback works fine still, so probably not a good idea to throw directly, but would there be a way to alert to changes to the event in our ToT tests?

[connorjclark]

Can you extract the followup work to a new issue?

[adamraine]

LGTM

[paulirish]

Still outstanding review comments:

• first item moved to trace processor - cleanup and fixes ☂️  #14708
• second item:

  • Would there be an easy way to make sure we don't fall back to this in new traces? If somehow the thread_name event changed format it would be good if the change wasn't masked by the fallback still working.

  • just sorted this with 6cbf6cc I restored the key trace event to our fixture traces that had stripped it out. There's no more fallback, but that's fine as it wasn't necessary.
• third item (frameEvents is wrong) moved to trace processor - cleanup and fixes ☂️  #14708

That resolves the only remaining open threads on this PR.

[brendankenny]

LGTM!