Opened 3 months ago

Closed 4 weeks ago

Last modified 4 weeks 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: Unknown Verified working: no
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 (12)

comment:1 Changed 3 months ago by sebastian

  • Cc sebastian added

comment:2 Changed 3 months ago by mjethani

  • Review URL(s) modified (diff)

comment:3 Changed 3 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 3 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 3 months ago by sebastian (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 3 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 5 weeks ago by mjethani

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

comment:7 Changed 4 weeks ago by abpbot

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

comment:8 Changed 4 weeks ago by mjethani

  • Description modified (diff)
  • Ready set

comment:9 Changed 4 weeks ago by mjethani

  • Description modified (diff)

comment:10 Changed 4 weeks ago by mjethani

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

comment:11 Changed 4 weeks ago by mjethani

  • Cc greiner added

comment:12 Changed 4 weeks ago by greiner

  • Blocking 7417 added
Note: See TracTickets for help on using tickets.