Opened 7 months ago

Closed 2 days ago

#5174 closed change (fixed)

[emscripten] Allow brackets { and } in element hiding filters

Reported by: trev Assignee: hfiguiere
Priority: P2 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: #4122 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29580558/

Description

Background

We implemented #4684 on master branch but not in Emscripten.

What to change

  • Allow { and } in the selector part of element hiding filters.
  • Change ElemHideBase::GetSelector to return an OwnedString.
  • Make ElemHideBase::GetSelector scan the selector before returning - only return unchanged if no braces are found.
  • If braces are found, replace { by \x7B and } by \x7D respectively (note the trailing space, it is necessary).
  • Add unit test to verify correctness, similar to testElemHideRulesWithBraces test on master branch.

Change History (10)

comment:1 Changed 5 weeks ago by hfiguiere

  • Owner set to hfiguiere

comment:2 Changed 5 weeks ago by hfiguiere

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

comment:3 in reply to: ↑ description ; follow-up: Changed 5 weeks ago by sergz

replace { by \x7B and } by \x7D respectively (note the trailing space, it is necessary).

Why are they necessary?

comment:4 in reply to: ↑ 3 Changed 5 weeks ago by sergz

Replying to sergz:

replace { by \x7B and } by \x7D respectively (note the trailing space, it is necessary).

Why are they necessary?

Despite I have found https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.js#newcode831 it's still not clear because it's expected that only two next bytes should be treated after \x.

comment:5 Changed 5 weeks ago by trev

You mean, why trailing spaces are necessary? That's CSS syntax, it allows up to six hex digit to denote a character - or whatever number of digits plus whitespace. So if we changed {12 into \x7B12 the CSS parser would consider all that a single character. We could use \x00007B but then the parser would eat up any spaces following the bracket. These aren't theoretical scenarios, we actually got all of these bugs reported for Element Hiding Helper.

comment:6 Changed 5 weeks ago by hfiguiere

Reference:

https://www.w3.org/TR/css-syntax-3/#consume-an-escaped-code-point

The spec is pretty clear about it.

comment:7 follow-up: Changed 5 weeks ago by trev

Oh my, the correct replacement is \7B , without the x. This has been implemented incorrectly on the master branch as well.

comment:8 in reply to: ↑ 7 Changed 5 weeks ago by sergz

Replying to trev:

Oh my, the correct replacement is \7B , without the x. This has been implemented incorrectly on the master branch as well.

Without the x no questions.

comment:9 Changed 2 days ago by abpbot

A commit referencing this issue has landed:
Issue 5174 - Allow '{' and '}' in selectors

comment:10 Changed 2 days ago by hfiguiere

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.