rpl
commented
1 year ago I looked some more into this and adding a Firefox DevTools logpoint on the detectFacebookOnPage function confirms that (likely starting from the addition of the DOM mutation observer introduced in https://github.com/mozilla/contain-facebook/pull/840) it is being called "a lot".
Each time the detectFacebookOnPage async function is called, the extension is exchanging some extension messages to retrieve options from the background page, on pages that triggers the mutation observer for a large enough number of DOM elements the extension seems to be flooding the background script with messages to retrieve the options needed to decide if the content script should be run patternDetection(EMAIL_PATTERN_DETECTION_SELECTORS, "email", target); (which is specifically related to the Firefox Relay integrations in Facebook container).
Based on that a possible smaller change to reduce the performance impact observed in Bug 1794686 (the redash page linked in the bugzilla issue includes a quite a lot SVG elements that are created from the telemetry data to build the SVG-based graphs in the redash dashboard) may be to just call detectFacebookOnPage(document) once when the number of the DOM elements collected by the DOM mutation observer are more than a certain threshold, given that the main point of calling detectFacebookOnPage is to run call patternDetection(...) and doing that for each of the DOM element mutated is going to be more expensive than calling it just once for the entire document.
That change may look like:
diff --git a/src/content_script.js b/src/content_script.js
index 6446513..9369c3b 100755
--- a/src/content_script.js
+++ b/src/content_script.js
@@ -5,6 +5,11 @@
// "[title*='Facebook']",
// "[aria-label*='Facebook']",
+// Maximum number of mutated elements that would be worth to process
+// one by one using detectFacebookOnPage, fallback to process the entire
+// document once when the mutationsSet.size is higher than this value.
+const MAX_MUTATEDSET_SIZE = ...;
+
const EMAIL_PATTERN_DETECTION_SELECTORS = [
"input[type='email']",
];
@@ -870,7 +875,7 @@ async function CheckIfURLShouldBeBlocked() {
// Reinitialize the content script for mutated nodes
const observer = new MutationObserver((mutations) => {
- new Set(mutations.flatMap(mutation => {
+ const mutationsSet = new Set(mutations.flatMap(mutation => {
switch (mutation.type) {
case "attributes":
return mutation.target;
@@ -880,7 +885,12 @@ async function CheckIfURLShouldBeBlocked() {
default:
return [];
}
- })).forEach(target => contentScriptInit(false, null, target));
+ }));
+ if (mutationsSet.size > MAX_MUTATEDSET_SIZE) {
+ contentScriptInit(false, null, document);
+ } else {
+ mutationsSet.forEach(target => contentScriptInit(false, null, target));
+ }
});
// Check for mutations in the entire documentTo be fair, I'd guess that the options that are being retrieved through the extension messaging API should not be changing that often: https://github.com/mozilla/contain-facebook/blob/1cbf3532201f6ed5f10b2442e2525304fa882950/src/content_script.js#L717-L725
And so, a more as part of a more complete refactoring of the content script it may be worth considering retrieving those options just once from the caller side and pass them to the detectFacebookOnPage function, which may also help to reduce the impact of calling that function for each of the mutated DOM elements detected, which may be done with some more minimal changes with something like the following (diff I applied locally on top of the other diff from this comment)
diff --git a/src/content_script.js b/src/content_script.js
index 49df112..a99f885 100755
--- a/src/content_script.js
+++ b/src/content_script.js
@@ -86,6 +86,17 @@ const OBSERVER_ATTRIBUTES = [
"data-oauthserver", "data-partner", "data-tag", "data-test-id", "data-tracking",
"href", "id", "title"];
+async function getContentScriptOptions() {
+ const options = {};
+ options.relayAddonEnabled = await getRelayAddonEnabledFromBackground();
+ // Check if any FB trackers were blocked, scoped to only the active tab
+ options.trackersDetectedOnCurrentPage = await checkIfTrackersAreDetectedOnCurrentPage();
+ // Check if user dismissed the Relay prompt
+ options.relayAddonPromptDismissed = await getLocalStorageSettingFromBackground("hideRelayEmailBadges");
+ options.checkboxTicked = localStorage.getItem("checkbox-ticked");
+ return options;
+}
+
async function getLocalStorageSettingFromBackground(setting) {
// Send request to background determine if to show Relay email field prompt
const backgroundResp = await browser.runtime.sendMessage({
@@ -711,7 +722,7 @@ function patternDetection(selectionArray, socialActionIntent, target) {
}
}
-async function detectFacebookOnPage(target) {
+async function detectFacebookOnPage(target, options) {
if (!checkForTrackers) {
return;
}
@@ -719,15 +730,17 @@ async function detectFacebookOnPage(target) {
patternDetection(PASSIVE_SHARE_PATTERN_DETECTION_SELECTORS, "share-passive", target);
patternDetection(SHARE_PATTERN_DETECTION_SELECTORS, "share", target);
patternDetection(LOGIN_PATTERN_DETECTION_SELECTORS, "login", target);
- const relayAddonEnabled = await getRelayAddonEnabledFromBackground();
- // Check if any FB trackers were blocked, scoped to only the active tab
- const trackersDetectedOnCurrentPage = await checkIfTrackersAreDetectedOnCurrentPage();
-
- // Check if user dismissed the Relay prompt
- const relayAddonPromptDismissed = await getLocalStorageSettingFromBackground("hideRelayEmailBadges");
+ if (!options) {
+ options = await getContentScriptOptions();
+ }
- const checkboxTicked = localStorage.getItem("checkbox-ticked");
+ const {
+ relayAddonEnabled,
+ trackersDetectedOnCurrentPage,
+ relayAddonPromptDismissed,
+ checkboxTicked,
+ } = options;
if (relayAddonPromptDismissed && !relayAddonEnabled && !relayAddonPromptDismissed.hideRelayEmailBadges && trackersDetectedOnCurrentPage && checkboxTicked !== "true") {
patternDetection(EMAIL_PATTERN_DETECTION_SELECTORS, "email", target);
@@ -819,7 +832,7 @@ browser.runtime.onMessage.addListener(message => {
// let callCount = 0;
let contentScriptDelay = 999;
-async function contentScriptInit(resetSwitch, msg, target = document) {
+async function contentScriptInit(resetSwitch, msg, target = document, options = undefined) {
// Second arg is for debugging to see which contentScriptInit fires
// Call count tracks number of times contentScriptInit has been called
// callCount = callCount + 1;
@@ -831,7 +844,7 @@ async function contentScriptInit(resetSwitch, msg, target = document) {
// Resource call is not in FBC/FB Domain and is a FB resource
if (checkForTrackers && msg !== "other-domain") {
- await detectFacebookOnPage(target);
+ await detectFacebookOnPage(target, options);
screenUpdate();
}
}
@@ -874,7 +887,7 @@ async function CheckIfURLShouldBeBlocked() {
await contentScriptInit(false);
// Reinitialize the content script for mutated nodes
- const observer = new MutationObserver((mutations) => {
+ const observer = new MutationObserver(async (mutations) => {
const mutationsSet = new Set(mutations.flatMap(mutation => {
switch (mutation.type) {
case "attributes":
@@ -886,10 +899,11 @@ async function CheckIfURLShouldBeBlocked() {
return [];
}
}));
+ const options = await getContentScriptOptions();
if (mutationsSet.size > MAX_MUTATEDSET_SIZE) {
- contentScriptInit(false, null, document);
+ contentScriptInit(false, null, document, options);
} else {
- mutationsSet.forEach(target => contentScriptInit(false, null, target));
+ mutationsSet.forEach(target => contentScriptInit(false, null, target, options));
}
});I haven't tried these changes on a page where the contains-facebook UI elements are expected to be shown, but on the redash page linked in the bugzilla issue the extensions seems to not having the same kind of performance impact that the current version have (e.g. I collected some profile data using the Firefox Profiler and the extension wasn't "dominating" the profile anymore).
Hopefully these additional details may be already helpful enough to prepare a new version which reintroduce the changes introduced in 2.3.5 (minus the perf impact ;-)), but also feel free to ping me if I can help with some more
willdurand
For background, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1794686 and https://github.com/mozilla/contain-facebook/issues/883
There was a performance issue likely caused by a mutation observer in the version 2.3.5 of this extension. Quoting @rpl:
We should resolve this problem before we attempt to ship a new version.