Mutation observer causes severe performance issues

[willdurand]

For background, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1794686 and #883

There was a performance issue likely caused by a mutation observer in the version 2.3.5 of this extension. Quoting @rpl:

I see an mutation observer that may have been introduced on the content script side (6a79742) which may be the reason why detectFacebookOnPage (which at a first glance seems to dominate the profile attached to bug 1794686) is called more often if certain webpages are being loaded in a tab.

We should resolve this problem before we attempt to ship a new version.

[rpl]

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 #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 document

To be fair, I'd guess that the options that are being retrieved through the extension messaging API should not be changing that often:

contain-facebook/src/content_script.js

Lines 717 to 725 in 1cbf353

	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");
	const checkboxTicked = localStorage.getItem("checkbox-ticked");

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

[rpl]

I couple more side notes for a couple more things I noticed while digging into the perf issue and that I forgot to mention in my previous comment (even if not strictly needed as part of fixing the specific issue triggered by the mutation observer):

• as part of the current version of detectFacebookOnPage there is a call to localStorage.getItem, which is synchronous and so the less we call it the better:

  • contain-facebook/src/content_script.js

    Line 725 in 1cbf353

    const checkboxTicked = localStorage.getItem("checkbox-ticked");

• escapeKeyListener seems to be adding a keydown listener on the document, and it is called at the end of detectFacebookOnPage, I guess that may only be needed to be called once, instead of adding a new keydown listener on document.body each time detectFacebookOnPage is called for a mutated DOM element:

  • contain-facebook/src/content_script.js

    Line 732 in 1cbf353

    escapeKeyListener();

  • contain-facebook/src/content_script.js

    Lines 766 to 772 in 1cbf353

    	function escapeKeyListener() {
    	document.body.addEventListener("keydown", function (e) {
    	if (e.key === "Escape" && document.querySelector(".fbc-wrapper")) {
    	closeIframe();
    	}
    	});
    	}

[willdurand]

@maxxcrawford I see a PR has been merged for the initial problem but @rpl also mentioned other things to explore and improve. Would you be able to file new follow-up issues? And could you close this issue when that is done? Thanks