Opened 3 months ago

Closed 3 weeks ago

#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: Unknown Verified working: no
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/45
https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/merge_requests/249
https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/55

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:

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).

Change History (38)

comment:1 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 2 months ago by jsonesen

  • Cc jsonesen sebastian kzar added

comment:3 Changed 2 months ago by jsonesen

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

comment:4 Changed 2 months ago by jsonesen

  • Owner set to jsonesen

comment:5 Changed 2 months ago by mjethani

  • Description modified (diff)
  • Summary changed from Update adblockpluscore dependency to hg:b30aa01e56ca to Update adblockpluscore dependency to hg:7c1cf4865d70

comment:6 Changed 2 months ago by mjethani

  • Cc greiner added

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().

comment:7 Changed 2 months ago 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 2 months ago 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 2 months ago by mjethani

  • Description modified (diff)
  • Summary changed from Update adblockpluscore dependency to hg:7c1cf4865d70 to Update adblockpluscore dependency to hg:728da87ae4b0

comment:10 Changed 2 months ago by mjethani

I have updated the changset to hg:728da87ae4b0. This includes the changes to the test runners requested by Tristan.

comment:11 Changed 8 weeks ago by jsonesen

  • Review URL(s) modified (diff)

comment:12 Changed 8 weeks ago by greiner

  • Blocked By 7401 added

comment:13 Changed 8 weeks ago 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 8 weeks ago by mjethani

@jsonesen are you seeing any errors in the background page console?

comment:15 Changed 7 weeks ago 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 7 weeks ago 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 7 weeks ago 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 7 weeks ago by mjethani

There are no regressions in this dependency update that I am aware of.

Last edited 7 weeks ago by mjethani (previous) (diff)

comment:19 Changed 7 weeks ago 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.

Last edited 7 weeks ago by mjethani (previous) (diff)

comment:20 follow-up: Changed 7 weeks ago 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: Changed 7 weeks ago 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 7 weeks ago 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.

Last edited 7 weeks ago by sebastian (previous) (diff)

comment:23 in reply to: ↑ 21 Changed 7 weeks ago 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: Changed 7 weeks ago 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 7 weeks ago 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 7 weeks ago 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 7 weeks ago by mjethani

  • Review URL(s) modified (diff)

comment:28 Changed 7 weeks ago by mjethani

  • Description modified (diff)

comment:29 Changed 7 weeks ago by greiner

  • Blocked By 7423 added

comment:30 Changed 6 weeks ago by mjethani

  • Description modified (diff)

comment:31 Changed 5 weeks ago by jsonesen

  • Blocked By 6936 added

comment:32 Changed 5 weeks ago by mjethani

  • Description modified (diff)

comment:33 Changed 5 weeks ago by mjethani

  • Description modified (diff)

comment:34 Changed 5 weeks ago by greiner

  • Blocked By 6936 removed

comment:35 Changed 3 weeks ago by greiner

  • Description modified (diff)

Updated adblockplusui hashes and added reference to UI issues to ticket description.

comment:36 Changed 3 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 7308 - Update adblockpluscore dependency to hg:728da87ae4b0

comment:37 Changed 3 weeks ago by mjethani

  • Description modified (diff)

comment:38 Changed 3 weeks ago by mjethani

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.