Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#303 closed change (fixed)

Don't load element hiding rules

Reported by: fhd Assignee:
Priority: P2 Milestone: Adblock-Plus-for-Android-1.3
Module: Adblock-Plus-for-Android Keywords:
Cc: jobp@…, arthur, trev Blocked By:
Blocking: #1541 Platform: Unknown
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/4980445527670784/
http://codereview.adblockplus.org/6680224334872576/

Description (last modified by fhd)

Background

Element hiding is currently disabled, mainly to save memory. However, ever since we moved to libadblockplus, element hiding rules are being kept in memory again.

What to change

Don't load any element hiding filters.

Once element hiding is properly disabled, we can create an issue for enabling it again. But before we can do that, we need to make sure that the elemhide filter option works, that we don't cause blank pages in Chrome, and ideally we should have reduced memory usage significantly with #145.

Change History (18)

comment:1 Changed 5 years ago by fhd

  • Description modified (diff)

comment:2 Changed 5 years ago by fhd

  • Description modified (diff)

comment:3 Changed 5 years ago by mapx

  • Cc smultron45@… added

comment:4 Changed 5 years ago by jobp

  • Cc jobp@… added

comment:5 Changed 5 years ago by arthur

  • Cc arthur added

comment:6 Changed 5 years ago by fhd

  • Platform set to Unknown

For reference: Here's how we kept elemhide rules from loading prior to the libadblockplus integration: https://hg.adblockplus.org/adblockplusandroid/rev/0148c2a0398b

Since we can't directly modify the Core code anymore (nor should we!), we either need to add this as a feature to libadblockplus, or find an Android-specific workaround for the time being. Wouldn't really mind the latter.

comment:7 Changed 5 years ago by mapx

  • Cc smultron45@… removed

comment:8 Changed 5 years ago by rjeschke

  • Review URL(s) modified (diff)

comment:9 Changed 5 years ago by fhd

  • Blocked By 1460 added

comment:10 Changed 5 years ago by fhd

As discussed in the review, adding this as a libadblockplus feature is pretty easy and doesn't require a Core change. In that case it seems like the best route.

comment:11 Changed 4 years ago by rjeschke

I would still implement the hack first and create a follow up issue for removing the hack. The reason why I would like to go this way is the following:

If we do not want to change the core we have to remove the filters after they have been loaded. This does only create a small benefit, as we could reduce long-time memory usage. However, peak memory usage is still high (this could lead to a OOM with instant kill) and because of v8's GC behaviour it is not guaranteed when the memory will be freed.

The IMHO correct way to implement this is to make a change to the core to prevent loading of elemhide filters in the first place instead of removing them afterwards. This change, however, will result in some more changes concerning e.g. properties and also in testing issues.

comment:12 Changed 4 years ago by fhd

  • Cc newshades trev added; arthur removed

Your call at the end of the day. Frankly, I don't particularly like either hack and was hoping to sit this out until we have typed objects, and the saved memory is negligible :)

If we're going to mess with the response text, we should ensure as good as possible that the response actually contains a filter list.

comment:13 Changed 4 years ago by fhd

  • Cc arthur added; newshades removed

Restored the CC list (was only planning to add Wladimir, no idea what happened).

comment:14 Changed 4 years ago by rjeschke

  • Review URL(s) modified (diff)

comment:15 Changed 4 years ago by rjeschke

  • Blocking 1541 added

comment:16 Changed 4 years ago by rjeschke

  • Blocked By 1460 removed

comment:17 Changed 4 years ago by rjeschke

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

comment:18 Changed 4 years ago by rjeschke

  • Milestone set to Adblock-Plus-for-Android-1.3
Note: See TracTickets for help on using tickets.