perf: (rc, stay behavior) uBO scriptlet infinitely spamming MutationObserver attributes events triggered by itself

[seia-soto]

Prerequisites

• I verified that this is not a filter list issue. Report any issues with filter lists or broken website functionality in the uAssets issue tracker.
• This is NOT a YouTube, Facebook or Twitch report. These sites MUST be reported by clicking their respective links.
• This is not a support issue or a question. For support, questions, or help, visit /r/uBlockOrigin.
• I performed a cursory search of the issue tracker to avoid opening a duplicate issue.
• The issue is not present after disabling uBO in the browser.
• I checked the documentation to understand that the issue I am reporting is not normal behavior.

I tried to reproduce the issue when...

• uBO is the only extension.
• uBO uses default lists and settings.
• using a new, unmodified browser profile.

Description

In uBlock Origin scriptlets involving DOM attribute modifications (calling DOMTokenList or Element.prototype.(set|remove)Attribute) may cause an infinite loop issue when it's used with MutationObserver. In this case, I'm handling remove-class scriptlet with stay behavior.

This is happening because the attribute modification event in mutation observer does contain cases the actual value haven't changed. However, uBO scriptlet does call attribute modifier methods even there's no class name in the element. Thus the handle of the MutationObserver in the script is called again and the same process happens infinitely.

Suggested solution

I suggest adding a simple check to avoid calling attribute modification methods meaninglessly. We also need to check if a similar problem happens in other scriptlets involving attribute modification.

    const rmclass = function() {
        timer = undefined;
        try {
            const nodes = document.querySelectorAll(selector);
            for ( const node of nodes ) {
                for ( const token of classTokens ) {
                    if (node.classList.contains(token)) { // Check if a node contains actual class name
                        node.classList.remove(...classTokens);

                        break;
                    }
                }
            }
        } catch(ex) {
        }
        if ( mustStay ) { return; }
        if ( document.readyState !== 'complete' ) { return; }
        observer.disconnect();
    };

Credits to @philipp-classen who found the point of performance issue is happening at MutationObserver.

refs ghostery/adblocker#3913
refs ghostery/ghostery-extension#757 (comment)

A specific URL where the issue occurs.

https://www.reddit.com/

Steps to Reproduce

1. Enable the following filter in My Filters tab.

reddit.com##+js(remove-class, scroll-disabled, body, stay)

2. Open reddit.com and set breakpoint to attribute modification on body element.

In case of Chrome, you can do this by right clicking body element -> Break on -> attribute modifications.

Otherwise, you may put a simple logger in the scriptlet content to see if it actually does infinite loop.

Expected behavior

uBO does stop catching the classes in element if there's nothing to catch.

Actual behavior

1. Once MutationObserver gets MutationRecords, it calls rmclass function.

https://github.com/gorhill/uBlock/blob/5de19ace916d940c19cfbdfd792e001f174b2085/assets/resources/scriptlets.js#L2287

    const mutationHandler = mutations => {
        ...
        timer = self.requestIdleCallback(rmclass, { timeout: 67 });
    };
    const observer = new MutationObserver(mutationHandler);

2. In rmclass function, it calls DOMTokenList.prototype.remove function without checking explicitly.

https://github.com/gorhill/uBlock/blob/5de19ace916d940c19cfbdfd792e001f174b2085/assets/resources/scriptlets.js#L2265

    const rmclass = function() {
        timer = undefined;
        try {
            const nodes = document.querySelectorAll(selector);
            for ( const node of nodes ) {
                node.classList.remove(...classTokens);
            }
        } catch(ex) {
        }
        if ( mustStay ) { return; }
        if ( document.readyState !== 'complete' ) { return; }
        observer.disconnect();
    };

However, the behavior argument orders the observer to stay on the page. In this case, the handler of the living MutationObserver is called again and the infinite process starts.

uBO version

1.56.0

Browser name and version

Version 118.0.5993.117 (Official Build, ungoogled-chromium) (arm64)

Operating System and version

macOS arm 14.4.1 (23E224)

[gorhill]

Thanks for the report/investigation. I didn't expect the browser to generate mutation events when no actual change occurs.

By the way, in uBO lists we wouldn't use a filter such as:

reddit.com##+js(remove-class, scroll-disabled, body, stay)

We would use:

reddit.com##body.scroll-disabled:remove-class(scroll-disabled)

Which doesn't suffer the issue.

---

I see the filter originates from EasyList Notifications, this should be modified to use the second form above. cc @ryanbr

[seia-soto]

Thanks for fast response. I also agree that 'stay' behavior should not happen in common.

[partingscientist]

I saw the fix for ra on gorhill/uBlock@91dfcbe, but not for rc. Just making sure, was it intentionally omitted, or accidentally overlooked?

[u-RraaLL]

It starts here?

[partingscientist]

No, I mean in ra, this line is added, but the analog of it for rc is not, which is what was originally suggested.

[gorhill]

The classes are removed all at once for rc since there is an API to do this.

---

The fix is that only elements with class(es) to remove are looked up: gorhill/uBlock@91dfcbe#diff-30b28769623e5478a0f68519eda037164484cfb444cb5a8e48518fa7bb32e658R2267.

[partingscientist]

I must have misunderstood something previously, apologies.