search

replayio / node

Integration of the node.js runtime with the record/replay driver
https://nodejs.org/
Other
20 stars 3 forks source link

Node sourcemaps are not being applied #83

Closed hbenl closed 2 years ago

hbenl commented 2 years ago

Version

No response

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

Create a recording with node of a script with sourcemaps and open the recording in Replay.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

Sourcemaps are not being applied.

Additional information

The source objects we get from basic processing in node recordings don't contain a sourceMapURL, but the sourcemaps are uploaded with their sourceMapURL, so the backend thinks they don't match. We could:

  1. ensure that the source objects from basic processing do contain their sourceMapURL
  2. write the sourcemaps in node recordings without their sourceMapURL
  3. ignore the sourceMapURL when matching sourcemaps in the backend if it is undefined in the source object from basic processing

The first option seems to be the most desirable, but I don't know how much effort that would be. Alternatively the third option would be easy to do and should be safe if we require the source's contentHash to be available and match in that case.

hbenl commented 2 years ago

@bhackett1024 which option do you prefer?

hbenl commented 2 years ago

https://app.replay.io/recording/4106355d-feaf-4ec7-a002-9b5feecb1f87 is a node recording with a sourcemap. I've manually removed the target_map_url_hash in the database for that sourcemap and now it is being applied.

gideonred commented 2 years ago

Putting in Todo this sprint for Holger to have a look.

bhackett1024 commented 2 years ago

The backend shouldn't be doing stuff to special case node recordings. It seems like for the first solution we'd just have to implement Target.getSourceMapURL properly here: https://github.com/replayio/node/blob/7a9f91f51e3c706c3676a8e017958f9e82c5489d/lib/internal/recordreplay/main.js#L160

If that's not necessary for actually getting things to work then it does seem like relaxing the backend matching like you suggest would be better, we want the available source maps to apply as often as possible. TBH though I don't have a lot of familiarity with our source map matching logic and @loganfsmyth is more knowledgeable.

hbenl commented 2 years ago

I tried to implement this similar to how it works in gecko-dev: save the sourceMapURLs in a Map and read from that Map in Target_getSourceMapURL(). However, this doesn't work, I guess that the Debugger.scriptParsed event (from which we get the sourceMapURL) is sent after the Target.getSourceMapURL command is sent from recordreplay::OnNewSource(). And Target_getSourceMapURL() is synchronous, so we can't wait for the sourceMapURL to show up. @loganfsmyth What do you think about option 3? If a source's url and content match I think it's safe to apply the sourcemap.

hbenl commented 2 years ago

or I could move the RecordReplayRegisterScript() call to the end of this function: https://github.com/replayio/node/blob/7a9f91f51e3c706c3676a8e017958f9e82c5489d/deps/v8/src/debug/debug.cc#L2253-L2286

@bhackett1024 That seems to work, but would it break anything?

bhackett1024 commented 2 years ago

@bhackett1024 That seems to work, but would it break anything?

I don't think this would break anything, this sounds like a good solution. Eventually it would be best to consolidate the scriptParsed and RegisterScript logic and if we're doing them in similar places that would be a good step forwards and makes more sense anyways.