Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#6808 closed defect (fixed)

Element hiding filter still applied on domain whitelisted by $elemhide

Reported by: arthur Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-3.3-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, kzar, scuturic, mbollien, rscott, Ross, BrentM, weissmar Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29838555/

Description (last modified by kzar)

Environment

Windows 10 Pro
Chrome 68.0.3440.75 (Official Build) (64-bit)
ABP 3.2.0.2093, also happening with 3.2
EasyList
AA

How to reproduce

  1. Open the ABP tab in the dev tools panel
  2. Go to https://www.olx.pl/motoryzacja/samochody/
  3. Change the drop down item on the left to show only "ELEMHIDE" items

Observed behaviour

You see @@||olx.pl^$elemhide from AA being applied but also ##iframe[id^="google_ads_frame"] from EasyList

Expected behaviour

Element hiding should be whitelisted completely.

Notes

  • There is no domain for the document for that hiding filter. Also, if you add olx.pl#@#iframe[id^="google_ads_frame"], it works like on every 2nd refresh.
  • This is has been a problem since commit 7781a9fc138e.

Hints for testers

  • Ensure that element hiding whitelisting works for old.pl as described above.
  • Since this change tweaks how we keep track of frames and pages it's important to test for regressions with ad blocking, element hiding and whitelisting in general.

Change History (13)

comment:1 Changed 2 years ago by arthur

  • Description modified (diff)

comment:2 Changed 2 years ago by arthur

  • Description modified (diff)

comment:3 Changed 2 years ago by kzar

  • Owner set to kzar
  • Priority changed from Unknown to P2

I can reproduce, but this definitely isn't a regression since the last release. Investigating now.

comment:4 Changed 2 years ago by kzar

  • Description modified (diff)

comment:5 Changed 2 years ago by kzar

updatePageFrameStructure is not being called for the parent frame of the one we care about, which means that the parent frame is unknown. url.extractHostFromFrame doesn't return the hostname, whitelisting.checkWhitelisted isn't finding the exception filter and ElemHide.getSelectorsForDomain is being called for the generic domain.

It's hard to see how exactly this troublesome parent frame is being created, since the website creates a shit-load of frames. However, I suspect this is a limitation of Chrome, since even browser.webNavigation.getAllFrames does not list this parent frame (despite the fact it does list several frames with that parentFrameId!)

So I guess we might have to fall back to the frame's tab details in the case that chain of frame parents gets broken like this.

comment:6 Changed 2 years ago by kzar

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

So I guess we might have to fall back to the frame's tab details in the case that chain of frame parents gets broken like this.

Yea, that seems to work.

comment:7 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6808 - Default to top frame of page if parent can't be found

comment:8 Changed 2 years ago by kzar

  • Cc rscott Ross added
  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:9 Changed 2 years ago by kzar

Looks like the workaround has made the next release against the odds Arthur! (Assuming that it passes QA.)

comment:10 Changed 2 years ago by arthur

Thanks again, really appreciate it! :) Just tried it out and it's working for me.

comment:11 Changed 2 years ago by kzar

  • Cc BrentM added

Hey Brent, I just was notified that this issue also applies to AdBlock. You might want to update the adblockpluschrome dependency when you get a chance.

comment:12 Changed 2 years ago by BrentM

  • Cc weissmar added

comment:13 Changed 2 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Looks fixed. Element hiding, whitelisting etc. also look fine after the change.

ABP 3.2.2103
Chrome 67 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51 / Windows 10

Note: See TracTickets for help on using tickets.