chore(ui-mode): properly communicate file lookups over trace server connection

[agg23]

#33542 was intended to decouple the UI Mode interface from the underlying webserver, allowing it to connect to the trace server and webserver separately. The core functionality, file retrieval, appears to not have functioned as expected.

• The functionality watched for service worker connections on /file/ (trailing slash), but these routes do not have any further content in the URL path, but have it all in a query string. So the route to hit is actually /file (no trailing slash), so this code never ran.
• Even if you do have the code run, it relies on a trace server connection to be active. The code as written only establishes a trace server connection object once you load a trace for the first time. This means if you load the UI and don't start running a test, you cannot retrieve any files.

This previously worked because the route we catch on the service worker (/trace/file) also happens to be a valid route on our webserver/trace server, with the same usage semantics. These requests were bypassing the service worker and going directly to the service.

---

This change makes all of the requests go through the service worker as expected. This has the side effect of fixing file previous in HMR mode.

[Skn0tt]

Good catch! The change looks fine to me.

About this:

This means if you load the UI and don't start running a test, you cannot retrieve any files.

This is about the "Source" tab not showing contents before test run, right? Did you discover this through manual testing, or do we have a test case for it?

[agg23]

This is about the "Source" tab not showing contents before test run, right? Did you discover this through manual testing, or do we have a test case for it?

This was through manual testing, as the problem only appeared once I fixed the original SW routing bug. I imagine a test would have caught it had I pushed only that change, but given I have not run the test suite against only that change and have not seen such a test, I cannot be certain.

[Skn0tt]

I see! Given we'd have shipped a bug if you didn't perform thorough manual testing, I think a adding a test for this would be nice. I wonder whether our testing agents can do a good job on this :D

[Skn0tt]

don't think the test is blocking though, I can also try generating one in the coming days

[agg23]

As expected, we do have test coverage here.

[github-actions]

Test results for "tests 1"

1 failed
❌ [playwright-test] › test-serial.spec.ts:326 › test.describe.serial should work inside test.describe.parallel @macos-latest-node18-1

3 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`
⚠️ [chromium-library] › library/proxy.spec.ts:93 › should proxy local network requests › with other bypasses › link-local `@ubuntu-22.04-chromium-tip-of-tree`

46901 passed, 811 skipped

---

Merge workflow run.

[dgozman]

I am a bit lost of what's going on. It seems to me that the purpose of #33542 and this PR is to support HMR? If so, I now feel that's a big price to pay for HMR to work for all our features. Perhaps we should undo file/ support with HMR entirely and simplify our code a lot? I am not comfortable shipping this complexity to our users. WDYT?

[agg23]

I personally think HMR is very valuable, but I was somewhat surprised to see the changes made to support it. I don't think it is a bad idea to decouple the web and trace servers, though with only a single supported usecase that isn't even user facing (HMR) I'm not sure of its worth. I don't think there's much complexity in this code in particular, and Pavel is currently redoing parts of it anyway (for which I'm holding this PR).