From a295cc1735971c74944a4fe389cd766bcf71c4f2 Mon Sep 17 00:00:00 2001 From: fatadel Date: Wed, 6 May 2026 18:08:08 +0200 Subject: [PATCH 1/2] Translate URL track-index state through profile sanitization Previously, hidden non-thread tracks (screenshots, network, IPC) reappeared after publishing a sanitized profile when sanitization also removed a thread: the URL state's hidden-track sets and track-order were reset whenever threads were re-indexed, even though the non-thread tracks still existed in the sanitized profile. attemptToPublish now builds an old to new TrackIndex map by matching tracks on stable identity (pid, screenshot id, threadIndex, counterIndex, visual-progress singleton) and translates hiddenGlobalTracks, globalTrackOrder, hiddenLocalTracksByPid, and localTrackOrderByPid through it on SANITIZED_PROFILE_PUBLISHED. Marker tracks key on a string-table index that sanitization reshuffles, so they are not matched. Fixes #2947 --- src/actions/publish.ts | 171 +++++++++++++++- src/profile-logic/tracks.ts | 144 ++++++++++++++ src/reducers/url-state.ts | 43 +++-- src/test/store/publish.test.ts | 80 +++++++- src/test/unit/tracks-sanitization.test.ts | 225 ++++++++++++++++++++++ src/types/actions.ts | 8 + 6 files changed, 652 insertions(+), 19 deletions(-) create mode 100644 src/test/unit/tracks-sanitization.test.ts diff --git a/src/actions/publish.ts b/src/actions/publish.ts index c20027b649..d481c5af21 100644 --- a/src/actions/publish.ts +++ b/src/actions/publish.ts @@ -17,17 +17,32 @@ import { getDataSource, getProfileNameForStorage, getUrlPredictor, + getGlobalTrackOrder, + getHiddenGlobalTracks, + getHiddenLocalTracksByPid, + getLocalTrackOrderByPid, + getTabFilter, } from 'firefox-profiler/selectors/url-state'; import { getProfile, getZeroAt, getCommittedRange, + getGlobalTracks, + getLocalTracksByPid, + getTabToThreadIndexesMap, } from 'firefox-profiler/selectors/profile'; import { viewProfile } from './receive-profile'; import { ensureExists } from 'firefox-profiler/utils/types'; import { extractProfileTokenFromJwt } from 'firefox-profiler/utils/jwt'; import { withHistoryReplaceStateSync } from 'firefox-profiler/app-logic/url-handling'; import { persistUploadedProfileInformationToDb } from 'firefox-profiler/app-logic/uploaded-profiles-db'; +import { + computeGlobalTracks, + computeLocalTracksByPid, + computeOldTrackIndexToNewTrackIndexMap, + computeHiddenTracksAfterSanitization, + computeTrackOrderAfterSanitization, +} from 'firefox-profiler/profile-logic/tracks'; import type { Action, @@ -36,6 +51,11 @@ import type { StartEndRange, State, Profile, + Pid, + RawCounter, + CounterIndex, + ThreadIndex, + TrackIndex, ProfileIndexTranslationMaps, } from 'firefox-profiler/types'; import { compress } from 'firefox-profiler/utils/gz'; @@ -232,6 +252,47 @@ async function persistJustUploadedProfileInformationToDb( } } +/** + * Map each old CounterIndex to its new CounterIndex across a sanitization + * step. Sanitization removes counters whose parent thread is removed; the + * survivors keep their relative order. Each counter is identified by + * (pid, category, name, mainThreadIndex), with the old-side mainThreadIndex + * normalized through `oldThreadIndexToNew`. + */ +function _computeOldCounterIndexToNew( + oldCounters: RawCounter[] | null | undefined, + newCounters: RawCounter[] | null | undefined, + oldThreadIndexToNew: Map +): Map { + const result = new Map(); + if (!oldCounters || !newCounters) { + return result; + } + const newKeyToIndex = new Map(); + for (let i = 0; i < newCounters.length; i++) { + const c = newCounters[i]; + newKeyToIndex.set( + `${c.pid}|${c.category}|${c.name}|${c.mainThreadIndex}`, + i + ); + } + for (let i = 0; i < oldCounters.length; i++) { + const c = oldCounters[i]; + const newMainThreadIndex = oldThreadIndexToNew.get(c.mainThreadIndex); + if (newMainThreadIndex === undefined) { + // The counter's parent thread is gone in the new profile, so the + // counter is gone too. + continue; + } + const key = `${c.pid}|${c.category}|${c.name}|${newMainThreadIndex}`; + const newIndex = newKeyToIndex.get(key); + if (newIndex !== undefined) { + result.set(i, newIndex); + } + } + return result; +} + export type ProfileEncodingResult = | { type: 'SUCCESS'; @@ -413,6 +474,95 @@ export function attemptToPublish( if (removeProfileInformation) { const { committedRanges, translationMaps, profile } = sanitizedInformation; + + // When sanitization re-indexed tracks, translate the URL state's + // track-index references through old → new track-index space so the + // user's choices for tracks that survived sanitization stay attached + // to the right tracks. + let remappedHiddenGlobalTracks: Set | null = null; + let remappedGlobalTrackOrder: TrackIndex[] | null = null; + let remappedHiddenLocalTracksByPid: Map> | null = + null; + let remappedLocalTrackOrderByPid: Map | null = null; + + const oldThreadIndexToNew = translationMaps?.oldThreadIndexToNew; + if (oldThreadIndexToNew) { + const oldProfile = getProfile(prePublishedState); + const oldCounterIndexToNew = _computeOldCounterIndexToNew( + oldProfile.counters, + profile.counters, + oldThreadIndexToNew + ); + + const tabToThreadIndexesMap = + getTabToThreadIndexesMap(prePublishedState); + const tabFilter = getTabFilter(prePublishedState); + const newGlobalTracks = computeGlobalTracks( + profile, + tabFilter, + tabToThreadIndexesMap + ); + const newLocalTracksByPid = computeLocalTracksByPid( + profile, + newGlobalTracks + ); + + const oldGlobalTracks = getGlobalTracks(prePublishedState); + const oldLocalTracksByPid = getLocalTracksByPid(prePublishedState); + + const globalOldToNew = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks: oldGlobalTracks, + newTracks: newGlobalTracks, + oldThreadIndexToNew, + oldCounterIndexToNew, + }); + + remappedHiddenGlobalTracks = computeHiddenTracksAfterSanitization({ + oldHiddenTracks: getHiddenGlobalTracks(prePublishedState), + oldTrackIndexToNewTrackIndex: globalOldToNew, + }); + remappedGlobalTrackOrder = computeTrackOrderAfterSanitization({ + oldTrackOrder: getGlobalTrackOrder(prePublishedState), + oldTrackIndexToNewTrackIndex: globalOldToNew, + }); + + remappedHiddenLocalTracksByPid = new Map(); + remappedLocalTrackOrderByPid = new Map(); + const oldHiddenLocalByPid = + getHiddenLocalTracksByPid(prePublishedState); + const oldLocalOrderByPid = getLocalTrackOrderByPid(prePublishedState); + + for (const [pid, newLocalTracks] of newLocalTracksByPid) { + const oldLocalTracks = oldLocalTracksByPid.get(pid) ?? []; + const localOldToNew = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks: oldLocalTracks, + newTracks: newLocalTracks, + oldThreadIndexToNew, + oldCounterIndexToNew, + }); + const oldHiddenForPid = oldHiddenLocalByPid.get(pid); + if (oldHiddenForPid !== undefined) { + remappedHiddenLocalTracksByPid.set( + pid, + computeHiddenTracksAfterSanitization({ + oldHiddenTracks: oldHiddenForPid, + oldTrackIndexToNewTrackIndex: localOldToNew, + }) + ); + } + const oldOrderForPid = oldLocalOrderByPid.get(pid); + if (oldOrderForPid !== undefined) { + remappedLocalTrackOrderByPid.set( + pid, + computeTrackOrderAfterSanitization({ + oldTrackOrder: oldOrderForPid, + oldTrackIndexToNewTrackIndex: localOldToNew, + }) + ); + } + } + } + // Hide the old UI gracefully. await dispatch(hideStaleProfile()); @@ -423,7 +573,11 @@ export function attemptToPublish( committedRanges, translationMaps, profileName, - prePublishedState + prePublishedState, + remappedHiddenGlobalTracks, + remappedGlobalTrackOrder, + remappedHiddenLocalTracksByPid, + remappedLocalTrackOrderByPid ) ); @@ -504,13 +658,22 @@ export function resetUploadState(): Action { /** * Report to the UrlState that the profile was sanitized. This will re-map any stored * indexes or information that has been sanitized away. + * + * The four track-index payload fields carry URL state translated into the + * post-sanitization track-index space when sanitization re-indexed tracks + * (translationMaps.oldThreadIndexToNew is non-null). They are null when no + * remap is needed; in that case the existing reducer state is left untouched. */ export function profileSanitized( hash: string, committedRanges: StartEndRange[] | null, translationMaps: ProfileIndexTranslationMaps | null, profileName: string, - prePublishedState: State | null + prePublishedState: State | null, + hiddenGlobalTracks: Set | null = null, + globalTrackOrder: TrackIndex[] | null = null, + hiddenLocalTracksByPid: Map> | null = null, + localTrackOrderByPid: Map | null = null ): Action { return { type: 'SANITIZED_PROFILE_PUBLISHED', @@ -519,6 +682,10 @@ export function profileSanitized( translationMaps, profileName, prePublishedState, + hiddenGlobalTracks, + globalTrackOrder, + hiddenLocalTracksByPid, + localTrackOrderByPid, }; } diff --git a/src/profile-logic/tracks.ts b/src/profile-logic/tracks.ts index ea5c690bbb..feea152152 100644 --- a/src/profile-logic/tracks.ts +++ b/src/profile-logic/tracks.ts @@ -10,8 +10,10 @@ import type { Pid, GlobalTrack, LocalTrack, + Track, TrackIndex, RawCounter, + CounterIndex, Tid, TrackReference, TabID, @@ -496,6 +498,148 @@ export function addProcessCPUTracksForProcess( return newLocalTracksByPid; } +/** + * Build an identity key for a track based on properties that survive + * sanitization. When a translation map is provided for the dimension a track + * keys on, the source-side index is normalized through it; a missing entry + * means the track was removed and the key is null. Marker tracks key on a + * string-table index that sanitization reshuffles, so they're not matched + * here. + */ +function _trackIdentityKey( + track: Track, + threadIndexTranslation: Map | null, + counterIndexTranslation: Map | null +): string | null { + switch (track.type) { + case 'process': + return `process:${track.pid}`; + case 'screenshots': + return `screenshots:${track.id}`; + case 'visual-progress': + case 'perceptual-visual-progress': + case 'contentful-visual-progress': + return track.type; + case 'thread': + case 'network': + case 'ipc': + case 'event-delay': { + let ti: ThreadIndex | undefined = track.threadIndex; + if (threadIndexTranslation !== null) { + ti = threadIndexTranslation.get(track.threadIndex); + if (ti === undefined) { + return null; + } + } + return `${track.type}:${ti}`; + } + case 'counter': { + let ci: CounterIndex | undefined = track.counterIndex; + if (counterIndexTranslation !== null) { + ci = counterIndexTranslation.get(track.counterIndex); + if (ci === undefined) { + return null; + } + } + return `counter:${ci}`; + } + case 'marker': + return null; + default: + throw assertExhaustiveCheck(track, 'Unhandled Track type.'); + } +} + +/** + * Map each old TrackIndex to its new TrackIndex when both old and new track + * lists describe the same profile across a sanitization step. Tracks are + * matched by stable identity (pid, screenshot id, threadIndex, counterIndex, + * or visual-progress singleton); old-side thread- and counter-index values + * are normalized through the supplied translation maps before comparison. + * Tracks with no match in `newTracks` are absent from the result. + */ +export function computeOldTrackIndexToNewTrackIndexMap({ + oldTracks, + newTracks, + oldThreadIndexToNew, + oldCounterIndexToNew, +}: { + readonly oldTracks: ReadonlyArray; + readonly newTracks: ReadonlyArray; + readonly oldThreadIndexToNew: Map | null; + readonly oldCounterIndexToNew: Map | null; +}): Map { + const newKeyToTrackIndex = new Map(); + for (let i = 0; i < newTracks.length; i++) { + const key = _trackIdentityKey(newTracks[i], null, null); + if (key !== null) { + newKeyToTrackIndex.set(key, i); + } + } + + const oldTrackIndexToNewTrackIndex = new Map(); + for (let i = 0; i < oldTracks.length; i++) { + const key = _trackIdentityKey( + oldTracks[i], + oldThreadIndexToNew, + oldCounterIndexToNew + ); + if (key === null) { + continue; + } + const newIndex = newKeyToTrackIndex.get(key); + if (newIndex !== undefined) { + oldTrackIndexToNewTrackIndex.set(i, newIndex); + } + } + + return oldTrackIndexToNewTrackIndex; +} + +/** + * Translate a Set of hidden track indexes from old to new track-index space. + * Hidden tracks whose old index has no match in the new track list are dropped. + */ +export function computeHiddenTracksAfterSanitization({ + oldHiddenTracks, + oldTrackIndexToNewTrackIndex, +}: { + readonly oldHiddenTracks: ReadonlySet; + readonly oldTrackIndexToNewTrackIndex: Map; +}): Set { + const newHiddenTracks = new Set(); + for (const oldIndex of oldHiddenTracks) { + const newIndex = oldTrackIndexToNewTrackIndex.get(oldIndex); + if (newIndex !== undefined) { + newHiddenTracks.add(newIndex); + } + } + return newHiddenTracks; +} + +/** + * Translate a track-order array from old to new track-index space, preserving + * relative order. Indexes with no match in the new track list are dropped; + * `initializeGlobalTrackOrder` / `initializeLocalTrackOrderByPid` append any + * new tracks not represented in the input order. + */ +export function computeTrackOrderAfterSanitization({ + oldTrackOrder, + oldTrackIndexToNewTrackIndex, +}: { + readonly oldTrackOrder: ReadonlyArray; + readonly oldTrackIndexToNewTrackIndex: Map; +}): TrackIndex[] { + const newTrackOrder: TrackIndex[] = []; + for (const oldIndex of oldTrackOrder) { + const newIndex = oldTrackIndexToNewTrackIndex.get(oldIndex); + if (newIndex !== undefined) { + newTrackOrder.push(newIndex); + } + } + return newTrackOrder; +} + /** * Take a profile and figure out what GlobalTracks it contains. */ diff --git a/src/reducers/url-state.ts b/src/reducers/url-state.ts index 724b545b9f..085244f0cc 100644 --- a/src/reducers/url-state.ts +++ b/src/reducers/url-state.ts @@ -362,9 +362,10 @@ const globalTrackOrder: Reducer = (state = [], action) => { case 'CHANGE_TAB_FILTER': return action.globalTrackOrder; case 'SANITIZED_PROFILE_PUBLISHED': - // If some threads were removed, do not even attempt to figure this out. It's - // complicated, and not many people use this feature. - return action.translationMaps?.oldThreadIndexToNew ? [] : state; + // The action carries a track order in the post-sanitization track-index + // space when sanitization re-indexed tracks; otherwise it is null and + // the existing state remains valid. + return action.globalTrackOrder ?? state; default: return state; } @@ -411,9 +412,10 @@ const hiddenGlobalTracks: Reducer> = ( return hiddenGlobalTracks; } case 'SANITIZED_PROFILE_PUBLISHED': - // If any threads were removed, this was because they were hidden. - // Reset this state. - return action.translationMaps?.oldThreadIndexToNew ? new Set() : state; + // The action carries the hidden-track set in the post-sanitization + // track-index space when sanitization re-indexed tracks; otherwise it + // is null and the existing state remains valid. + return action.hiddenGlobalTracks ?? state; default: return state; } @@ -497,8 +499,10 @@ const hiddenLocalTracksByPid: Reducer>> = ( return hiddenLocalTracksByPid; } case 'SANITIZED_PROFILE_PUBLISHED': - // If any threads were removed then this information is no longer valid. - return action.translationMaps?.oldThreadIndexToNew ? new Map() : state; + // The action carries the hidden-local-track map in the post- + // sanitization track-index space when sanitization re-indexed tracks; + // otherwise it is null and the existing state remains valid. + return action.hiddenLocalTracksByPid ?? state; default: return state; } @@ -520,9 +524,10 @@ const localTrackOrderByPid: Reducer> = ( return localTrackOrderByPid; } case 'SANITIZED_PROFILE_PUBLISHED': - // If any threads were removed then remove this information. It's complicated - // to compute, and not many people use it. - return action.translationMaps?.oldThreadIndexToNew ? new Map() : state; + // The action carries the local-track order map in the post-sanitization + // track-index space when sanitization re-indexed tracks; otherwise it + // is null and the existing state remains valid. + return action.localTrackOrderByPid ?? state; default: return state; } @@ -538,10 +543,18 @@ const localTrackOrderChangedPids: Reducer> = ( localTrackOrderChangedPids.add(action.pid); return localTrackOrderChangedPids; } - case 'SANITIZED_PROFILE_PUBLISHED': - // In localTrackOrderByPid above the state is reset in this case, - // let's reset it here as well. - return action.translationMaps?.oldThreadIndexToNew ? new Set() : state; + case 'SANITIZED_PROFILE_PUBLISHED': { + // Drop pids that no longer have a local track order in the sanitized + // profile. When sanitization didn't re-index any tracks, the action's + // localTrackOrderByPid is null and the existing set remains valid. + const newLocalTrackOrderByPid = action.localTrackOrderByPid; + if (newLocalTrackOrderByPid === null) { + return state; + } + return new Set( + [...state].filter((pid) => newLocalTrackOrderByPid.has(pid)) + ); + } default: return state; } diff --git a/src/test/store/publish.test.ts b/src/test/store/publish.test.ts index 1b9f9f5c74..b86dc065a3 100644 --- a/src/test/store/publish.test.ts +++ b/src/test/store/publish.test.ts @@ -32,11 +32,19 @@ import { getPathInZipFileFromUrl, getHash, getTransformStack, + getHiddenGlobalTracks, } from '../../selectors/url-state'; -import { getHasPreferenceMarkers } from '../../selectors/profile'; +import { + getHasPreferenceMarkers, + getGlobalTracks, +} from '../../selectors/profile'; import { urlFromState } from '../../app-logic/url-handling'; import { getHasZipFile } from '../../selectors/zipped-profiles'; -import { getProfileFromTextSamples } from '../fixtures/profiles/processed-profile'; +import { + getProfileFromTextSamples, + addRawMarkersToThread, + makeCompositorScreenshot, +} from '../fixtures/profiles/processed-profile'; import { getProfileWithFakeGlobalTrack, getHumanReadableTracks, @@ -553,6 +561,74 @@ describe('attemptToPublish', function () { expect(formatTree(callTreeAfter)).toEqual(['- C (total: 1, self: 1)']); }); + it('keeps a hidden screenshot track hidden after sanitization', async function () { + const { profile } = getProfileFromTextSamples('A', 'B'); + + // Cause sanitization on publish. + profile.meta.updateChannel = 'release'; + + // Two main threads in different processes. + profile.threads[0].name = 'GeckoMain'; + profile.threads[0].isMainThread = true; + profile.threads[0].pid = '111'; + profile.threads[1].name = 'GeckoMain'; + profile.threads[1].isMainThread = true; + profile.threads[1].pid = '222'; + + // Add a CompositorScreenshot marker to thread 0 (pid 111) so a screenshot + // global track is created. + addRawMarkersToThread(profile.threads[0], profile.shared, [ + makeCompositorScreenshot(0.5), + ]); + + const store = storeWithProfile(profile); + const { dispatch, getState, resolveUpload, assertUploadSuccess } = + setupFakeUploadsWithStore(store); + + // Keep screenshots in the sanitized profile so the screenshot track + // survives sanitization (only the hidden thread will be removed). + dispatch(updateSharingOption('includeScreenshots', true)); + + // Locate the global track indexes. + const globalTracksBefore = getGlobalTracks(getState()); + const screenshotTrackIndex = globalTracksBefore.findIndex( + (t) => t.type === 'screenshots' + ); + const secondProcessTrackIndex = globalTracksBefore.findIndex( + (t) => t.type === 'process' && t.pid === '222' + ); + expect(screenshotTrackIndex).toBeGreaterThanOrEqual(0); + expect(secondProcessTrackIndex).toBeGreaterThanOrEqual(0); + + // Hide both: the screenshot track stays in the profile (because + // includeScreenshots is true), but pid 222 is sanitized away (because + // includeHiddenThreads is false by default for release-channel profiles). + dispatch(hideGlobalTrack(screenshotTrackIndex)); + dispatch(hideGlobalTrack(secondProcessTrackIndex)); + + expect(getHiddenGlobalTracks(getState())).toEqual( + new Set([screenshotTrackIndex, secondProcessTrackIndex]) + ); + + // Publish with sanitization. + const publishAttempt = dispatch(attemptToPublish()); + resolveUpload(JWT_TOKEN); + await assertUploadSuccess(publishAttempt); + + // After publish, the screenshot track should still exist and still be + // hidden in the URL state, with its TrackIndex remapped to the post- + // sanitization position. + const globalTracksAfter = getGlobalTracks(getState()); + const newScreenshotTrackIndex = globalTracksAfter.findIndex( + (t) => t.type === 'screenshots' + ); + expect(newScreenshotTrackIndex).toBeGreaterThanOrEqual(0); + + expect(getHiddenGlobalTracks(getState())).toEqual( + new Set([newScreenshotTrackIndex]) + ); + }); + describe('with zip files', function () { const setupZipFileTests = async () => { const { store } = await storeWithZipFile([ diff --git a/src/test/unit/tracks-sanitization.test.ts b/src/test/unit/tracks-sanitization.test.ts new file mode 100644 index 0000000000..1e7e4bb2a9 --- /dev/null +++ b/src/test/unit/tracks-sanitization.test.ts @@ -0,0 +1,225 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import { + computeOldTrackIndexToNewTrackIndexMap, + computeHiddenTracksAfterSanitization, + computeTrackOrderAfterSanitization, +} from '../../profile-logic/tracks'; + +import type { Track } from 'firefox-profiler/types'; + +describe('computeOldTrackIndexToNewTrackIndexMap', function () { + it('matches process tracks by pid', function () { + const oldTracks: Track[] = [ + { type: 'process', pid: '1', mainThreadIndex: 0 }, + { type: 'process', pid: '2', mainThreadIndex: 1 }, + { type: 'process', pid: '3', mainThreadIndex: 2 }, + ]; + const newTracks: Track[] = [ + { type: 'process', pid: '1', mainThreadIndex: 0 }, + { type: 'process', pid: '3', mainThreadIndex: 1 }, + ]; + const map = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks, + newTracks, + oldThreadIndexToNew: null, + oldCounterIndexToNew: null, + }); + expect(map.get(0)).toBe(0); + expect(map.has(1)).toBe(false); + expect(map.get(2)).toBe(1); + }); + + it('matches screenshot tracks by id', function () { + const oldTracks: Track[] = [ + { type: 'screenshots', id: 'win-A', threadIndex: 0 }, + { type: 'screenshots', id: 'win-B', threadIndex: 0 }, + ]; + const newTracks: Track[] = [ + { type: 'screenshots', id: 'win-B', threadIndex: 0 }, + ]; + const map = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks, + newTracks, + oldThreadIndexToNew: null, + oldCounterIndexToNew: null, + }); + expect(map.has(0)).toBe(false); + expect(map.get(1)).toBe(0); + }); + + it('matches visual-progress singletons by type', function () { + const oldTracks: Track[] = [ + { type: 'visual-progress' }, + { type: 'perceptual-visual-progress' }, + { type: 'contentful-visual-progress' }, + ]; + const newTracks: Track[] = [ + { type: 'contentful-visual-progress' }, + { type: 'visual-progress' }, + ]; + const map = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks, + newTracks, + oldThreadIndexToNew: null, + oldCounterIndexToNew: null, + }); + expect(map.get(0)).toBe(1); // visual-progress + expect(map.has(1)).toBe(false); // perceptual was removed + expect(map.get(2)).toBe(0); // contentful + }); + + it('translates threadIndex via oldThreadIndexToNew for thread-keyed tracks', function () { + const oldTracks: Track[] = [ + { type: 'thread', threadIndex: 0 }, + { type: 'network', threadIndex: 1 }, + { type: 'ipc', threadIndex: 2 }, + { type: 'event-delay', threadIndex: 3 }, + ]; + // After sanitization thread #1 was removed; the others were renumbered. + const oldThreadIndexToNew = new Map([ + [0, 0], + [2, 1], + [3, 2], + ]); + const newTracks: Track[] = [ + { type: 'thread', threadIndex: 0 }, + { type: 'ipc', threadIndex: 1 }, + { type: 'event-delay', threadIndex: 2 }, + ]; + const map = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks, + newTracks, + oldThreadIndexToNew, + oldCounterIndexToNew: null, + }); + expect(map.get(0)).toBe(0); // thread:0 → new threadIndex 0 + expect(map.has(1)).toBe(false); // network:1 has no surviving thread + expect(map.get(2)).toBe(1); // ipc:2 → new threadIndex 1 + expect(map.get(3)).toBe(2); // event-delay:3 → new threadIndex 2 + }); + + it('translates counterIndex via oldCounterIndexToNew for counter tracks', function () { + const oldTracks: Track[] = [ + { type: 'counter', counterIndex: 0 }, + { type: 'counter', counterIndex: 1 }, + { type: 'counter', counterIndex: 2 }, + ]; + // Counter #1 was sanitized away; #2 is now at index 1. + const oldCounterIndexToNew = new Map([ + [0, 0], + [2, 1], + ]); + const newTracks: Track[] = [ + { type: 'counter', counterIndex: 0 }, + { type: 'counter', counterIndex: 1 }, + ]; + const map = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks, + newTracks, + oldThreadIndexToNew: null, + oldCounterIndexToNew, + }); + expect(map.get(0)).toBe(0); + expect(map.has(1)).toBe(false); + expect(map.get(2)).toBe(1); + }); + + it('does not match marker tracks', function () { + // Marker tracks key on a string-table index that sanitization reshuffles, + // so the helper deliberately skips them. + const markerTrack = { + type: 'marker' as const, + threadIndex: 0, + markerSchema: { name: 'CustomA' } as any, + markerName: 42, + }; + const map = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks: [markerTrack as Track], + newTracks: [markerTrack as Track], + oldThreadIndexToNew: null, + oldCounterIndexToNew: null, + }); + expect(map.has(0)).toBe(false); + }); + + it('handles a mixed track list (process + screenshots + visual-progress)', function () { + const oldTracks: Track[] = [ + { type: 'process', pid: '1', mainThreadIndex: 0 }, + { type: 'screenshots', id: 'win-A', threadIndex: 0 }, + { type: 'process', pid: '2', mainThreadIndex: 1 }, + { type: 'visual-progress' }, + ]; + const newTracks: Track[] = [ + { type: 'process', pid: '1', mainThreadIndex: 0 }, + { type: 'visual-progress' }, + ]; + const map = computeOldTrackIndexToNewTrackIndexMap({ + oldTracks, + newTracks, + oldThreadIndexToNew: null, + oldCounterIndexToNew: null, + }); + expect(map.get(0)).toBe(0); // process pid 1 + expect(map.has(1)).toBe(false); // screenshot win-A removed + expect(map.has(2)).toBe(false); // process pid 2 removed + expect(map.get(3)).toBe(1); // visual-progress preserved + }); +}); + +describe('computeHiddenTracksAfterSanitization', function () { + it('drops hidden indexes whose track was sanitized away', function () { + const oldTrackIndexToNewTrackIndex = new Map([ + [0, 0], + [2, 1], + [3, 2], + ]); + const result = computeHiddenTracksAfterSanitization({ + oldHiddenTracks: new Set([0, 1, 3]), + oldTrackIndexToNewTrackIndex, + }); + // Old 0 → new 0 (kept); old 1 has no mapping (sanitized away); old 3 → new 2. + expect([...result].sort()).toEqual([0, 2]); + }); + + it('returns an empty set when no hidden tracks survive', function () { + const result = computeHiddenTracksAfterSanitization({ + oldHiddenTracks: new Set([5, 6, 7]), + oldTrackIndexToNewTrackIndex: new Map([[0, 0]]), + }); + expect(result.size).toBe(0); + }); + + it('returns an empty set when no tracks are hidden', function () { + const result = computeHiddenTracksAfterSanitization({ + oldHiddenTracks: new Set(), + oldTrackIndexToNewTrackIndex: new Map([[0, 0]]), + }); + expect(result.size).toBe(0); + }); +}); + +describe('computeTrackOrderAfterSanitization', function () { + it('preserves relative order, dropping unmapped indexes', function () { + const oldTrackIndexToNewTrackIndex = new Map([ + [0, 2], + [1, 0], + [3, 1], + ]); + const result = computeTrackOrderAfterSanitization({ + oldTrackOrder: [3, 0, 1, 2], // index 2 has no mapping → dropped + oldTrackIndexToNewTrackIndex, + }); + expect(result).toEqual([1, 2, 0]); + }); + + it('returns an empty array when no order entries survive', function () { + const result = computeTrackOrderAfterSanitization({ + oldTrackOrder: [5, 6, 7], + oldTrackIndexToNewTrackIndex: new Map([[0, 0]]), + }); + expect(result).toEqual([]); + }); +}); diff --git a/src/types/actions.ts b/src/types/actions.ts index 8bd6eadb26..e49eddc7ec 100644 --- a/src/types/actions.ts +++ b/src/types/actions.ts @@ -558,6 +558,14 @@ type UrlStateAction = readonly translationMaps: ProfileIndexTranslationMaps | null; readonly profileName: string; readonly prePublishedState: State | null; + // URL-state track-index references in the post-sanitization track-index + // space, populated when sanitization re-indexed tracks + // (translationMaps.oldThreadIndexToNew is non-null). When null, the + // existing reducer state is left untouched. + readonly hiddenGlobalTracks: Set | null; + readonly globalTrackOrder: TrackIndex[] | null; + readonly hiddenLocalTracksByPid: Map> | null; + readonly localTrackOrderByPid: Map | null; } | { readonly type: 'SET_DATA_SOURCE'; From d935cc70a5e90b11a2f2414781af1671695a52f4 Mon Sep 17 00:00:00 2001 From: fatadel Date: Mon, 18 May 2026 14:05:42 +0200 Subject: [PATCH 2/2] Recompute tabToThreadIndexesMap from the sanitized profile When a tab filter was active during publishing, a hidden non-thread track (e.g. a screenshot or a per-process network track) could reappear after sanitization. attemptToPublish was remapping the URL state's hidden-track set against a global track list filtered using the prePublishedState's tabToThreadIndexesMap; that map's Set values referred to old thread indexes, so when sanitization shifted surviving threads down by removing an earlier thread, filterGlobalTracksByTab picked the wrong threads and the hidden track ended up at a stale TrackIndex. attemptToPublish now builds the map from the sanitized profile's threads and pages so its ThreadIndex values are valid in the new track-index space. The innerWindowID to tabID computation is extracted into a shared computeInnerWindowIDToTabMap helper that the getInnerWindowIDToTabMap selector also uses. --- src/actions/publish.ts | 15 +++-- src/profile-logic/profile-data.ts | 17 ++++++ src/selectors/profile.ts | 15 +---- src/test/store/publish.test.ts | 97 +++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 17 deletions(-) diff --git a/src/actions/publish.ts b/src/actions/publish.ts index d481c5af21..4ee038e2d7 100644 --- a/src/actions/publish.ts +++ b/src/actions/publish.ts @@ -29,7 +29,6 @@ import { getCommittedRange, getGlobalTracks, getLocalTracksByPid, - getTabToThreadIndexesMap, } from 'firefox-profiler/selectors/profile'; import { viewProfile } from './receive-profile'; import { ensureExists } from 'firefox-profiler/utils/types'; @@ -43,6 +42,10 @@ import { computeHiddenTracksAfterSanitization, computeTrackOrderAfterSanitization, } from 'firefox-profiler/profile-logic/tracks'; +import { + computeInnerWindowIDToTabMap, + computeTabToThreadIndexesMap, +} from 'firefox-profiler/profile-logic/profile-data'; import type { Action, @@ -494,13 +497,17 @@ export function attemptToPublish( oldThreadIndexToNew ); - const tabToThreadIndexesMap = - getTabToThreadIndexesMap(prePublishedState); + // The selector-derived map keys ThreadIndex values from the + // prePublishedState; rebuild it against the sanitized profile. + const newTabToThreadIndexesMap = computeTabToThreadIndexesMap( + profile.threads, + computeInnerWindowIDToTabMap(profile.pages) + ); const tabFilter = getTabFilter(prePublishedState); const newGlobalTracks = computeGlobalTracks( profile, tabFilter, - tabToThreadIndexesMap + newTabToThreadIndexesMap ); const newLocalTracksByPid = computeLocalTracksByPid( profile, diff --git a/src/profile-logic/profile-data.ts b/src/profile-logic/profile-data.ts index 80e3395805..91b202d3aa 100644 --- a/src/profile-logic/profile-data.ts +++ b/src/profile-logic/profile-data.ts @@ -4469,6 +4469,23 @@ export function determineTimelineType(profile: Profile): TimelineType { return 'cpu-category'; } +/** + * Compute an innerWindowID → tabID lookup from a profile's pages list. + * Returns null when the profile has no pages. + */ +export function computeInnerWindowIDToTabMap( + pages: PageList | null | undefined +): Map | null { + if (!pages) { + return null; + } + const innerWindowIDToTabMap = new Map(); + for (const page of pages) { + innerWindowIDToTabMap.set(page.innerWindowID, page.tabID); + } + return innerWindowIDToTabMap; +} + /** * Compute a map of tab to thread indexes map. This is useful for learning which * threads are involved for tabs. This is mainly used for the tab selector on diff --git a/src/selectors/profile.ts b/src/selectors/profile.ts index 236ad21ce4..ad7f8ff41c 100644 --- a/src/selectors/profile.ts +++ b/src/selectors/profile.ts @@ -16,6 +16,7 @@ import { getFriendlyThreadName, processCounter, getInclusiveSampleIndexRangeForSelection, + computeInnerWindowIDToTabMap, computeTabToThreadIndexesMap, computeStackTableFromRawStackTable, reserveFunctionsForCollapsedResources, @@ -428,19 +429,7 @@ export const getInnerWindowIDToPageMap: Selector | null> = createSelector(getPageList, (pages) => { - if (!pages) { - // Return null if there are no pages. - return null; - } - - const innerWindowIDToTabMap: Map = new Map(); - for (const page of pages) { - innerWindowIDToTabMap.set(page.innerWindowID, page.tabID); - } - - return innerWindowIDToTabMap; -}); +> | null> = createSelector(getPageList, computeInnerWindowIDToTabMap); /** * Return a map of tab to thread indexes map. This is useful for learning which diff --git a/src/test/store/publish.test.ts b/src/test/store/publish.test.ts index b86dc065a3..11cc9aaf1b 100644 --- a/src/test/store/publish.test.ts +++ b/src/test/store/publish.test.ts @@ -59,6 +59,7 @@ import { hideGlobalTrack, commitRange, } from '../../actions/profile-view'; +import { changeTabFilter } from '../../actions/receive-profile'; import { retrieveUploadedProfileInformationFromDb, @@ -629,6 +630,102 @@ describe('attemptToPublish', function () { ); }); + it('keeps a hidden screenshot track hidden after sanitization when a tab filter is active', async function () { + const { profile } = getProfileFromTextSamples('A', 'B', 'C', 'D', 'E'); + profile.meta.updateChannel = 'release'; + + profile.threads[0].name = 'GeckoMain'; + profile.threads[0].isMainThread = true; + profile.threads[0].processType = 'default'; + profile.threads[0].pid = '100'; + profile.threads[1].name = 'GeckoMain'; + profile.threads[1].isMainThread = true; + profile.threads[1].processType = 'tab'; + profile.threads[1].pid = '200'; + profile.threads[2].name = 'GeckoMain'; + profile.threads[2].isMainThread = true; + profile.threads[2].processType = 'tab'; + profile.threads[2].pid = '300'; + profile.threads[3].name = 'GeckoMain'; + profile.threads[3].isMainThread = true; + profile.threads[3].processType = 'tab'; + profile.threads[3].pid = '400'; + profile.threads[4].name = 'GeckoMain'; + profile.threads[4].isMainThread = true; + profile.threads[4].processType = 'tab'; + profile.threads[4].pid = '500'; + + const tab1ID = 1; + const tab2ID = 2; + const tab1InnerWindowID = 1001; + const tab2InnerWindowID = 1002; + profile.pages = [ + { + tabID: tab1ID, + innerWindowID: tab1InnerWindowID, + url: 'https://tab1.example.com/', + embedderInnerWindowID: 0, + }, + { + tabID: tab2ID, + innerWindowID: tab2InnerWindowID, + url: 'https://tab2.example.com/', + embedderInnerWindowID: 0, + }, + ]; + profile.threads[0].usedInnerWindowIDs = [tab1InnerWindowID]; + profile.threads[1].usedInnerWindowIDs = [tab1InnerWindowID]; + profile.threads[2].usedInnerWindowIDs = [tab1InnerWindowID]; + profile.threads[3].usedInnerWindowIDs = [tab2InnerWindowID]; + profile.threads[4].usedInnerWindowIDs = [tab2InnerWindowID]; + + addRawMarkersToThread(profile.threads[2], profile.shared, [ + makeCompositorScreenshot(0.5), + ]); + + const store = storeWithProfile(profile); + const { dispatch, getState, resolveUpload, assertUploadSuccess } = + setupFakeUploadsWithStore(store); + + dispatch(updateSharingOption('includeScreenshots', true)); + dispatch(changeTabFilter(tab1ID)); + + const globalTracksBefore = getGlobalTracks(getState()); + const pid200TrackIndex = globalTracksBefore.findIndex( + (t) => t.type === 'process' && t.pid === '200' + ); + const screenshotTrackIndex = globalTracksBefore.findIndex( + (t) => t.type === 'screenshots' + ); + expect(pid200TrackIndex).toBeGreaterThanOrEqual(0); + expect(screenshotTrackIndex).toBeGreaterThanOrEqual(0); + + dispatch(hideGlobalTrack(pid200TrackIndex)); + dispatch(hideGlobalTrack(screenshotTrackIndex)); + + expect(getHiddenGlobalTracks(getState())).toContain(screenshotTrackIndex); + + const publishAttempt = dispatch(attemptToPublish()); + resolveUpload(JWT_TOKEN); + await assertUploadSuccess(publishAttempt); + + const globalTracksAfter = getGlobalTracks(getState()); + expect( + globalTracksAfter.some((t) => t.type === 'process' && t.pid === '200') + ).toBe(false); + expect( + globalTracksAfter.some((t) => t.type === 'process' && t.pid === '300') + ).toBe(true); + const newScreenshotTrackIndex = globalTracksAfter.findIndex( + (t) => t.type === 'screenshots' + ); + expect(newScreenshotTrackIndex).toBeGreaterThanOrEqual(0); + + expect(getHiddenGlobalTracks(getState())).toContain( + newScreenshotTrackIndex + ); + }); + describe('with zip files', function () { const setupZipFileTests = async () => { const { store } = await storeWithZipFile([