Opened 3 months ago

Closed 3 months ago

Last modified 2 months 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 3 months ago by arthur

  • Description modified (diff)

comment:2 Changed 3 months ago by arthur

  • Description modified (diff)

comment:3 Changed 3 months 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 3 months ago by kzar

  • Description modified (diff)

comment:5 Changed 3 months 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 3 months 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 3 months 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 3 months 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 3 months ago by kzar

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

comment:10 Changed 3 months ago by arthur

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

comment:11 Changed 3 months 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 3 months ago by BrentM

  • Cc weissmar added

comment:13 Changed 2 months 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.