Opened on 02/05/2019 at 05:29:16 AM
Closed on 03/28/2019 at 10:09:45 AM
Last modified on 07/25/2019 at 08:31:28 PM
#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/ |
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):
- Where a method formerly expected the thirdParty argument, now it does not
- 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.
Attachments (0)
Change History (13)
comment:1 Changed on 02/05/2019 at 05:40:20 AM by sebastian
- Cc sebastian added
comment:3 Changed on 02/05/2019 at 09:08:48 AM by mjethani
- Priority changed from P2 to P3
comment:4 follow-up: ↓ 5 Changed on 02/05/2019 at 10:41:16 PM 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.
comment:5 in reply to: ↑ 4 Changed on 02/08/2019 at 07:24:08 AM 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 on 03/20/2019 at 08:45:36 AM by mjethani
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:7 Changed on 03/27/2019 at 04:52:35 PM by abpbot
A commit referencing this issue has landed:
Issue 7260 - Internalize third-party request check in matcher
comment:10 Changed on 03/28/2019 at 10:09:45 AM by mjethani
- Resolution set to fixed
- Status changed from reviewing to closed
comment:11 Changed on 03/28/2019 at 10:10:10 AM by mjethani
- Cc greiner added
comment:12 Changed on 03/28/2019 at 01:19:34 PM by greiner
- Blocking 7417 added
comment:13 Changed on 07/25/2019 at 08:31:28 PM 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
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.