profiler icon indicating copy to clipboard operation
profiler copied to clipboard

Move stackIndex existence check at if condition in SampleTooltipContents

Open theoniko opened this issue 11 months ago • 3 comments

Solves https://github.com/firefox-devtools/profiler/issues/5290

theoniko avatar Feb 06 '25 09:02 theoniko

I'm not sure about this patch. If sampleIndex can be an invalid index, it means there's a bug somewhere else.

mstange avatar Feb 06 '25 20:02 mstange

I'm not sure about this patch. If sampleIndex can be an invalid index, it means there's a bug somewhere else.

yeah, null could happen when it's filtered out (this is what @canova hits with this STR), but undefined as in the original bug, i'm not so sure. Even null IMO shouldn't happen because we should not show the tooltip at all and therefore not render the component in the first place... therefore the condition might need to be in the SampleGraph itself for Nazim's case.

julienw avatar Feb 07 '25 09:02 julienw

Yeah, for the SampleGraph case, we were passing the filteredThread initially directly because they get removed once you drop samples etc. That time it made sense to just get the filtered thread and use that since we don't need the unfiltered one / range filtered one.

There is this case now:

  1. You hover over the sample in the sample graph and show the tooltip.
  2. You don't move the mouse so the hovered sample doesn't get updated.
  3. You drop the sample.

So in this case we still have the same hoveredSample in the sample graph. And we only check for the sample itself being null or not in the SampleGraph case. We might want to pass only the rangeFilteredThread to the SampleTooltipContents, but i don't think it's a great solution either as we will still continue to see the removed sample tooltip when we drop samples. So SampleGraph solution might be to check the stack itself and add the tooltip only if it's not null during its render.

Even null IMO shouldn't happen because we should not show the tooltip at all and therefore not render the component in the first place...

We actually show the tooltip in case the sample is not there, but this case is slightly different. For example when you profile without the stack sampling, but you still capture the CPU usage, we will not sample; but every sample will only contain the CPU usage values. You can try this case locally by checking the No Periodic Sampling option.


For the undefined case, it's really weird and probably coming from somewhere else. Not so sure where it comes from.

canova avatar Feb 07 '25 10:02 canova