Broken V8 source map cache file URL parsing on Windows

[clemyan]

Somewhere along the node v15 line, V8 coverage data output by node changed format on Windows. Specifically, file URLs within the source-map-cache changed from

file://C:\some\path\main.js

to

file:///C:/some/path/main.js

(Note the extra forward slash in front)

I haven't bisected further but v15.0.0 is using backslashes and v15.14.0 is using forward slashes.

---

This causes _resolveSource to return incorrect file path on Windows (e.g. /C:/some/path/main.js), in turn causing load to reject when trying to read the wrong path from fs.

---

The easy solution would be to check process.platform and strip any leading forward slash, but I was thinking whether there is a more robust solution. A few ideas/notes:

1. Since v8-to-istanbul supports down to node 10.10.0, we cannot rely on fileURLToPath which was added in 10.12.0. Or we can just copy node's implementation?
2. Regarding the webpack:// protocol: looks like URLs inside webpack sourcemaps are fully configurable? But the default format look pretty easily parsable using new URL
3. With that in mind, should we just parse everything using new URL? Seem doable if we ignore UNC paths (which I don't think currently works anyway?)

The biggest challenge currently is the tests doesn't want to run at all on Windows machines (Well, on my Windows machine, but running the tests inside a Linux docker container on the same machine works so I am pretty sure it is a Windows thing), so I can't easily experiment.

[bcoe]

@clemyan I like your suggestion of trying to use fileURLToPath. How might we have caught this issue with our windows tests? I'm wondering why this wasn't caught by our windows tests in c8? is there an additional test we should add?

[clemyan]

Well the bug only started happening since some where in the v15 line of node (when the file URL format changed), and c8's CI tests only test up to v14.

I saw you made some changes in node 15.11.0 regarding source maps, so I have tested against 15.10.0, but it is still affected. In other words:

• node versions 15.0.0 and before are not affected by this bug
• node versions 15.10.0 and after are affected

---

Regarding fileURLToPath, that's what I have used to verify my PR to c8 didn't break anything. But dropping node 10.10 support would be a breaking change? (Or are you planning to do this anyway for node 10 EOL?)

---

I am seeing a number of issues over the years both here and in c8 regarding source maps. Now that I have thought more about this, I am convinced there is a bigger root problem that cannot be fixed without a major refactor. Two major issues I see are:

• The relation between original source and source maps is actually many:many. (Imagine 3 source files are bundled and then code split into 2 generated code files). Currently, v8-to-istanbul work on a per-source-file basis, and only accept one source map per original source.
• Resolution of source paths should be done using URL resolution semantics, and relative to the URL of the source map in accordance with the spec. But we don't have the URL of the source map. I haven't thoroughly tested this but I don't think we can handle something like dist/main.js using to dist/maps/main.js.map because sources should be resolved relative to dist/maps/ rather than dist/.

If you are interested, I can open a separate mega-issue and see if we can work out a solution.

[bcoe]

The relation between original source and source maps is actually many:many. (Imagine 3 source files are bundled and then code split into 2 generated code files). Currently, v8-to-istanbul work on a per-source-file basis, and only accept one source map per original source.

Today we support a source map that may map to multiple source files, but you're saying that there may be cases where one source file has multiple source maps?

Resolution of source paths should be done using URL resolution semantics, and relative to the URL of the source map in accordance with the spec. But we don't have the URL of the source map. I haven't thoroughly tested this but I don't think we can handle something like dist/main.js using to dist/maps/main.js.map because sources should be resolved relative to dist/maps/ rather than dist/.

I agree that we should make any changes necessary to get us inline with the spec.

Regarding fileURLToPath, that's what I have used to verify my PR to c8 didn't break anything. But dropping node 10.10 support would be a breaking change? (Or are you planning to do this anyway for node 10 EOL?)

Let's bump to the engines field to 10.12.0, but in a breaking change.

I'm not quite ready to drop Node 10 support in c8, because we use it heavily at my work, and we're likely going to be lagging behind dropping Node 10 by a few months.

---

If you are interested, I can open a separate mega-issue and see if we can work out a solution.

I would certainly be interested in any additional fixes you are able to submit 👍 I would love to break the work into small chunks if possible.

[clemyan]

Today we support a source map that may map to multiple source files, but you're saying that there may be cases where one source file has multiple source maps?

Yes. If you need a concrete case:

• I have two entrypoints, a.js and b.js, both import from common.js
• I bundle the two entrypoints separately, producing a.min.js(.map) and b.min.js(.map)
• I run a test suite that covers both a.min.js and b.min.js

In this case, both a.min.js.map and b.min.js.map have two sources, one of which is common.js. So, to correctly report coverage for common.js, we need to pool together the scriptCovs of a.min.js and b.min.js.

Crude idea:

• Collect all relevant scriptCovs of generated code
• Use relevant source maps (either from source-map-cache or from fs) to map to equivalent scriptCovs of original source files
• Accumulate scriptCovs for both the generated code and source code (subject to include/exclude filtering, of course)
• Finally, for all relevant files (both generated and original source), merge accumulated scriptCovs and covert

I will try to whip up a PoC over the week.

---

I would certainly be interested in any additional fixes you are able to submit 👍 I would love to break the work into small chunks if possible.

I will see what I can do.

---

And I forgot to mention this before. So I said tests don't run on Windows, but I was wrong. I messed up when swapping around node versions to test this issue. Turns out I was just running into tapjs/tapjs#746. So that wasn't a Windows/Linux thing.

In terms of results, looks like the test are failing due to Windows-related issues: (Node 14.16.1)

  🌈 SUMMARY RESULTS 🌈


 FAIL  test/v8-to-istanbul.js 3 failed of 19 316.632ms
expected Array [   'C:\\<redacted>\\v8-to-istanbul\\test\\fixtures\\scripts\\src\\index.ts',   'C:\\<redacted>\\v8-to-istanbul\\test\\fixtures\\scripts\\src\\utils.ts' ] to equal Array [ '/src/index.ts', '/src/utils.ts' ] (at '0', A has 'C:\\<redacted>\\v8-to-istanbul\\test\\fixtures\\scripts\\src\\index.ts' and B ha s '/src/index.ts')
 ✖ The expression evaluated to a falsy value:    assert(v8ToIstanbul.path.includes('v8-to-istanbul/test/fixtures/one-up/relative-source-root.js'))
expected Array [   'C:\\<redacted>\\v8-to-istanbul\\test\\fixtures\\scripts\\webpack\\bootstrap',   'C:\\<redacted>\\v8-to-istanbul\\test\\fixtures\\scripts\\src\\index.ts',   'C:\\<redacted>\\v8-to-istanbul\\test\\fixtures\\scripts\\src\\utils.ts' ] to equal Array [ '/webpack/bootstrap', '/src/index.ts', '/src/utils .ts' ] (at '0', A has 'C:\\<redacted>\\v8-to-istanbul\\test\\fixtures\\scripts\\webpack\\bootstrap' and B has '/webpack/bootstrap')

[bcoe]

@clemyan is the initial issue addressed now, with the patch we landed on c8? I'm wondering if we can close this issue, and open issues in the future for any additional work you'd like to take on.

[clemyan]

@bcoe For my use case, yes, since I run my test suite via ts-node directly. But the c8 test cases for precompiled TypeScript and UglifyJS are still failing (on Windows Node 16) due to this issue.

Since you are open to bumping up to 10.12.0, I want to just do a fix with url.fileURLToPath. Will open PR after I double-check by running the c8 test cases again.

I'll open a separate issue for the other stuff.