Opened 2 years ago

Closed 2 years ago

#7056 closed defect (rejected)

ElemHide.getSelectorsForDomain returns duplicates for generic and domain selectors

Reported by: zkhetsuriani Assignee:
Priority: Unknown Milestone:
Module: Core Keywords:
Cc: mjethani, sergz, greiner Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):


I'm testing FilterEngine::GetElementHidingSelectors(const std::string& domain) in libadblockplus, which eventually returns the result of ElemHide.getSelectorsForDomain(domain, ElemHide.ALL_MATCHING, false). The example:

  auto& filterEngine = GetFilterEngine();

  // element hiding selectors

  std::vector<std::string> sels = filterEngine.GetElementHidingSelectors("");
  ASSERT_EQ(sels[0], "#testcase-eh-id");
  ASSERT_EQ(sels[1], "#testcase-eh-id");

As you can see, the result (sels vector) contains "#testcase-eh-id" twice - the one comes from the generic filter and another from the domain filter.

Change History (7)

comment:1 Changed 2 years ago by mjethani

Zura, this is expected behavior. It is not ideal, but it would be extra work to remove any duplicates, and the web extension doesn't care about duplicates. If this is an issue on any other platform, for now, it would be up to the caller to remove any duplicates.

By the way, now the new version of this function is generateStyleSheetForDomain (#6957). It will be even harder now to filter out any duplicate selectors now, even for the implementation itself, because we don't keep the selectors in memory any more but just keep the generated style sheet instead. Let me know if this becomes a problem.

comment:2 Changed 2 years ago by sergz

Zurab, could you please measure how many duplicates are there for a couple of most popular web sites (one can find them in e.g. alexa top)?

comment:3 Changed 2 years ago by greiner

  • Cc greiner added

comment:4 Changed 2 years ago by mjethani

I'm not seeing any duplicates in practice with the default subscriptions.

I added these two lines at the end of the getConditionalSelectors function in lib/elemHide.js (next branch):

  console.log([...getUnconditionalSelectors(), ...selectors].length +
              " selectors");
  console.log(new Set([...getUnconditionalSelectors(), ...selectors]).size +
              " selectors (set)");

Always get the same number.

comment:5 Changed 2 years ago by sergz

Good to know. I think we can close it as "worksforme", any objections?

comment:6 Changed 2 years ago by mjethani

In theory it's possible to get duplicate selectors, and I'm sure it may happen when you subscribe to multiple filter lists. But it's not a "bug" in the sense that it's intended to work this way. If this becomes an issue on a mobile platform, we can reopen this issue.

I am closing this as rejected for now.

Last edited 2 years ago by mjethani (previous) (diff)

comment:7 Changed 2 years ago by mjethani

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