Opened 3 months ago

Closed 8 weeks ago

Last modified 4 weeks ago

#5438 closed change (fixed)

Listen to DOM changes to eventually reapply filters.

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

https://codereview.adblockplus.org/29494577

Description (last modified by trev)

Background

Currently, we only apply element hiding emulation filters once. Subsequent DOM modifications are mostly ignored, only new stylesheets are processed.

What to change

We need to listen to DOM modifications and reapply the filters as necessary. At the same time, we should avoid doing too much work. There seem to be four scenarios to consider here:

  1. A new subtree is added to the document: this is the obvious case, we have to apply all DOM-dependent rules on that subtree, just in case any of the elements there are matched. Here, "DOM-dependent rules" means all rules that aren't applied via selectors. In terms of our code that's pattern.selectors.some(s => s.preferHideWithSelector) && !pattern.selectors.some(s => s.requiresHiding).
  2. Some DOM modification (can be a new subtree but also attribute modifications for example) has side-effects on an -abp-has() pseudo of a rule. For example, -abp-has(.foo) wasn't matching for a node before but now a class="foo" attribute has been added on its child. In order to detect which nodes might be affected by a DOM change we need to keep track of all nodes we evaluated -abp-has() pseudos for. We can use a WeakMap for that - for each element that we evaluate this._innerSelectors on we store the reference to the remaining chain. So if the chain is [PlainSelector(#foo), HasSelector(.bar), PlainSelector(+ div)] then we should use the map to associate [HasSelector(.bar), PlainSelector(+ div)] with the element #foo. Note that we might have to associate multiple chains with the same element. For any DOM modifications later, we have to go through the parents of the modified element and check whether we find them in the map. If we do we need to re-evaluate the associated chains on these elements.
  3. Some DOM modification has side-effects on a -abp-properties() pseudo of a DOM-dependent rule. This could happen if -abp-properties() and -abp-has() are used in the same rule, e.g. #foo:-abp-has(.bar) + -abp-properties(background-color: red). If the class attribute for the next sibling of the #foo element changes and with it the background color, this rule might apply even though it didn't before. I think that we want to ignore this scenario for now.
  4. Some DOM modification has side-effects on a plain selector rule. For example, we have the selector #foo + .bar + .bas processed via element hiding emulation. If the class attribute for the next sibling of the #foo element changes to bar this rule might match some element even though it didn't before. I think that we want to ignore this scenario, with the support for plain selectors via element hiding emulation being a temporary hack that we only implemented because it was so easy.

Note

This require the changes for issue #5395 as we'll plug into the same logic.

Change History (9)

comment:1 Changed 3 months ago by hfiguiere

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

comment:2 Changed 3 months ago by trev

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

comment:3 Changed 3 months ago by hfiguiere

  • Blocking 5000 added

comment:4 Changed 3 months ago by arthur

  • Cc arthur added

comment:5 Changed 3 months ago by trev

Felix and me discussed the approach here today. The trouble is, the approach I outlined so far is fairly complicated while still leaving lots of holes. In the end, we have to face the fact that with our set of rules a change to a DOM element might affect any other DOM element anywhere in the document. We cannot limit the work in a meaningful way.

So now the thought is: what if we reapply all filters on any DOM modification but make sure it happens in a non-blocking way? In detail, this should mean:

  • Make sure that all generators yield a value occasionally. Currently, this is only an issue for HasSelector.getElements() and ContainsSelector.getElements() which might do significant work without yielding anything. So these should yield the special null value from their inner loop (only if the loop iteration doesn't yield an element already). In HasSelector.getElements() we also need to add yield null to the outer loop because it won't necessarily go into the inner loop.
  • We need to make sure that this special null value gets passed on all the way through. So makeSelector() should return null for node == null. evaluate() needs to yield null for selector == null. HasSelector.getElements() needs to ignore null selectors in the inner loop but still yield for them.
  • ElemHideEmulation.addSelectors() has to be made asynchronous. Whenever a result is yielded by evaluate(), it should check whether it already spent more than 50 ms processing. If it did, it should store its state (remaining patterns to process, current generator) and call window.setTimeout(..., 0) to resume processing. I guess that resuming looping through generator values via for..in is possible, but we could always resort to manual looping via generator.next().
  • Any DOM modification should trigger ElemHideEmulation.addSelectors() via an unfiltered mutation observer. We have to implement rate limiting here:
    • If processing is already ongoing, set a flag to schedule a new run. Once current processing finishes, it should schedule a new run after MIN_INVOCATION_INTERVAL.
    • If a new processing run is already scheduled, don't do anything.
    • If last processing run finished less than MIN_INVOCATION_INTERVAL ago, schedule a new run after an appropriate interval.
    • If last processing run finished more than MIN_INVOCATION_INTERVAL ago, do processing immediately.

Hubert, what do you think?

comment:6 Changed 2 months ago by hfiguiere

Updated the patch.

comment:7 Changed 8 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5438 - Observer DOM changes to reapply filters.

comment:8 Changed 8 weeks ago by hfiguiere

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

comment:9 Changed 4 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Hiding elements that match after DOM changes works in general. I've made a note to create test cases for the examples described above.

ABP 1.13.3.1838
Chrome 49 / 61 / Windows 10
Opera 39 / 47 / Windows 10

Note: See TracTickets for help on using tickets.