Use `frame.name?.startsWith` for stackprof

[jez]

Sometimes, stackprof frames don't get generated with a name in the frame. I think it's probably worth tracking down why that is, but in the mean time, speedscope simply crashes with a method call on undefined. The crash is bad because it only shows up in the console--there's no visible message saying that speedscope failed to parse and load a profile.

For more information, see #378

This fixes the crash by simply skipping the logic in demangle if the name field isn't present on a frame. That's probably a fine tradeoff? Because in this case, stackprof is generating ruby frames, which means that C++ name demangling won't apply.

I have tested this by running the scripts/prepare-test-installation.sh script and verifying that bin/cli.js can now successfully load the included profile. Before these changes, I verified that speedscope failed with the behavior mentioned in #378.

I've also included a snapshot test case, but it seems that the Jest test harness only tests the parsing, not the rendering (correct me if I'm wrong). So I haven't actually been able to create an automated test that would catch a regression. Please let me know if there's a better way to have written this test.

I've staged the commits on this branch so that the second commit (dcb9840) showcases the minimal diff to a stackprof file that reproduces the bug. That is, rather than look at the thousands of new lines in the stackprof profile, you can view the second commit to see the salient part of the file.

[jez]

Fixes #378

[coveralls]

coverage: 42.072% (+0.2%) from 41.844% when pulling f5201e3597ad10f6319e5aafbd4e5ee7302028e5 on jez:jez-stackprof-null-name into 81a6f29ad1eb632683429084bd6c5f497667fb5e on jlfwong:main.

[jlfwong]

Hi @jez!

Thanks for working on fixing this, and including tests no less.

I'd prefer to fix this early in the import process, because the name? is incongruent with the type signature. Whenever I see defensive programming despite the type signature indicating that the scenario should be impossible, I find myself worried. If there was anywhere else that assumed that name was non-null, we'd be playing whack-a-mole finding all the places that rely on it being non-null when it can be null. I'd rather make just make sure it's actually non-null.

It seems to me like the correct fix is to change https://github.com/jlfwong/speedscope/blob/main/src/import/stackprof.ts to ensure that the frame names are present.

This would mean updating importFromStackprof to do something like this:

for (let frame of stackprofProfile.frames) {
  if (!('name' in frame) || frame.name == null) {
    frame.name = '(unknown)'
  }
}

I also wonder if this is a bug in stackprof itself that should be fixed upstream

[jez]

Thanks for the review, @jlfwong!

I picked a slightly different implementation because I was getting a TypeScript error I was not familiar with. (Something about not being able to use for...of with a type declared like {[...]: ...}. I think it wanted me to write a type for the Symbol.iterator somehow?) In any case, I think this implementation is equivalent.

[jez]

I also wonder if this is a bug in stackprof itself that should be fixed upstream

It probably is! For me it's easier to work around it here in speedscope, because we already have tons and tons of recorded stackprof profiles in our production profiler that I don't really want to have to try to fix—merely upgrading stackprof would only fix the new profiles, and not let me visualize old, already-captured profiles.

[jez]

Also FWIW, if you do have a reproduction case for stackprof, would encourage you to report it as a bug on stackprof to prevent others from running into confusing behavior in the future

I did try to reproduce it, but was unable to. I will continue to try to reproduce it, and will report upstream.

[jez]

I've updated the test to make it smaller. I just copied the smallest stackprof JSON test and deleted the "name": ... property from one of the frames in that file to simulate what happens when the Stackprof generates frames without a name. You can see just the changes to the test in the last commit.

[jlfwong]

Thanks for the follow through! This will go out with the next release

[jlfwong]

This is now live on https://speedscope.app and published to npm as part of v1.15.2. Thanks for your contribution!