Opened 3 months ago

Closed 2 months ago

Last modified 4 weeks ago

#6296 closed defect (fixed)

Procedural hiding stops working when ...:-abp-has(> ...:-abp-contains(...)) is used

Reported by: Lain_13 Assignee: hfiguiere
Priority: P2 Milestone:
Module: Core Keywords:
Cc: hfiguiere, mjethani, kzar, mapx, sergz Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29676761/

Description

Environment

Google Chrome 63.0.3239.132
Adblock Plus development build 3.0.2.1953

How to reproduce

  1. Add filter litlib.net#?#tr:-abp-has(> td[class^="content"]:-abp-contains(Реклама))
  2. Visit page: https://www.litlib.net/tags

Another example of the same error:

  1. Add filter example.com#?#div:-abp-has(> span:-abp-contains(Advertisment)) from this page: https://adblockplus.org/filter-cheatsheet
  2. Visit http://example.com/ and look in the console for the same error.

Observed behaviour

Empty header with text "РЕКЛАМА:" is visible above the block "ТЕГИ:" in the middle of the page.

Expected behaviour

Block with header "РЕКЛАМА:" (and that header) is hidden.

Error in console

elemHideEmulation.js:243 Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Element': '> *' is not a valid selector.
    at ContainsSelector.getElements (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:956:28)
    at getElements.next (<anonymous>)
    at ContainsSelector.getSelectors (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:948:14)
    at getSelectors.next (<anonymous>)
    at evaluate (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:845:30)
    at evaluate.next (<anonymous>)
    at evaluate (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:851:14)
    at evaluate.next (<anonymous>)
    at HasSelector.getElements (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:914:16)
    at getElements.next (<anonymous>)

As I understand code there lacks special treatments for cases when >, + or ~ are used at the start to indicate that element should be either direct child or neighbor of current element.

include.preload.js source with debug:

ContainsSelector.prototype = {
  requiresHiding: true,

  *getSelectors(prefix, subtree, stylesheet)
  {
    for (let element of this.getElements(prefix, subtree, stylesheet))
      yield [makeSelector(element, ""), subtree];
  },

  *getElements(prefix, subtree, stylesheet) // debug: prefix = "> td[class^="content"]", subtree = tr, stylesheet = Array(298)
  {
    let actualPrefix = (!prefix || incompletePrefixRegexp.test(prefix)) ? // debug: actualPrefix = "> td[class^="content"]", prefix = "> td[class^="content"]"
        prefix + "*" : prefix;
    let elements = subtree.querySelectorAll(actualPrefix); // crash here due to incomplete actualPrefix

    for (let element of elements)
    {
      if (element.textContent.includes(this._text))
        yield element;
      else
        yield null;
    }
  }
};

Change History (16)

comment:1 Changed 3 months ago by mapx

  • Cc hfiguiere mjethani kzar mapx added

comment:2 Changed 3 months ago by hfiguiere

  • Owner set to hfiguiere

This should work and it doesn't. Looking at it.

comment:3 Changed 3 months ago by Lain_13

As I understand > should work with :scope on the left side.

Example (run on the https://www.litlib.net/tags):

let node;
node = document.querySelector('.content_c').closest('table').closest('tr');
// returns: <tr>​…​</tr>​
node.querySelector(':scope > .content_c')
// since .content_c is not a direct child of the :scope the result is 'null'
node = document.querySelector('.content_c').closest('tr');
// this tr is direct parent of .content_c: <tr style=​"display:​ none !important;​">​…​</tr>​
node.querySelector(':scope > .content_c')
// result: <td class=​"content_c">​Реклама: ​</td>​

That won't work with + and ~, though. They are addressing elements outside of the current scope. So, generating selector for the current node might be a necessity. Actually, even node.querySelector() won't work in such case since it's scoped to the node. You'll have to call it at least from a parent node or stright from a document (depending on how you construct an unique selector for the current node).

Last edited 3 months ago by Lain_13 (previous) (diff)

comment:4 follow-up: Changed 3 months ago by hfiguiere

+ and ~ do not make sense inside a :-abp-has() since it is implicitly about children.

comment:5 Changed 3 months ago by hfiguiere

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:6 Changed 3 months ago by hfiguiere

  • Component changed from Unknown to Core

comment:7 in reply to: ↑ 4 Changed 3 months ago by Lain_13

Replying to hfiguiere:
True, I somehow forgot that it's for :-abp-has().

comment:8 Changed 3 months ago by kzar

  • Cc sergz added
  • Priority changed from Unknown to P2
  • Ready set

comment:9 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6296 - Handle relative prefix in :-abp-has()

comment:10 Changed 3 months ago by hfiguiere

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:11 Changed 2 months ago by Lain_13

  • Resolution fixed deleted
  • Status changed from closed to reopened

ABP (1.13.5 and 3.0.2.1974 (dev))
Google Chrome 64.0.3282.167 (Official Build) (64-bit)

Domain: stratege.ru
Filter: stratege.ru#?#div[class$="_box"]:-abp-has(> div[class*="title"]:-abp-contains(Реклама))

Log:

elemHideEmulation.js:243 Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Element': '> div[class*="title"]' is not a valid selector.
    at ContainsSelector.getElements (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:972:28)
    at getElements.next (<anonymous>)
    at ContainsSelector.getSelectors (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:964:14)
    at getSelectors.next (<anonymous>)
    at evaluate (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:861:30)
    at evaluate.next (<anonymous>)
    at evaluate (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:867:14)
    at evaluate.next (<anonymous>)
    at HasSelector.getElements (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/include.preload.js:930:16)
    at getElements.next (<anonymous>)

A commit referencing this issue has landed:
​Issue 6296 - Handle relative prefix in :-abp-has()

As I understand code related to this fix were moved to HasSelector.prototype.getElements, while the issue actually happens in the ContainsSelector.prototype.getElements.

comment:12 Changed 2 months ago by hfiguiere

The thing is that adblockpluscore hasn't been updated in that development build of Adblock Plus so the fix you are expecting isn't even there.

Last edited 2 months ago by hfiguiere (previous) (diff)

comment:13 Changed 2 months ago by Lain_13

Strange, I thought it's already there. Now I see it's not even in the changelog of dev build. Sorry about that. Please close this issue again.

comment:14 Changed 2 months ago by hfiguiere

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:15 Changed 8 weeks ago by hfiguiere

I have filed issue #6412 to update the dependencies and have this change brought over to the WebExtension.

comment:16 Changed 4 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed.

A version of this 127.0.0.1#?#div:-abp-has(> span:-abp-contains(Advertisment)) against a test page with the element worked as expected. I couldn't reproduce the example using litlib.net above.

ABP 3.0.2.1983
Firefox 51 / 58 / Windows 10
Chrome 49 / 65 / Windows 7
Opera 36 / 51 / Windows 7

Note: See TracTickets for help on using tickets.