From f3dc7d66aee589c98bdbcc0689eff33b8077d7ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Wed, 31 Jan 2024 11:59:37 +0100 Subject: [PATCH 1/3] Return null instead of throwing an error if it fails to get the resource name in active tab view Previously if it was failing to find a resource name, it was throwing an error. This was happening because we are explicitly excluding the `about:blank` and `about:newtab` pages. They are excluded so we can find the correct page name. --- src/profile-logic/active-tab.js | 53 +++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/src/profile-logic/active-tab.js b/src/profile-logic/active-tab.js index e4142acedb..f350437f8b 100644 --- a/src/profile-logic/active-tab.js +++ b/src/profile-logic/active-tab.js @@ -84,11 +84,17 @@ export function computeActiveTabTracks( mainTrackIndexes.push(threadIndex); } else { if (!isTabFilteredThreadEmpty(threadIndex, state)) { - resources.push({ - type: 'sub-frame', - threadIndex, - name: _getActiveTabResourceName(thread, innerWindowIDToPageMap), - }); + const resourceName = _getActiveTabResourceName( + thread, + innerWindowIDToPageMap + ); + if (resourceName !== null) { + resources.push({ + type: 'sub-frame', + threadIndex, + name: resourceName, + }); + } } } } else { @@ -96,11 +102,17 @@ export function computeActiveTabTracks( // track. Find out if that thread contains the active tab data, and add it // as a resource track if it does. if (!isTabFilteredThreadEmpty(threadIndex, state)) { - resources.push({ - type: 'thread', - threadIndex, - name: _getActiveTabResourceName(thread, innerWindowIDToPageMap), - }); + const resourceName = _getActiveTabResourceName( + thread, + innerWindowIDToPageMap + ); + if (resourceName !== null) { + resources.push({ + type: 'thread', + threadIndex, + name: resourceName, + }); + } } } @@ -189,11 +201,12 @@ function isTopmostThread( /** * Returns the name of active tab resource track. * It can be either a sub-frame or a regular thread. + * If it fails to find a name, returns null. */ function _getActiveTabResourceName( thread: Thread, innerWindowIDToPageMap: Map -): string { +): string | null { if (thread.isMainThread) { // This is a sub-frame. // Get the first innerWindowID inside the thread that's also present of innerWindowIDToPageMap. @@ -216,6 +229,16 @@ function _getActiveTabResourceName( // find them, so we can get the real resource name. const page = innerWindowIDToPageMap.get(data.innerWindowID); return ( + // During a page load, iframes usually start with an about:blank page + // and then they navigate to the url. While it's in about:blank, we + // might catch some markers. We would like to exclude them because + // `about:blank` name doesn't bring any value as a resource name and + // we would prefer to display the _real_ url instead which is + // captured after the about:blank. + // FIXME: I think we initially added `about:newtab` here to exclude + // the "new tab page -> website" navigation from showing the + // `about:newtab` all the time. But I don't think we have to worry + // about this for iframes. So maybe remove this? page && page.url !== 'about:blank' && page.url !== 'about:newtab' ); } @@ -225,9 +248,15 @@ function _getActiveTabResourceName( firstInnerWindowID = markerData.innerWindowID; } } + if (firstInnerWindowID === undefined || firstInnerWindowID === null) { - throw new Error('There must be an innerWindowID in the thread'); + // Since we excluded the about:blank and about:newtab pages, we might fail + // to find a valid innerWindowID. This might happen when an ad blocker + // blocks an iframe and therefore it fails to load. In that case, we + // should return null. + return null; } + const page = ensureExists(innerWindowIDToPageMap.get(firstInnerWindowID)); return page.url; } From cd5e6b7dc106bea8ebf78209c6636316e381efa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Wed, 31 Jan 2024 12:00:58 +0100 Subject: [PATCH 2/3] Make getHumanReadableActiveTabTracks also print the resource tracks For some reason we weren't printing the resource tracks. This wasn't great because it's important to see all the resources there to be able to test the active tab correctly. --- src/test/fixtures/profiles/tracks.js | 17 +++++++++++++++++ src/test/store/active-tab.test.js | 1 + 2 files changed, 18 insertions(+) diff --git a/src/test/fixtures/profiles/tracks.js b/src/test/fixtures/profiles/tracks.js index 2829255f2e..c14ffee4ed 100644 --- a/src/test/fixtures/profiles/tracks.js +++ b/src/test/fixtures/profiles/tracks.js @@ -337,6 +337,7 @@ export function getStoreWithMemoryTrack(pid: Pid = '222') { */ export function getHumanReadableActiveTabTracks(state: State): string[] { const globalTracks = profileViewSelectors.getActiveTabGlobalTracks(state); + const resourceTracks = profileViewSelectors.getActiveTabResourceTracks(state); const selectedThreadIndexes = urlStateSelectors.getSelectedThreadIndexes(state); const text: string[] = []; @@ -369,6 +370,22 @@ export function getHumanReadableActiveTabTracks(state: State): string[] { } } + for (const resourceTrack of resourceTracks) { + switch (resourceTrack.type) { + case 'sub-frame': + text.push(` - iframe: ${resourceTrack.name}`); + break; + case 'thread': + text.push(` - ${resourceTrack.name}`); + break; + default: + throw assertExhaustiveCheck( + resourceTrack, + 'Unhandled ActiveTabResourceTrack.' + ); + } + } + return text; } diff --git a/src/test/store/active-tab.test.js b/src/test/store/active-tab.test.js index 8a5268f7b6..6739d69f7a 100644 --- a/src/test/store/active-tab.test.js +++ b/src/test/store/active-tab.test.js @@ -165,6 +165,7 @@ describe('ActiveTab', function () { const { getState } = setup(profile, false); expect(getHumanReadableActiveTabTracks(getState())).toEqual([ 'main track [tab] SELECTED', + ' - iframe: Page #2', ]); }); }); From 706ddfb673da3e22c202a98091d682c4feabf8b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Wed, 31 Jan 2024 12:01:23 +0100 Subject: [PATCH 3/3] Add a test for the new behavior to make sure that we are not crashing This test is crashing when you revert the first commit. I had to change the setup function to make sure that it doesn't overwrite the profile.pages array that we overwritten already with a custom page url. --- src/test/store/active-tab.test.js | 68 ++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/src/test/store/active-tab.test.js b/src/test/store/active-tab.test.js index 6739d69f7a..753956ca8f 100644 --- a/src/test/store/active-tab.test.js +++ b/src/test/store/active-tab.test.js @@ -26,12 +26,21 @@ import { getTimelineType, } from '../../selectors/url-state'; import { stateFromLocation } from '../../app-logic/url-handling'; +import { ensureExists } from '../../utils/flow'; import type { Profile } from 'firefox-profiler/types'; describe('ActiveTab', function () { - function setup(p = getProfileWithNiceTracks(), addInnerWindowID = true) { - const { profile, ...pageInfo } = addActiveTabInformationToProfile(p); + function setup( + p = getProfileWithNiceTracks(), + addInnerWindowID = true, + pageInfo = null + ) { + let profile = p; + if (!pageInfo) { + ({ profile, ...pageInfo } = addActiveTabInformationToProfile(profile)); + } + // Add the innerWindowIDs so we can compute the first thread as main track. if (addInnerWindowID) { profile.threads[0].frameTable.innerWindowID[0] = @@ -168,6 +177,61 @@ describe('ActiveTab', function () { ' - iframe: Page #2', ]); }); + + it('do not show the thread when it fails to find the iframe resource name', function () { + const { + profile: p, + funcNamesDictPerThread: [firstThread], + } = getProfileFromTextSamples( + // First thread is the main thread of the first tab (which is the + // active tab) + `A`, + // The second thread is an iframe of the first thread. + `B` + ); + + p.threads[0].name = 'GeckoMain'; + p.threads[0].isMainThread = true; + p.threads[1].name = 'GeckoMain'; + p.threads[1].isMainThread = true; + + const { profile, ...pageInfo } = addActiveTabInformationToProfile(p); + addInnerWindowIdToStacks( + profile.threads[0], + /* listOfOperations */ + [ + // first tab is the active tab. + { + innerWindowID: pageInfo.firstTabInnerWindowIDs[0], + callNodes: [[firstThread.A]], + }, + ] + ); + addMarkersToThreadWithCorrespondingSamples(profile.threads[1], [ + // All about:blank or about:newtab markers are ignored during the + // track name computation because they don't provide the correct innerWindowID. + // This thread SHOULD NOT be shown in the tracks. + [ + 'This marker will be filtered', + 1, + 2, + { + type: 'tracing', + category: 'Navigation', + innerWindowID: pageInfo.iframeInnerWindowIDsWithChild, + }, + ], + ]); + + // Lastly, we need to put the iframe innerWindowID url to about:blank to test this case. + ensureExists(profile.pages)[1].url = 'about:blank'; + + const { getState } = setup(profile, false, pageInfo); + + expect(getHumanReadableActiveTabTracks(getState())).toEqual([ + 'main track [tab] SELECTED', + ]); + }); }); });