Use `frame.name?.startsWith` for stackprof #419

jez · 2023-05-19T01:58:27Z

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.

This stackprof was generated from https://github.com/sorbet/sorbet by running the sorbet-runtime gem's test suite. This profile itself doesn't exhibit the bug where stackprof sometimes generates profiles with a missing name. Unfortunately, I can only get that to happen when using stackprof at work, where the contents of the profile result contain sensitive file, class, and method names that cannot be shared publically. So instead, I've opted to edit this file to forcibly make it exhibit the bug (see the next commit).

As promised, this frame is going to have it's name deleted to forcibly exhibit the bug.

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 jlfwong#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 jlfwong#378.

jez · 2023-05-19T02:00:38Z

Fixes #378

coveralls · 2023-06-04T05:21:57Z

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

jlfwong · 2023-06-04T05:33:43Z

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 · 2023-06-13T21:17:57Z

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 · 2023-06-13T21:24:43Z

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.

jlfwong

Thanks for adding tests and shifting the change closer to the problem.

Two more changes to land this:

The change to profile.ts needs reverting
I try to keep tests as minimal as possible to make them easy to debug in the future. Large profiles make this more challenging. Is it possible to create a reduced profile? If the nature of doing that is tricky because you aren't sure what triggers the undefined name behaviour, then this is okay as-is

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

jlfwong · 2023-06-13T21:33:41Z

src/lib/profile.ts

@@ -406,7 +406,7 @@ export class Profile {
    for (let frame of this.frames) {
      // This function converts a mangled C++ name such as "__ZNK7Support6ColorFeqERKS0_"
      // into a human-readable symbol (in this case "Support::ColorF::==(Support::ColorF&)")
-      if (frame.name.startsWith('__Z')) {
+      if (frame.name?.startsWith('__Z')) {


This still needs removing

jez · 2023-06-13T22:09:40Z

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 · 2023-06-13T22:15:42Z

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 · 2023-06-15T08:31:05Z

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

jlfwong · 2023-06-22T05:57:36Z

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

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 jlfwong#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 jlfwong#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 added 3 commits May 18, 2023 18:53

Delete name from one of the frames

dcb9840

As promised, this frame is going to have it's name deleted to forcibly exhibit the bug.

jez mentioned this pull request May 19, 2023

Web flamegraph sometimes crashes with "startsWith of undefined" (rack-mini-profiler) #378

Open

Properly track frame name in types

d80a76f

jlfwong reviewed Jun 13, 2023

View reviewed changes

jez added 3 commits June 13, 2023 15:10

Also remove ? from profile.ts

3476cf3

Make the trace smaller

feddcca

Show behavior in new test

f5201e3

jlfwong merged commit e9133be into jlfwong:main Jun 15, 2023
8 checks passed

jez deleted the jez-stackprof-null-name branch June 15, 2023 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `frame.name?.startsWith` for stackprof #419

Use `frame.name?.startsWith` for stackprof #419

jez commented May 19, 2023

jez commented May 19, 2023

coveralls commented Jun 4, 2023 •

edited

jlfwong commented Jun 4, 2023 •

edited

jez commented Jun 13, 2023

jez commented Jun 13, 2023

jlfwong left a comment

jlfwong Jun 13, 2023

jez commented Jun 13, 2023

jez commented Jun 13, 2023

jlfwong commented Jun 15, 2023 •

edited

jlfwong commented Jun 22, 2023

Use frame.name?.startsWith for stackprof #419

Use frame.name?.startsWith for stackprof #419

Conversation

jez commented May 19, 2023

jez commented May 19, 2023

coveralls commented Jun 4, 2023 • edited

jlfwong commented Jun 4, 2023 • edited

jez commented Jun 13, 2023

jez commented Jun 13, 2023

jlfwong left a comment

Choose a reason for hiding this comment

jlfwong Jun 13, 2023

Choose a reason for hiding this comment

jez commented Jun 13, 2023

jez commented Jun 13, 2023

jlfwong commented Jun 15, 2023 • edited

jlfwong commented Jun 22, 2023

Use `frame.name?.startsWith` for stackprof #419

Use `frame.name?.startsWith` for stackprof #419

coveralls commented Jun 4, 2023 •

edited

jlfwong commented Jun 4, 2023 •

edited

jlfwong commented Jun 15, 2023 •

edited