Opened 11 months ago

Closed 10 months ago

Last modified 9 months 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 11 months ago by mapx

  • Cc hfiguiere mjethani kzar mapx added

comment:2 Changed 11 months ago by hfiguiere

  • Owner set to hfiguiere

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

comment:3 Changed 11 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 11 months ago by Lain_13 (previous) (diff)

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

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

comment:5 Changed 11 months ago by hfiguiere

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

comment:6 Changed 11 months ago by hfiguiere

  • Component changed from Unknown to Core

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

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

comment:8 Changed 11 months ago by kzar

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

comment:9 Changed 10 months ago by abpbot

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

comment:10 Changed 10 months ago by hfiguiere

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

comment:11 Changed 10 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 10 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 10 months ago by hfiguiere (previous) (diff)

comment:13 Changed 10 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 10 months ago by hfiguiere

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

comment:15 Changed 10 months ago by hfiguiere

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

comment:16 Changed 9 months 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.