From bf36041db0efee9e386d12de399f057cfa2334bf Mon Sep 17 00:00:00 2001 From: Krzysztof Modras Date: Fri, 22 Nov 2024 16:16:14 +0100 Subject: [PATCH 1/5] Request: fix aborted navigations --- reporting/snapshots/0004/snapshot.json | 162 ++++++++++++++++--------- reporting/src/request/page-store.js | 24 ++++ 2 files changed, 132 insertions(+), 54 deletions(-) diff --git a/reporting/snapshots/0004/snapshot.json b/reporting/snapshots/0004/snapshot.json index 0f44685..9f5df19 100644 --- a/reporting/snapshots/0004/snapshot.json +++ b/reporting/snapshots/0004/snapshot.json @@ -19,13 +19,13 @@ "t": -1730735663859, "tps": { "a-v2.sndcdn.com": { - "c": 32, - "content_length": 444706, - "resp_ob": 32, - "scheme_https": 32, - "status_200": 32, - "type_2": 16, - "type_3": 15, + "c": 35, + "content_length": 447390, + "resp_ob": 35, + "scheme_https": 35, + "status_200": 35, + "type_2": 18, + "type_3": 16, "type_4": 1 }, "accounts.google.com": { @@ -55,19 +55,37 @@ "type_3": 2 }, "cf-hls-media.sndcdn.com": { - "c": 27, - "content_length": 3202255, - "has_qs": 27, - "resp_ob": 27, - "scheme_https": 27, - "status_200": 27, - "type_11": 27 + "c": 37, + "content_length": 3842022, + "has_qs": 37, + "resp_ob": 37, + "scheme_https": 37, + "status_200": 37, + "type_11": 37 + }, + "d2g5n8b622dsn7.cloudfront.net": { + "c": 1, + "content_length": 1, + "resp_ob": 1, + "scheme_https": 1, + "status_404": 1, + "type_11": 1 + }, + "events.statsigapi.net": { + "c": 2, + "content_length": 17, + "has_post": 1, + "resp_ob": 2, + "scheme_https": 2, + "status_200": 1, + "status_202": 1, + "type_11": 2 }, "featuregates.org": { - "c": 6, - "has_post": 3, - "scheme_https": 6, - "type_11": 6 + "c": 8, + "has_post": 4, + "scheme_https": 8, + "type_11": 8 }, "geolocation.onetrust.com": { "c": 1, @@ -75,12 +93,13 @@ "type_11": 1 }, "i1.sndcdn.com": { - "c": 13, - "content_length": 186264, - "resp_ob": 13, - "scheme_https": 13, - "status_200": 13, - "type_3": 13 + "c": 307, + "content_length": 826774, + "resp_ob": 307, + "scheme_https": 307, + "status_200": 307, + "type_11": 2, + "type_3": 305 }, "ingest.us.sentry.io": { "c": 1, @@ -118,9 +137,9 @@ "type_11": 1 }, "sb.scorecardresearch.com": { - "c": 1, - "scheme_https": 1, - "type_2": 1 + "c": 4, + "scheme_https": 4, + "type_2": 4 }, "secure.quantserve.com": { "c": 1, @@ -165,6 +184,14 @@ "scheme_https": 1, "type_2": 1 }, + "wave.sndcdn.com": { + "c": 4, + "content_length": 4921, + "resp_ob": 4, + "scheme_https": 4, + "status_200": 4, + "type_11": 4 + }, "www.googletagmanager.com": { "c": 3, "has_qs": 3, @@ -205,13 +232,13 @@ "t": -1730735663859, "tps": { "a-v2.sndcdn.com": { - "c": 32, - "content_length": 444706, - "resp_ob": 32, - "scheme_https": 32, - "status_200": 32, - "type_2": 16, - "type_3": 15, + "c": 35, + "content_length": 447390, + "resp_ob": 35, + "scheme_https": 35, + "status_200": 35, + "type_2": 18, + "type_3": 16, "type_4": 1 }, "assets.web.soundcloud.cloud": { @@ -241,19 +268,37 @@ "type_3": 2 }, "cf-hls-media.sndcdn.com": { - "c": 27, - "content_length": 3202255, - "has_qs": 27, - "resp_ob": 27, - "scheme_https": 27, - "status_200": 27, - "type_11": 27 + "c": 37, + "content_length": 3842022, + "has_qs": 37, + "resp_ob": 37, + "scheme_https": 37, + "status_200": 37, + "type_11": 37 + }, + "d2g5n8b622dsn7.cloudfront.net": { + "c": 1, + "content_length": 1, + "resp_ob": 1, + "scheme_https": 1, + "status_404": 1, + "type_11": 1 + }, + "events.statsigapi.net": { + "c": 2, + "content_length": 17, + "has_post": 1, + "resp_ob": 2, + "scheme_https": 2, + "status_200": 1, + "status_202": 1, + "type_11": 2 }, "featuregates.org": { - "c": 6, - "has_post": 3, - "scheme_https": 6, - "type_11": 6 + "c": 8, + "has_post": 4, + "scheme_https": 8, + "type_11": 8 }, "geolocation.onetrust.com": { "c": 1, @@ -261,12 +306,13 @@ "type_11": 1 }, "i1.sndcdn.com": { - "c": 13, - "content_length": 186264, - "resp_ob": 13, - "scheme_https": 13, - "status_200": 13, - "type_3": 13 + "c": 307, + "content_length": 826774, + "resp_ob": 307, + "scheme_https": 307, + "status_200": 307, + "type_11": 2, + "type_3": 305 }, "ingest.us.sentry.io": { "c": 1, @@ -304,9 +350,9 @@ "type_11": 1 }, "sb.scorecardresearch.com": { - "c": 1, - "scheme_https": 1, - "type_2": 1 + "c": 4, + "scheme_https": 4, + "type_2": 4 }, "secure.quantserve.com": { "c": 1, @@ -351,6 +397,14 @@ "scheme_https": 1, "type_2": 1 }, + "wave.sndcdn.com": { + "c": 4, + "content_length": 4921, + "resp_ob": 4, + "scheme_https": 4, + "status_200": 4, + "type_11": 4 + }, "www.googletagmanager.com": { "c": 3, "has_qs": 3, diff --git a/reporting/src/request/page-store.js b/reporting/src/request/page-store.js index 72eca6f..dd0d7e3 100644 --- a/reporting/src/request/page-store.js +++ b/reporting/src/request/page-store.js @@ -77,6 +77,9 @@ export default class PageStore { chrome.webNavigation.onBeforeNavigate.addListener(this.#onBeforeNavigate); chrome.webNavigation.onCommitted.addListener(this.#onNavigationCommitted); chrome.webNavigation.onCompleted.addListener(this.#onNavigationCompleted); + chrome.webNavigation.onErrorOccurred.addListener( + this.#onNavigationErrorOccured, + ); chrome.windows.onFocusChanged?.addListener(this.#onWindowFocusChanged); // popupate initially open tabs @@ -103,6 +106,9 @@ export default class PageStore { chrome.webNavigation.onCompleted.removeListener( this.#onNavigationCompleted, ); + chrome.webNavigation.onErrorOccurred.removeListener( + this.#onNavigationErrorOccured, + ); chrome.windows.onFocusChanged?.removeListener(this.#onWindowFocusChanged); } @@ -234,6 +240,24 @@ export default class PageStore { this.#pages.set(tabId, nextPage); }; + #onNavigationErrorOccured = (details) => { + const { frameId, tabId, url, error } = details; + + if (frameId !== 0) { + return; + } + + // navigation aborted most likely due to redirect + // revert to previous page as soon a new navigation will start + if (error === 'Error code 2152398850') { + const page = this.#pages.get(tabId); + + if (page && page.url === url) { + this.#pages.set(tabId, page.previous); + } + } + }; + #onNavigationCommitted = (details) => { const { frameId, tabId } = details; const page = this.#pages.get(tabId); From aa3ce4994523c9a530c3e32b721b475881a996ff Mon Sep 17 00:00:00 2001 From: Krzysztof Modras Date: Fri, 22 Nov 2024 17:10:41 +0100 Subject: [PATCH 2/5] Cleanup --- reporting/src/request/page-store.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/reporting/src/request/page-store.js b/reporting/src/request/page-store.js index dd0d7e3..d4c4d07 100644 --- a/reporting/src/request/page-store.js +++ b/reporting/src/request/page-store.js @@ -243,12 +243,21 @@ export default class PageStore { #onNavigationErrorOccured = (details) => { const { frameId, tabId, url, error } = details; - if (frameId !== 0) { + if (frameId !== 0 || !url.startsWith('http')) { return; } - // navigation aborted most likely due to redirect - // revert to previous page as soon a new navigation will start + /** + * + On Firefox some navigation can trigger following event sequence: + + * onBeforeNavigate + * onErrorOccurred with "error":"Error code 2152398850" + * onBeforeNavigate + * onCommitted + + That was causing an extra page object to be created which was messing up the previous page logic. + */ if (error === 'Error code 2152398850') { const page = this.#pages.get(tabId); From 891f855e2343cbd13a8200ffb8d1c22c500ea1ca Mon Sep 17 00:00:00 2001 From: Krzysztof Modras Date: Sun, 24 Nov 2024 18:07:21 +0100 Subject: [PATCH 3/5] prevent double staging --- reporting/snapshots/0006/snapshot.json | 10 ++++------ reporting/src/request/page-store.js | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/reporting/snapshots/0006/snapshot.json b/reporting/snapshots/0006/snapshot.json index 606e3ee..4cfcc60 100644 --- a/reporting/snapshots/0006/snapshot.json +++ b/reporting/snapshots/0006/snapshot.json @@ -950,15 +950,14 @@ }, "www.google.com": { "c": 3, - "content_length": 46, + "content_length": 44, "has_post": 1, "has_qs": 3, "referer_leak_path": 3, "referer_leak_site": 3, - "resp_ob": 5, + "resp_ob": 3, "scheme_https": 3, "status_200": 2, - "status_204": 2, "status_302": 1, "type_19": 2, "type_3": 1 @@ -1959,15 +1958,14 @@ }, "www.google.com": { "c": 3, - "content_length": 46, + "content_length": 44, "has_post": 1, "has_qs": 3, "referer_leak_path": 3, "referer_leak_site": 3, - "resp_ob": 5, + "resp_ob": 3, "scheme_https": 3, "status_200": 2, - "status_204": 2, "status_302": 1, "type_19": 2, "type_3": 1 diff --git a/reporting/src/request/page-store.js b/reporting/src/request/page-store.js index d4c4d07..88658c7 100644 --- a/reporting/src/request/page-store.js +++ b/reporting/src/request/page-store.js @@ -220,7 +220,7 @@ export default class PageStore { } // We are starting a navigation to a new page - if the previous page is complete (i.e. fully // loaded), stage it before we create the new page info. - if (page.state === PAGE_LOADING_STATE.COMPLETE) { + if (page.state === PAGE_LOADING_STATE.COMPLETE && !page.destroyed) { this.#stagePage(page); } } From 934a05bffd71e0a4e5131097d3fc2cde59889768 Mon Sep 17 00:00:00 2001 From: Krzysztof Modras Date: Mon, 2 Dec 2024 14:38:02 +0100 Subject: [PATCH 4/5] cleanup --- reporting/src/request/page-store.js | 1 + 1 file changed, 1 insertion(+) diff --git a/reporting/src/request/page-store.js b/reporting/src/request/page-store.js index 88658c7..a17576e 100644 --- a/reporting/src/request/page-store.js +++ b/reporting/src/request/page-store.js @@ -258,6 +258,7 @@ export default class PageStore { That was causing an extra page object to be created which was messing up the previous page logic. */ + // 2152398850 stands for NS_BINDING_ABORTED https://searchfox.org/mozilla-central/rev/6597dd03bad82c891d084eed25cafd0c85fb333e/tools/ts/config/error_list.json#48 if (error === 'Error code 2152398850') { const page = this.#pages.get(tabId); From 58e93ca594c2fed87db018a4cfae949a6de12ae0 Mon Sep 17 00:00:00 2001 From: Krzysztof Modras Date: Tue, 3 Dec 2024 18:37:46 +0100 Subject: [PATCH 5/5] Move destroyed check to stagePage --- reporting/src/request/page-store.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/reporting/src/request/page-store.js b/reporting/src/request/page-store.js index a17576e..189db55 100644 --- a/reporting/src/request/page-store.js +++ b/reporting/src/request/page-store.js @@ -8,7 +8,7 @@ * 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 logger from '../logger.js'; import ChromeStorageMap from './utils/chrome-storage-map.js'; const PAGE_TTL = 1000 * 60 * 60; // 1 hour @@ -118,6 +118,10 @@ export default class PageStore { } #stagePage(page) { + if (page.destroyed) { + logger.warn('[PageStore] trying to stage a page multipe times', page.url); + return; + } makePageActive(page, false); page.destroyed = Date.now(); // unset previous (to prevent history chain memory leak) @@ -220,7 +224,7 @@ export default class PageStore { } // We are starting a navigation to a new page - if the previous page is complete (i.e. fully // loaded), stage it before we create the new page info. - if (page.state === PAGE_LOADING_STATE.COMPLETE && !page.destroyed) { + if (page.state === PAGE_LOADING_STATE.COMPLETE) { this.#stagePage(page); } }