Opened on 02/21/2019 at 04:18:04 PM
Closed on 04/27/2019 at 09:27:01 AM
Last modified on 07/26/2019 at 03:49:42 AM
#7308 closed change (fixed)
Update adblockpluscore dependency to hg:728da87ae4b0
| Reported by: | mjethani | Assignee: | mjethani |
|---|---|---|---|
| Priority: | Unknown | Milestone: | Adblock-Plus-for-Chrome-Opera-Firefox-next |
| Module: | Platform | Keywords: | |
| Cc: | jsonesen, sebastian, kzar, greiner | Blocked By: | #7401, #7423 |
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | no | Confidential: | no |
| Tester: | Ross | Verified working: | yes |
| Review URL(s): |
https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/45 |
||
Description (last modified by mjethani)
Background
We have made some breaking changes in adblockpluscore since the last dependency update and it would be a good idea to update the dependency once again.
Here is the list of changes:
- Issue 7267 - Optimize memory layout of filter domain maps in matcher
- Issue 7268 - Unescape { and } for :-abp-contains()
- Issue 7285 - Abstract caching logic
- Issue 7295 - Add non-active filters to blocking group
- Issue 7284 - Move CSS escaping to createStyleSheet
- Issue 7094 - Keep subscription filters by text only
- Issue 7294 - Implement strip-fetch-query-parameter snippet
- Issue 7294 - Pass necessary functions to strip-fetch-query-parameter
- Issue 7291 - Don't mangle selectors with a '*'
- Issue 7303 - Deprecate the use of String.substr()
- Issue 7094 - Link filters with subscriptions in INI parser
- Issue 7291 - Handle *.foo in qualifySelector()
- Issue 7296 - Implement lightweight URL parsing
- Issue 7311 - Split _checkEntryMatchByDomain()
- Issue 7312 - Ignore lone pure generic filters for specific-only queries
- Issue 7321 - Lower-case non-literal patterns for case-insensitive matching
- Issue 7324 - Simplify logic for domains property
- Issue 7356 - Allow wrapping of function properties in snippets
- Issue 7324 - Set _domains property for non-empty domain maps
See also related UI changes.
What to change
Update adblockpluscore dependency to hg:728da87ae4b0 git:da8c662.
Replace let {isThirdParty} = require("../adblockpluscore/lib/domain"); with let {isThirdParty} = require("../adblockpluscore/lib/url"); in lib/csp.js, lib/filterComposer.js, lib/popupBlocker.js, lib/requestBlocker.js, and lib/whitelisting.js.
Replace the following type of loop:
for (let filter of subscription.filters())
{
...
}
With the following:
for (let text of subscription.filterText())
{
let filter = Filter.fromText(text);
...
}
In lib/hitLogger.js and anywhere else necessary.
Replace any calls to filterAt() with filterTextAt() wherever necessary (see full details in #7094).
Use the new parseURL() function (#7296) where appropriate.
Update adblockplusui dependency to hg:7c7ff4c65b20 git:ec910b0.
Remove lib/filterValidation.js and related tests.
Hints for testers
Please see individual tickets for the changes above.
Additionally, try this:
<!DOCTYPE html>
<html>
<body>
<img src="https:example.com"> <!-- Note: URL is not canonicalized -->
</body>
</html>
The filter ||example.com^$image should block the request to https:example.com (which will likely appear correctly as https://example.com/ in the console). The filter |https://example.com/$image should also block the same request. Also make sure the $third-party and $~third-party options work in this case (e.g. |https://example.com/$image,~third-party should not block the request, unless the document is loaded from example.com).
Attachments (0)
Change History (39)
comment:2 Changed on 03/13/2019 at 05:23:44 AM by jsonesen
- Cc jsonesen sebastian kzar added
comment:3 Changed on 03/13/2019 at 06:15:51 AM by jsonesen
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:4 Changed on 03/13/2019 at 06:16:03 AM by jsonesen
- Owner set to jsonesen
comment:5 Changed on 03/13/2019 at 09:23:08 AM by mjethani
- Description modified (diff)
- Summary changed from Update adblockpluscore dependency to hg:b30aa01e56ca to Update adblockpluscore dependency to hg:7c1cf4865d70
comment:6 Changed on 03/13/2019 at 10:00:37 AM by mjethani
- Cc greiner added
comment:7 Changed on 03/13/2019 at 12:53:48 PM by kzar
Issue looks pretty good, will triage it once the adblockplusui part is taken care of. But, please could you add a "Hints for testers" section?
comment:8 Changed on 03/13/2019 at 04:29:45 PM by greiner
I'm not sure how to best approach this from our end and it's unfortunate that we couldn't prepare for it.
Considering that we're already finishinig up #7343 and #6936 and that we only have a single Mercurial branch to supply dependency updates to both adblockpluschrome branches, I'd suggest getting #7343 in first, then this one (see also ui#366) and finally #6936 as soon as we're starting work towards the next major release.
comment:9 Changed on 03/16/2019 at 10:55:24 AM by mjethani
- Description modified (diff)
- Summary changed from Update adblockpluscore dependency to hg:7c1cf4865d70 to Update adblockpluscore dependency to hg:728da87ae4b0
comment:10 Changed on 03/16/2019 at 10:58:46 AM by mjethani
I have updated the changset to hg:728da87ae4b0. This includes the changes to the test runners requested by Tristan.
comment:11 Changed on 03/24/2019 at 07:20:35 PM by jsonesen
- Review URL(s) modified (diff)
comment:12 Changed on 03/25/2019 at 05:12:40 PM by greiner
- Blocked By 7401 added
comment:13 Changed on 03/26/2019 at 03:41:33 PM by jsonesen
Just an update here, it seems I may have found a core regression. With #7401 and #7308 applied, restarting the browser causes filters to be lost (the memory on start is 23mb and blocking is not happening with no errors anywhere) then going to the UI and manually updating the filer lists from the advanced tab causes the memory footprint to jump to the expected ~110mb and blocking is resumed.
Also, ut behaves as expected on first run, but not subsequent runs.
comment:14 Changed on 03/27/2019 at 04:39:50 AM by mjethani
@jsonesen are you seeing any errors in the background page console?
comment:15 Changed on 03/28/2019 at 10:15:55 AM by mjethani
While more changes have landed in adblockpluscore, this dependency update is already quite complex so I would not add more to it. We will do another one after this.
comment:16 Changed on 03/28/2019 at 01:38:02 PM by greiner
In my opinion we should make sure not to introduce regressions during dependency updates unless we're not aware of them yet or there's a good enough reason to do so (e.g. introducing a small regression in favor of a larger fix). Therefore I'd oppose leaving out a fix for an issue that will break the next branch just for the sake of finishing up a dependency update.
comment:17 Changed on 03/28/2019 at 02:30:49 PM by kzar
Yea, I agree with both sentiments. We can't repeat the painful dependency update we had recently which risked carrying on indefinitely, but more importantly the purpose of the dependency update process in the first place is to avoid regressions.
So, I agree with Manish that we should stop including further commits with this dependency update, but with the important exception of commits that fix regressions caused by commits which were already included.
Unfortunately, in practice this might mean that some extra commits before the fix for a regression end up getting included too, but I don't see how we can easily avoid that.
comment:18 Changed on 03/28/2019 at 03:38:01 PM by mjethani
There are no regressions in this dependency update that I am aware of.
comment:19 Changed on 03/28/2019 at 03:41:39 PM by mjethani
Well I should add that the snippets do not entirely work as of this dependency update (i.e. strip-fetch-query-parameters does not work on Chrome <50 (#7407)), but I do not think this is bad enough for us to include the fix here if it means bringing in even more breaking changes which would require even more work.
If this is what Thomas meant then I'm OK with including more in this dependency update but then I am pretty sure it is going to take longer to land and keep people waiting for longer. I do not consider this to be practical.
comment:20 follow-up: ↓ 22 Changed on 03/28/2019 at 04:59:54 PM by kzar
So you're saying the snippet doesn't work on outdated versions of Chrome, but that it fails gracefully without breaking anything else? If so, I agree that doesn't count as a regression and isn't worth including in this dependency update.
comment:21 follow-up: ↓ 23 Changed on 03/28/2019 at 05:44:39 PM by greiner
The issue I was referring to is the one Jon outlined where nothing is being blocked because filters weren't loaded into memory when loading them from extension storage.
comment:22 in reply to: ↑ 20 Changed on 03/28/2019 at 08:05:46 PM by sebastian
Replying to kzar:
So you're saying the snippet doesn't work on outdated versions of Chrome, but that it fails gracefully without breaking anything else?
Not gracefully, there is an error. There is a fix that makes the snippet work properly on Chrome 49-50, but if we update the dependency to that revision this will also pull in a bunch of other changes here.
Anyway, we will have another dependency update before code freeze for 3.6, pulling in all the remaining changes (including the aforementioned fix). However, I personally wouldn't mind either to combine the dependency updates. Yes, this would require more integration changes, but we have to implement those soon anyway. Neither do we get to test anything sooner if we land this dependency update earlier, as this dependency update is landing in next and we don't merge next into master (which devbuilds are built from) before we enter code freeze.
Replying to greiner:
The issue I was referring to is the one Jon outlined where nothing is being blocked because filters weren't loaded into memory when loading them from extension storage.
Yeah, I'm also curious what that is about.
comment:23 in reply to: ↑ 21 Changed on 03/28/2019 at 08:13:45 PM by mjethani
Replying to greiner:
The issue I was referring to is the one Jon outlined where nothing is being blocked because filters weren't loaded into memory when loading them from extension storage.
Based on an IRC chat with Jon this turns out to be basically a non-issue, but perhaps he can throw some light on this.
I do agree that if this is the case then it would be rather serious. The point of any dependency update here is to make sure adblockpluschrome next branch is working out of the box with any sub-repos. If it is not working then it is kind of pointless to do it.
comment:24 follow-up: ↓ 25 Changed on 03/28/2019 at 08:20:04 PM by jsonesen
Sorry for the late update, essentially it was a non issue. However, I did notice that reloading the extension from the background page will double memory usage on each reload until the browser is restarted, looking at snapshots it is holding a copy of all the filters for every reload. But restarting the browser restores memory, additionally, this occurs on the current release so is not regression. I am good to move forward and am working in the UI update as we speak.
comment:25 in reply to: ↑ 24 Changed on 03/29/2019 at 08:48:31 AM by mjethani
Replying to jsonesen:
However, I did notice that reloading the extension from the background page will double memory usage on each reload until the browser is restarted [...]
If you are refreshing the background page in DevTools to reload the extension, I think it keeps a copy in memory for debugging purposes (e.g. comparing memory snapshots). The reliable way to reload the extension is to click on the reload button on the chrome://extensions page.
In general keeping DevTools open will increase the memory usage of the extension. I find myself closing it, reloading the extension, and then reopening it to debug again.
comment:26 Changed on 04/02/2019 at 08:19:49 AM by mjethani
- Description modified (diff)
- Owner changed from jsonesen to mjethani
Jon and I have decided to the split the work here by module: I will be doing adblockpluschrome now. Assigned to myself.
comment:27 Changed on 04/02/2019 at 09:47:24 AM by mjethani
- Review URL(s) modified (diff)
comment:28 Changed on 04/02/2019 at 09:48:38 AM by mjethani
- Description modified (diff)
comment:29 Changed on 04/02/2019 at 02:54:39 PM by greiner
- Blocked By 7423 added
comment:30 Changed on 04/10/2019 at 04:58:57 PM by mjethani
- Description modified (diff)
comment:31 Changed on 04/11/2019 at 11:35:56 PM by jsonesen
- Blocked By 6936 added
comment:32 Changed on 04/13/2019 at 11:07:04 AM by mjethani
- Description modified (diff)
comment:33 Changed on 04/13/2019 at 11:07:34 AM by mjethani
- Description modified (diff)
comment:34 Changed on 04/16/2019 at 03:09:48 PM by greiner
- Blocked By 6936 removed
comment:35 Changed on 04/26/2019 at 12:51:37 PM by greiner
- Description modified (diff)
Updated adblockplusui hashes and added reference to UI issues to ticket description.
comment:36 Changed on 04/27/2019 at 06:52:40 AM by abpbot
A commit referencing this issue has landed:
Issue 7308 - Update adblockpluscore dependency to hg:728da87ae4b0
comment:37 Changed on 04/27/2019 at 09:26:03 AM by mjethani
- Description modified (diff)
comment:38 Changed on 04/27/2019 at 09:27:01 AM by mjethani
- Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
- Resolution set to fixed
- Status changed from reviewing to closed
comment:39 Changed on 07/26/2019 at 03:49:42 AM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Done. Working as described. Child tickets working as described.
ABP 0.9.15.2339
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 have added Thomas. This will also require changes in UI. Subscription.prototype.filters() is gone, now you have to create the filter object from the text yielded from Subscription.prototype.filterText(), using Filter.fromText().