has/if filters rejected if contains > char after recent changes.

[kasper93]

Describe the issue

Well, just look at the log. Latest version of uBO rejects valid procedural filters. I think it worked not so long ago, so probably the "normalization" changes broke this.

Two examples from uBlock filters, there are many more that are rejected, I'm just posting this for reference.

Cosmetic filtering – invalid filter: section[class]:has(> a[rel][target] > span > img)
Cosmetic filtering – invalid filter: h5:has(> a[href^="javascript:"])

Also I noticed this one, which should be if instead of has

Cosmetic filtering – invalid filter: div[id^="hyperfeed_story_id_"]:has(a:has-text(Sponsored))

I wish I noticed this before you released stable. Bad luck.

Your settings

• Browser/version: Firefox Nightly
• uBlock Origin version: 1.14.12

[gorhill]

This happens only with Nightly, not with Firefox 56, neither with Chromium 61. I test mostly only with these two last.

So either this is a new issue in Nightly, or either the :scope pseudo selector is no longer supported.

[gorhill]

https://developer.mozilla.org/en-US/docs/Web/CSS/:scope:

This feature has been removed from the Web standards. Though some browsers may still support it, it is in the process of being dropped.

[gorhill]

Does it still work in FF57?

[gorhill]

Ok, in Nightly, Element.matches does not support :scope. However, Element.querySelector does support :scope.

Now I wonder if this is by design or a bug in the browser. uBO uses Element.matches to find out whether a CSS selector is valid because it is more efficient than using Element.querySelector.

[kasper93]

See the note. Seems to be by design:

Note: In JavaScript, when used with Element.querySelector, Element.querySelectorAll, or Element.closest, :scope refers to the element on which these methods are invoked. For example, document.body.querySelector(":scope") returns the body element. While support for :scope has been removed for CSS, this use of :scope continues to be supported.

Indeed it is Nightly only issue and seems to be recent, probably not more than few days.

[kasper93]

Now Element.matches uses Stylo. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1404897 (merged 4 days ago). And :scope is not supported.

But I think this is unwanted side effect of this change. While documentation doesn't explicitly say about Element.matches it shouldn't differ from Element.querySelector in regards to supported selectors.

[kasper93]

It is really late, but I filled bug report about this issue https://bugzilla.mozilla.org/show_bug.cgi?id=1406817

[gorhill]

There are really two distinct issues here, I did not address the second one:

Also I noticed this one, which should be if instead of has

I just reproduced this one.

[gorhill]

Actually, never mind, the second issue is a filter one, I mistakenly used :has instead of :if when I created the filters: uBlockOrigin/uAssets@0854b25. These filters are really invalid.

[gwarser]

@gorhill may this scope thing cause performance problems in forEachNode, contentscript.js:517?

I have example where this function timing increases from 400ms to 8 seconds when 250 filters like this:

##object[width="690"][height="85"]
##embed[width="120"][height="600"]

extracted from "Adguard Base Filters" are added to uBlock.

Should I create new issue, or wait for next Nightly build?

[gorhill]

@gwarser I have been working on refactoring cosmetic filtering as per #2984 as much as possible lately.

That code you point out no longer exists in the refactored code. I am currently benchmarking to find out whether it's worth just injecting all the highly generic filters (in which the selectors you quote belong) unconditionally. Either way, that slow forEachNode no longer exists for Chromium or Firefox.

Currently I still need to re-create back ability to log cosmetic filters following refactoring, and also the DOM inspector (this one is a real pain, I might just commit the changes before fixing -- the DOM inspector is a perfect example of how adding a feature can cause pain in the future).

[gorhill]

Also, to answer your specific question, the code at line 517 of contentscript.js does not use :scope. The :scope is only (maybe) used in the procedural selectors :has and :if.

[gorhill]

I have example where this function timing increases from 400ms to 8 seconds when 250 filters like this

By the way, you have a good URL which I could use for benchmarking purpose? What is the URL you had the issue?

[gwarser]

In this example it is NSFW http://motherless.com/ (images can be disabled) - carousel is making page unusable (script time-out warnings)

On my phone https://discourse.mozilla.org/ is hard, but this may be different code path (script timeout warning on refresh).

One more thing - I see this mentioned issue only on Nightly. Cannot reproduce on beta or stable.

[kasper93]

Nice example, the page is completely unusable at this moment with uBO.

By the way, the :scope issue has been fixed and it is working again in latest nightly build.

[kasper93]

@gwarser Here is bugreport for performance regression that we current;y have https://bugzilla.mozilla.org/show_bug.cgi?id=1405899 basically element.matches is very slow with Stylo.

@gorhill: For any benchmark you are doing now, please keep in mind that current Firefox Nightly have quite slow element.matches.

[gwarser]

I just do another test - refreshing https://discourse.mozilla.org/,
UBO default settings + "--- Banner sizes ---" sections from Adguard Base Filters copied into My Filters tab.

With layout.css.servo.enabled;false
processHighHighGeneric takes 435ms - https://perfht.ml/2ycJLJj
with true - 5105ms - https://perfht.ml/2ybW2NQ

[kasper93]

Could you please reply on bugzilla? We can continue discussion there.

[gorhill]

For any benchmark you are doing now, please keep in mind that current Firefox Nightly have quite slow element.matches.

That's the thing, the new code uses only user stylesheets on Firefox, no more matches or querySelectorAll.

[gorhill]

The Element.matches code has been removed now from cosmetic filtering on Firefox since #2984 was committed.

However, something to keep in mind while the issue is being worked by Firefox devs is that Element.matches is still used if the logger is opened, the content script which scans the DOM to find elements matching the injected filterset needs to use Element.matches to efficiently handle DOM changes (scan only the changes rather than the whole DOM), there is no way around this.

Edit: never mind, issue is fixed now in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1407864.