Opened 10 months ago

Closed 9 months ago

Last modified 5 months ago

#7260 closed change (fixed)

Internalize third-party request check in matcher

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

https://codereview.adblockplus.org/29998564/
https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/36

Description (last modified by mjethani)

Background

We moved the third-party request checking function into adblockpluscore recently (#7232). But there's no reason for external code to call this function directly: the matcher can check if a request is third-party based on the URL and the document's domain. Because most requests hit the cache anyway, of which we recently increased the capacity (#7254), the check wouldn't even have to be done in most cases.

What to change

See patch.

Integration notes

There are two changes in the APIs of Matcher and CombinedMatcher (both from lib/matcher.js):

  1. Where a method formerly expected the thirdParty argument, now it does not
  2. Where a method formerly expected a location argument (type string), it now expects a url argument (type URL or URLInfo (as returned by parseURL() in lib/url.js))

The following methods are affected:

  • Matcher.matchesAny()
  • CombinedMatcher.matchesAny()
  • CombinedMatcher.search()
  • CombinedMatcher.isWhitelisted()

This will require at least the following changes to other repos:

adblockpluschrome files:

  • lib/csp.js
  • lib/devtools.js
  • lib/filterComposer.js
  • lib/popupBlocker.js
  • lib/requestBlocker.js
  • lib/whitelisting.js

adblockplusui files:

  • background.js
  • messageResponder.js

Hints for testes

After the related to changes to adblockpluschrome, check that the $third-party filter option is working. If there's a filter like /foo/*$third-party then it should block a request to https://example.com/foo/bar/ only if the request is coming from a document not loaded from example.com. If the flag is flipped instead so that the filter looks like /foo/*$~third-party, then it should not block the request unless it is coming from example.com.

Change History (13)

comment:1 Changed 10 months ago by sebastian

  • Cc sebastian added

comment:2 Changed 10 months ago by mjethani

  • Review URL(s) modified (diff)

comment:3 Changed 10 months ago by mjethani

  • Priority changed from P2 to P3

I am not seeing any performance benefit after the changes. In fact, maybe it even hurts the performance. In any case, this is no longer about performance. As such I am setting this to a lower priority for now. Let's look into this for a future release.

comment:4 follow-up: Changed 10 months ago by sebastian

Even if there is no performance benefit per-se, I think from an API point of view it doesn't make any sense to provide an isThirdParty() function just so that the calling code can pass the result into the matcher. The simplifications in adblockpluschrome seem well worth it. After all, this was the reason we moved the public suffix list and related logic into core in the first place.

Last edited 10 months ago by sebastian (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 10 months ago by mjethani

Replying to sebastian:

Even if there is no performance benefit per-se, I think from an API point of view it doesn't make any sense to provide an isThirdParty() function just so that the calling code can pass the result into the matcher.

I agree.

comment:6 Changed 9 months ago by mjethani

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

comment:7 Changed 9 months ago by abpbot

A commit referencing this issue has landed:
Issue 7260 - Internalize third-party request check in matcher

comment:8 Changed 9 months ago by mjethani

  • Description modified (diff)
  • Ready set

comment:9 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:10 Changed 9 months ago by mjethani

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

comment:11 Changed 9 months ago by mjethani

  • Cc greiner added

comment:12 Changed 9 months ago by greiner

  • Blocking 7417 added

comment:13 Changed 5 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

ABP 0.9.15.2340
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2

Note: See TracTickets for help on using tickets.