script:inject with only negated domains cause cosmetic filtering engine to crash

[jspenguin2017]

Describe the issue

When a script:inject filter has only negated domains, cosmetic filter parser does not properly discard it.

One or more specific URLs where the issue occurs

http://example.com/

Screenshot in which the issue can be seen

Steps for anyone to reproduce the issue

1. Add ~example.com##script:inject(abort-on-proerty-read.js, test) to My filters
2. Go to http://example.com/ and open the console

Your settings

All default

• OS/version: Win10 15063
• Browser/version: Chrome 63
• uBlock Origin version: 1.14.22

Your filter lists

All default

Your custom filters (if any)

See steps to reproduce

Additional details

Although the crash is caused by an invalid filter, uBO is suppose to discard bad filters.

At around line 365 of cosmetic-filtering.js:

    if (
        this.hostnames.length === 0 &&
        this.unhide === 0 &&
        this.reNeedHostname.test(this.suffix)
    ) {
        this.invalid = true;
        return this;
    }

This is the logic which mark expensive filters as invalid if they are generic. However, it does not handle the case where all domains are negated.

Later, at around line 1175 of cosmetic-filtering.js:

    var applyGlobally = true;
    var hostname;
    while ( i-- ) {
        hostname = hostnames[i];
        if ( hostname.startsWith('~') === false ) {
            applyGlobally = false;
        }
        this.compileHostnameSelector(hostname, parsed, writer);
    }
    if ( applyGlobally ) {
        this.compileGenericSelector(parsed, writer);
    }

When there are only negated domains, applyGlobally will be true and it will use FilterContainer.prototype.compileGenericSelector to compile the filter, which does not expect expensive filters. The compiled data is invalid / corrupted which later cause error in cosmetic filtering engine.

Inside FilterContainer.prototype.compileGenericHideSelector, I see the comment:

// TODO: Detect and error on procedural cosmetic filters.

I'm not sure if you have plan on adding logic to handle it there, but for scriptlet injection, execution won't reach that point (the place where the comment is).

I think the filter should be re-checked for validity inside the if-statement for applyGlobally.

[gorhill]

I have been refactoring scriptlet injection (#3069), and I currently can't reproduce this specific error with the new code, but then I am reminded I need to handle the negated hostname case for filters which are not allowed to apply everywhere.

[jspenguin2017]

That's not quite right, the script:inject(...) part is parsed as an element selector in FilterContainer.prototype.compileGenericSelector, it never hit the scriptlet injection engine.

Firefox swallows lots of errors, so when you insert invalid CSS, on Firefox it will just silently fail. This is the same for many other (chrome / browser) APIs.

[gorhill]

That's not quite right

I said I refactored -- how would you know how the not-yet-committed refactored code looks like to be able to make a "not quite right" observation? Scriptlet injection is no longer handled by cosmeticFilteringEngine, it's in its own scriptletFilteringEngine.

[gorhill]

Fixed with a9f68fe.