Skip to content

Pass profile upgrade info to stateFromLocation in compare view#5890

Merged
mstange merged 4 commits intofirefox-devtools:mainfrom
ojuschugh1:fix-compare-url-upgraders
Mar 24, 2026
Merged

Pass profile upgrade info to stateFromLocation in compare view#5890
mstange merged 4 commits intofirefox-devtools:mainfrom
ojuschugh1:fix-compare-url-upgraders

Conversation

@ojuschugh1
Copy link
Copy Markdown
Contributor

This PR fixes #5871

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 9, 2026

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.42%. Comparing base (f8bed2b) to head (cb9c976).
⚠️ Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
src/actions/receive-profile.ts 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5890      +/-   ##
==========================================
- Coverage   85.45%   85.42%   -0.04%     
==========================================
  Files         321      321              
  Lines       32081    32168      +87     
  Branches     8842     8785      -57     
==========================================
+ Hits        27416    27479      +63     
- Misses       4233     4253      +20     
- Partials      432      436       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@mstange mstange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! So you're calling stateFromLocation twice now. I think it would be better to make it clear that the first call is only aiming to get the "data source" information about the URL. If you look at the code which handles non-compare URLs, it's doing manual splitting of the URL to get those pieces of information. Can you move the data source determination code into a shared place and use it from both call sites, so that stateFromLocation is only called once we have the profile upgrade info?

@ojuschugh1 ojuschugh1 force-pushed the fix-compare-url-upgraders branch 3 times, most recently from 4426394 to 865321e Compare March 15, 2026 10:04
@ojuschugh1 ojuschugh1 force-pushed the fix-compare-url-upgraders branch from 865321e to 11b80cc Compare March 15, 2026 15:40
Copy link
Copy Markdown
Contributor

@mstange mstange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@mstange
Copy link
Copy Markdown
Contributor

mstange commented Mar 18, 2026

I was trying to check whether this patch actually fixes the issue I reported originally but I'm getting an error. The error may be caused by a separate (pre-existing) bug.

Here's a pre-upgrade URL which has a "Focus on function" transform for WorkerGlobalScope.importScripts applied: https://deploy-preview-5739--perf-html.netlify.app/public/bd32mzpdcrj7ms80657qq8h7t75erwadr1yszcg/calltree/?globalTrackOrder=a0968431572&hiddenGlobalTracks=1w35w8&hiddenLocalTracksByPid=24844-0w3~44724-0~38200-0~26856-0~26332-023~30176-0~42128-0~34888-0~51112-0&thread=i&transforms=ff-57&v=12

If I go to https://profiler.firefox.com/compare/ and paste the above URL in both text boxes, click "Retrieve profiles", and then switch to one of the two identical "DOM Worker" threads, the "Focus on function" transform is wrong; it's focusing on mozilla::EventDispatcher::DispatchDOMEvent instead of on WorkerGlobalScope.importScripts. So far so good, this is what we expected due to this bug.

However, if I go to https://deploy-preview-5890--perf-html.netlify.app/compare/ (the deploy preview for this PR) and follow the same steps, I get an error: TypeError: can't access property "name", l[de] is undefined.

@ojuschugh1
Copy link
Copy Markdown
Contributor Author

Thanks for testing! I looked into this and I think it's a pre-existing bug, not caused by this PR.

The profile at that hash was stored on deploy-preview-5739 (Dec 2025), which is already at processed profile version 60. So upgradeInfo.v60 is undefined after deserialization and the v15 URL upgrader skips entirely. The ff-57 transform gets parsed as funcIndex 57 in the shared funcTable — same behavior as production without this PR.

The TypeError: can't access property "name", l[de] is undefined is likely from the per-thread funcIndex 57 (from the v=12 URL) being used directly as a shared funcTable index. On production this silently focuses on the wrong function, but the deploy preview merge with main (#5878) may have changed the code path enough to turn that silent wrong behavior into a crash.

I also added a bounds check in mergeProfilesForDiffing (latest commit) so out-of-bounds thread indexes give a clear error instead of a cryptic TypeError.

Should I file a separate issue for the pre-existing per-thread funcIndex bug?

@ojuschugh1 ojuschugh1 requested a review from mstange March 22, 2026 14:33
@mstange
Copy link
Copy Markdown
Contributor

mstange commented Mar 22, 2026

The profile at that hash was stored on deploy-preview-5739 (Dec 2025), which is already at processed profile version 60.

No it's not - the profile it's downloading from https://storage.googleapis.com/profile-store/bd32mzpdcrj7ms80657qq8h7t75erwadr1yszcg has meta.preprocessedProfileVersion == 55, and deploy-preview-5739 upgrades the in-memory representation of it to version 58 when it loads it.

@mstange
Copy link
Copy Markdown
Contributor

mstange commented Mar 24, 2026

After rebasing on top of #5906 I no longer see the error. Merging, thank you!

@mstange mstange merged commit c62a2eb into firefox-devtools:main Mar 24, 2026
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When loading comparison URLs, we're not supplying the individual URL upgraders with the profile info

2 participants