Source view for marker stacks #5633
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5633 +/- ##
==========================================
- Coverage 85.75% 85.64% -0.12%
==========================================
Files 311 311
Lines 30703 30784 +81
Branches 8443 8471 +28
==========================================
+ Hits 26329 26364 +35
- Misses 3950 3996 +46
Partials 424 424 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/components/shared/Backtrace.tsx
Outdated
|
|
||
| // Build a mapping from filtered frame index to stack index | ||
| const { stackTable } = thread; | ||
| const stackIndices: IndexIntoStackTable[] = []; |
There was a problem hiding this comment.
We've mostly aligned on using "indexes" instead of "indices" for the plural of index in this codebase. (Edit: the other review comments will make this comment obsolete because I'm also asking you to remove this variable)
src/components/shared/Backtrace.tsx
Outdated
| </li> | ||
| ) | ||
| ({ funcName, origin, isFrameLabel, category, inlineDepth }, i) => { | ||
| return ( |
There was a problem hiding this comment.
The addition of the return is unnecessary.
src/components/shared/Backtrace.tsx
Outdated
| thread | ||
| ); | ||
|
|
||
| // Build a mapping from filtered frame index to stack index |
There was a problem hiding this comment.
Compute this in getBacktraceItemsForStack instead; you can give BacktraceItem a new stackIndex property. And maybe rename funcNamesAndOrigins to backtraceItems.
src/components/shared/Backtrace.tsx
Outdated
| // Create event handlers for each stack frame to avoid arrow functions in JSX | ||
| const stackFrameHandlers = onStackFrameClick | ||
| ? stackIndices.map((stackIdx) => () => onStackFrameClick(stackIdx)) | ||
| : []; |
There was a problem hiding this comment.
Instead of creating a separate function per backtrace item, let's just create a single function. It should get the stackIndex from the event target. You can use a data-stackIndex attribute to store it on the element.
src/components/shared/Backtrace.tsx
Outdated
| onDoubleClick={ | ||
| onStackFrameClick ? stackFrameHandlers[i] : undefined | ||
| } |
There was a problem hiding this comment.
Given the hover effect and the hand cursor, I think we should do this on regular click, not on double click.
| // The props have changed but not the selectedItem. Check if it's still valid | ||
| // by attempting to get its info. If it throws or returns null, the item is | ||
| // no longer valid (e.g., filtered out). | ||
| try { |
There was a problem hiding this comment.
I'm not loving this try. Do you know in what cases the call to getHoveredItemInfo would throw?
src/components/app/BottomBox.tsx
Outdated
| // When we have a specific line number to scroll to (e.g., from a crash marker) | ||
| // but no timing data, create synthetic timings to highlight that line. |
There was a problem hiding this comment.
Can't we just forward the scrollToLine along as another prop for SourceView, e.g. highlightedLine? Fake sample counts aren't very useful.
| const lib = profile.libs[nativeSymbol.libIndex]; | ||
| if (!lib) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If a numeric indexing operation returns undefined it means there's a bug and we used a bad index. A check like this hides a bug; if we actually hit this return then we should find out why. Don't add this check.
| if (!isAssemblyViewAvailable) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I disagree with this change; if we add more buttons I don't want them to shift position depending on context. Is the disabled state not disabled enough?
src/selectors/code.tsx
Outdated
| return null; | ||
| } | ||
| return profile.libs[nativeSymbol.libIndex]; | ||
| return profile.libs[nativeSymbol.libIndex] ?? null; |
There was a problem hiding this comment.
Wait, does this mean I missed something in the crash marker stack review and we forgot to populate profile.libs? Profiles with bad lib indexes in the nativeSymbols table are invalid.
… marker table sidebar.
…ther than to the hotspot.
… marker chart tooltip.
…ated without hiding the selected marker (symbolication, showing the source code for a frame).
…formation in the profile.
08ad5ed to
9544c00
Compare
This would go nicely with the work I'm doing in https://bugzilla.mozilla.org/show_bug.cgi?id=1993214 to include crash stacks in test resource usage profiles.
deploy preview