fix(trace): Reload traces immediately on service worker start

[cpAdm]

This is a follow up from PR #37442, it fixes my PR comments mentioned there:

With the current restart mechanism, the traces would not reload since the /ping calls would come in first and clear (via the gc) the IDB stored clientIdToTraceUrls (since clientIdToTraceUrls is empy map at that point).
This PR introduces new way: it simply loads all the traces from IDB whenever the SW is restarted. When handling the fetches it will have to wait for this loading to complete, else /ping or /contexts might alter the IDB/local clientIdToTraceUrls.

Tip for testing: You can unregister and stop (which basically simulates a restart) service workers at chrome://serviceworker-internals/?devtools.

fixes: #37421

[github-actions]

Test results for "tests 1"

2 failed
❌ [playwright-test] › ui-mode-test-network-tab.spec.ts:204 › should not preserve selection across test runs @macos-latest-node18-1
❌ [playwright-test] › reporter-html.spec.ts:2999 › merged › execSync doesnt produce a second stdout attachment @macos-latest-node18-2

2 flaky

⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/page-emulate-media.spec.ts:144 › should keep reduced motion and color emulation after reload `@firefox-ubuntu-22.04-node18`

46922 passed, 821 skipped

---

Merge workflow run.

[pavelfeldman]

I don't think we want all the traces to be loaded for inactive clients. I think we want to move the if (!clientIdToTraceUrls.has(event.clientId)) { block of the original code up in doFetch and omit it for /context.

[cpAdm]

Makes sense. Although that would mean that if the first request after a reload is a /trace/snapshot call, the trace is not reloaded (since this is a different client worker (event.clientId) then the one that called /contexts) and we still return 404. Right?

[cpAdm]

Also we have to think about scenarios like:

• If one client re-connects, its traces are loaded again. But what if it then calls /ping, it will gc and remove traces (params in IndexDB) for all other clients.

Sidenote: How important is the gc? Isn't the browser already taking care of this by shutting down the SW? (we can always do the await self.clients.matchAll(); check whenever the SW restarts)

[pavelfeldman]

The GC is so that you could open 100 traces one after another in the same window (UI Mode) without exploding out of memory.

[pavelfeldman]

Makes sense. Although that would mean that if the first request after a reload is a /trace/snapshot call, the trace is not reloaded (since this is a different client worker (event.clientId) then the one that called /contexts) and we still return 404. Right?

Why is it a different client worker? I assume event.clientId are preserved, so should return the right value.

Also we have to think about scenarios like: If one client re-connects, its traces are loaded again. But what if it then calls /ping, it will gc and remove traces (params in IndexDB) for all other clients.

Why does gc remove traces for all other clients? I would assume that once the sw is restarted, self.clients.matchAll() would return them and no gc will happen?

[cpAdm]

Why is it a different client worker? I assume event.clientId are preserved, so should return the right value.

I should have been more clear here. What I meant is that each tracer viewer has two clients: one for the trace viewer itself (which also loads the trace) and one for the iframe (which fetches /snapshot). They have different event.clientIds (but indeed on a restart their Ids remain the same).

But I actually don't think it is currently possible for the /snapshot call to arrive first after a SW restart (its clientId is not linked to a saved trace). Since when selecting an action in the trace viewer, always first a /snapshotInfo call will be made (which reloads the trace). Then after that the /snapshot call is made, but at that time the trace has thus already been reloaded.

[pavelfeldman]

Great find!!!

[pavelfeldman]

I uploaded #37656 with the fix. If you think there are different use cases we need to address, please feel free to add a test and it'll be clear what is being fixed!

[cpAdm]

@pavelfeldman That indeed fixes some of the use cases,

I am currently trying different scenarios:

I have the following scenario which fails (was not sure how to put this into an actual test):

Given you open up 2 tabs with trace viewer in the same browser
And you load two different traces in each viewer
And you now simulate a SW restart (e.g. stop the service worker via chrome://serviceworker-internals/)
And you go back to one tab and wait for 10s (to make sure the first /ping comes in)
When you now hover over an action in one of these trace viewers
Then this only works in one of the trace viewers

Underlaying cause: The /ping call will call gc() which currently always saves the clientIdToTraceUrls to IndexDB. In this particular case it has only reloaded one client (the first /ping). So when the other client fetches /snapshotInfo (when hovering over an action), its client data is no longer in IndexDB to restore.