Opened 13 months ago

Closed 9 months ago

Last modified 8 months ago

#4599 closed defect (fixed)

Third party filters blocking first party requests

Reported by: scuturic Assignee: kzar
Priority: P1 Milestone: Adblock-Plus-1.13-for-Chrome-Opera
Module: Unknown Keywords:
Cc: sebastian, kzar, mapx, trev, offir, esanchez, rhana@…, arthur Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29362605/

Description (last modified by kzar)

Environment

Win 8.1
Chrome 54.0.2840.87
Adblock Plus 1.12.4 with only the filter ||apptap.com^$third-party and no subscriptions.

How to reproduce

  1. Create a new Chrome tab
  2. Type the URL http://www.apptap.com/ into the URL bar.
  3. Hit the Enter key.

OR

  1. Browse to https://static.kzar.co.uk/4599-link-test/
  2. Click on the link.

Observed behaviour

The page layout is often broken since the page's stylesheet was blocked. It doesn't happen every time but it does quite often. If you then refresh the page the stylesheet will not be blocked.

Expected behaviour

The page should be displayed correctly, the stylesheet shouldn't be blocked.

Notes

Change History (31)

comment:1 Changed 13 months ago by arthur

  • Cc sebastian kzar added

comment:2 Changed 13 months ago by mapx

  • Cc mapx added

comment:3 Changed 13 months ago by mapx

  • Cc trev added

comment:4 Changed 13 months ago by mapx

it seems a lot of sites have the same wrong behaviour:
http://www.appnext.com/
http://www.apptap.com/

How to reproduce it:

open a new tab, type www.apptap.com:

  • the css file will be blocked
  • F5 refresh ==> the page works fine

If you whitelist the site, repeat the steps above ==> the css file blocked again !

Could be a regression while fixing:
1649: 2016-08-31 17:11 +0200 Wladimir Palant
Issue #4386 - Fixed determining document domain, particularly after being redirected
?

see also in forum:
https://adblockplus.org/forum/viewtopic.php?f=10&t=49528

If you add
@@||apptap.com^$stylesheet

the issue is gone.

There is only a third-party filter in easylist:
||apptap.com^$third-party

Last edited 13 months ago by mapx (previous) (diff)

comment:5 Changed 13 months ago by mapx

  • Summary changed from CSS blocked on vsexshop.ru in Chrome to CSS blocked on all sites in easylist with ||example.com^$third-party filters

comment:6 Changed 13 months ago by mapx

  • Cc offir added

comment:7 Changed 13 months ago by mapx

  • Summary changed from CSS blocked on all sites in easylist with ||example.com^$third-party filters to CSS blocked on all sites in easylist with $third-party filters

comment:8 Changed 13 months ago by trev

I'd rather expect #4483 to be the cause.

comment:9 Changed 13 months ago by kzar

  • Owner set to kzar

comment:10 Changed 13 months ago by kzar

  • Description modified (diff)
  • Priority changed from Unknown to P1
  • Ready set
  • Summary changed from CSS blocked on all sites in easylist with $third-party filters to Third party filters blocking first party requests for pre-rendered pages

I can reproduce this too with Chrome 54 on Linux. I agree it's likely to be a regression, going to try and track down what caused this now.

comment:11 Changed 13 months ago by kzar

  • Description modified (diff)

Like Wladimir I expected the recent changes for #4483 to be to blame but it seems Mapx was right and that the changes for #4386 caused this. I can reproduce the problem with the Issue 4386 - Fixed determining document domain, particularly after being redirected revision but not before.

comment:12 Changed 13 months ago by mapx

yep, I was guessing is 4386 because the symptoms are the same as in the original 4386
"If you whitelist the site, repeat the steps above ==> the css file blocked again"

comment:13 Changed 13 months ago by kzar

  • Description modified (diff)

comment:14 Changed 13 months ago by kzar

Edit: I was mistaken, since as Sebastian pointed out in the code review Chrome gives pre-rendered pages a new tab id. As for the $sitekey stuff, I've opened #4632 for that.

Unless I'm mistaken it's basically the same problem as #4483.

If a page makes a web request before its onCommitted event has fired the request will be blocked / whitelisted based upon the old page structure. So if the URL for the previous page was of a different domain the request will be considered third party, or if the new page has a sitekey we won't know to take it into account.

Thing is the old behaviour was no better, arguably worse. The onBeforeNavigate event we used wasn't fired after redirections, so the page structure was not updated at all for those. Also it was fired for pre-rendered pages which meant that we effectively assumed that pre-rendered pages would always be navigated to. So we can't simply revert #4386 and I think it's a little unfair to consider this issue a regression caused by that change.

Ultimately I think we need to rethink the page structure logic so that pre-rendered pages can be accounted for properly. While we're at it I think adding unit tests for how the page structure is maintained based upon navigation events would be a pretty good idea. I don't think this is the right time to do that however since in the near future we plan to remove some of the Page abstractions (e.g. #4580) and other ext code now that Safari support has been dropped. I think it would make more sense to wait until after that work has been done.

So what can we do for now? Well how about we update the page structure both on onBeforeNavigate and onCommitted. We could remove the workaround for #4483 while at it and look at maintaining the page structure properly after the ext code has been removed. This would mostly be correct and not require major changes.

(Updating the page structure for pre-rendered pages is incorrect but perhaps that doesn't matter too much. One pre-rendered page will replace another until the user eventually has finished typing the URL. The latest guess should always be handled correctly. The only problem would be if the user stopped typing the URL and then continued to use the existing page without navigating at all. Note this was the case before #4483 landed so apparently not too much of a problem.)

Last edited 12 months ago by kzar (previous) (diff)

comment:15 Changed 13 months ago by kzar

(Dang, looks like I was at least partially mistaken since the $sitekey whitelisting for requests directly after a redirect broke when I took the #4483 workaround out. I'll see if the approach otherwise works now.)

Last edited 12 months ago by kzar (previous) (diff)

comment:16 Changed 13 months ago by kzar

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

comment:17 Changed 12 months ago by esanchez

  • Cc esanchez added

comment:18 Changed 12 months ago by kzar

  • Description modified (diff)
  • Summary changed from Third party filters blocking first party requests for pre-rendered pages to Third party filters blocking first party requests

(Turns out this is not only for pre-rendered pages.)

comment:19 Changed 12 months ago by kzar

(I filed Chromium issue 665843 for the underlying problem that pages and frames can make requests before their onCommitted event has fired.)

comment:20 Changed 12 months ago by kzar

  • Review URL(s) modified (diff)

comment:21 Changed 12 months ago by kzar

  • Cc rhana@… added

comment:22 Changed 12 months ago by arthur

  • Cc arthur added

comment:25 Changed 11 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next

comment:26 Changed 10 months ago by arthur

Is this the same issue?

Copy this URL https://advertise.isleofskye.com/, paste it into the address bar and press enter. Some resources are being blocked while they shouldn't. It's fine if you reload the page.

comment:27 Changed 10 months ago by kzar

Yes, that sounds like the same issue.

comment:28 Changed 9 months ago by 8ball

I have this issue on ABP for CHROME and OPERA - upon first load of my websites homepage my adsense advert is being blocked even when disabled on the site, if i refresh the page it displays...all other ads on website display fine.. just the homepage one doesnt

comment:29 Changed 9 months ago by kzar

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

comment:30 Changed 9 months ago by kzar

A commit referencing this issue has landed:
Issue 4598,4599,4647,4804 - Work around onCommitted flaw

comment:31 Changed 8 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed.

ABP 1.12.4.1739
Chrome 49 / 56 / Windows 10
Chrome 56 / OS X 10.12
Chrome 56 / Ubuntu 16.04
Opera 37 / 41 / Windows 7
Safari 10 / OS X 10.12

Last edited 8 months ago by Ross (previous) (diff)
Note: See TracTickets for help on using tickets.