Skip to content

Commit 41631b5

Browse files
authored
Fix null marker stack substitution (#5613)
Example with Focus function transform: [Production](https://share.firefox.dev/4mW78M3) | [Deploy preview](https://deploy-preview-5613--perf-html.netlify.app/public/ggqb1y28h0nmwyxaqjdhvrnmfpn58hyeg25cza8/calltree/?globalTrackOrder=q0phkm7in1c9f5g862boela34dj&hiddenGlobalTracks=1wo&hiddenLocalTracksByPid=2465-1w4~92294-0~36702-0~2523-0~36259-0w2~71663-01~71664-0&search=oae&thread=x6&transforms=ff-280&v=11) Example with JS-only view: [Production](https://share.firefox.dev/4poRI4M) | [Deploy preview](https://deploy-preview-5613--perf-html.netlify.app/public/rpnres9s7rdrsfb240wtskvrq5sv7xzn3f4m2p0/calltree/?globalTrackOrder=30w2&hiddenGlobalTracks=01&hiddenLocalTracksByPid=2588-1w3~2591-01&implementation=js&range=9133m2135~9517m70&search=aoeu&tabID=25&thread=f&v=11) Fixes #5612. The more stringent array bounds checks from #5599 revealed a bug that I introduced in #5549: Marker stacks were not being substituted if the `convertStack` lambda returned null. This PR fixes the type and restores the working code from before the TypeScript migration.
2 parents 4c96d7d + 124cf1d commit 41631b5

File tree

5 files changed

+55
-57
lines changed

5 files changed

+55
-57
lines changed

src/components/shared/MarkerContextMenu.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,10 +145,10 @@ class MarkerContextMenuImpl extends PureComponent<Props> {
145145
return !marker || marker.end === null;
146146
}
147147

148-
_convertStackToString(stack: IndexIntoStackTable): string {
148+
_convertStackToString(stack: IndexIntoStackTable | null): string {
149149
const { thread, implementationFilter } = this.props;
150150

151-
if (thread === null) {
151+
if (thread === null || stack === null) {
152152
return '';
153153
}
154154

src/components/tooltip/Marker.tsx

Lines changed: 34 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -423,40 +423,43 @@ class MarkerTooltipContents extends React.PureComponent<Props> {
423423
categories,
424424
} = this.props;
425425
const { data, start } = marker;
426-
if (data && 'cause' in data && data.cause) {
427-
const { cause } = data;
428-
const causeAge = cause.time !== undefined ? start - cause.time : 0;
429-
return [
430-
<TooltipDetailSeparator key="backtrace-separator" />,
431-
<TooltipDetail label="Stack" key="backtrace">
432-
<div className="tooltipDetailsBackTrace">
433-
{
434-
/* The cause's time might be later than the marker's start. For
426+
if (!data || !('cause' in data) || !data.cause) {
427+
return null;
428+
}
429+
const { time, stack } = data.cause;
430+
if (stack === null) {
431+
return null;
432+
}
433+
const causeAge = time !== undefined ? start - time : 0;
434+
return [
435+
<TooltipDetailSeparator key="backtrace-separator" />,
436+
<TooltipDetail label="Stack" key="backtrace">
437+
<div className="tooltipDetailsBackTrace">
438+
{
439+
/* The cause's time might be later than the marker's start. For
435440
example this happens in some usual cases when the cause is
436441
captured right when setting the end marker for tracing pairs of
437442
markers. */
438-
causeAge > 0 ? (
439-
<h2 className="tooltipBackTraceTitle">
440-
{data.type === 'Styles' || marker.name === 'Reflow'
441-
? `First invalidated ${formatTimestamp(
442-
causeAge
443-
)} before the flush, at:`
444-
: `Triggered ${formatTimestamp(causeAge)} ago, at:`}
445-
</h2>
446-
) : null
447-
}
448-
<Backtrace
449-
maxStacks={restrictHeightWidth ? 20 : Infinity}
450-
stackIndex={cause.stack}
451-
thread={thread}
452-
implementationFilter={implementationFilter}
453-
categories={categories}
454-
/>
455-
</div>
456-
</TooltipDetail>,
457-
];
458-
}
459-
return null;
443+
causeAge > 0 ? (
444+
<h2 className="tooltipBackTraceTitle">
445+
{data.type === 'Styles' || marker.name === 'Reflow'
446+
? `First invalidated ${formatTimestamp(
447+
causeAge
448+
)} before the flush, at:`
449+
: `Triggered ${formatTimestamp(causeAge)} ago, at:`}
450+
</h2>
451+
) : null
452+
}
453+
<Backtrace
454+
maxStacks={restrictHeightWidth ? 20 : Infinity}
455+
stackIndex={stack}
456+
thread={thread}
457+
implementationFilter={implementationFilter}
458+
categories={categories}
459+
/>
460+
</div>
461+
</TooltipDetail>,
462+
];
460463
}
461464

462465
_maybeRenderNetworkPhases() {

src/profile-logic/merge-compare.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1356,7 +1356,8 @@ function mergeMarkers(
13561356
if (oldData && 'cause' in oldData && oldData.cause) {
13571357
// The old data has a cause, we need to convert the stack.
13581358
const oldStack = oldData.cause.stack;
1359-
const newStack = translationMapForStacks.get(oldStack);
1359+
const newStack =
1360+
oldStack !== null ? translationMapForStacks.get(oldStack) : null;
13601361
if (newStack === undefined) {
13611362
throw new Error(
13621363
`Missing old stack entry ${oldStack} in the translation map.`

src/profile-logic/profile-data.ts

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2465,17 +2465,14 @@ export function updateThreadStacks(
24652465
oldData: MarkerPayload | null
24662466
): MarkerPayload | null {
24672467
if (oldData && 'cause' in oldData && oldData.cause) {
2468-
const stack = convertStack(oldData.cause.stack);
2469-
if (stack) {
2470-
// Replace the cause with the right stack index.
2471-
return {
2472-
...oldData,
2473-
cause: {
2474-
...oldData.cause,
2475-
stack,
2476-
},
2477-
};
2478-
}
2468+
// Replace the cause with the right stack index.
2469+
return {
2470+
...oldData,
2471+
cause: {
2472+
...oldData.cause,
2473+
stack: convertStack(oldData.cause.stack),
2474+
},
2475+
};
24792476
}
24802477
return oldData;
24812478
}
@@ -2536,17 +2533,14 @@ export function updateRawThreadStacksSeparate(
25362533
oldData: MarkerPayload | null
25372534
): MarkerPayload | null {
25382535
if (oldData && 'cause' in oldData && oldData.cause) {
2539-
const stack = convertSyncBacktraceStack(oldData.cause.stack);
2540-
if (stack) {
2541-
// Replace the cause with the right stack index.
2542-
return {
2543-
...oldData,
2544-
cause: {
2545-
...oldData.cause,
2546-
stack,
2547-
},
2548-
};
2549-
}
2536+
// Replace the cause with the right stack index.
2537+
return {
2538+
...oldData,
2539+
cause: {
2540+
...oldData.cause,
2541+
stack: convertSyncBacktraceStack(oldData.cause.stack),
2542+
},
2543+
};
25502544
}
25512545
return oldData;
25522546
}

src/types/markers.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ export type CauseBacktrace = {
201201
// No upgrader was written for this change.
202202
tid?: Tid;
203203
time?: Milliseconds;
204-
stack: IndexIntoStackTable;
204+
stack: IndexIntoStackTable | null;
205205
};
206206

207207
/**

0 commit comments

Comments
 (0)