Opened 9 months ago

Last modified 3 months ago

#6857 new defect

SpecialSubscription objects are never removed

Reported by: mjethani Assignee:
Priority: P3 Milestone:
Module: Core Keywords:
Cc: kzar, jsonesen, hfiguiere, sebastian, greiner Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29854572/

Description (last modified by mjethani)

Environment

Adblock Plus browser extension as of changeset a87d009836da on Chrome (macOS 10.12.6)

How to reproduce

  1. Build the Adblock Plus browser extension from source and load it as an unpacked extension
  2. Open DevTools for the background page and take a heap snapshot
  3. Search for "SpecialSubscription" in the heap snapshot and note down the number of SpecialSubscription objects (could be zero if this is a fresh install)
  4. Open the options page and add the filter !foo to the "My filters" box
  5. Take another heap snapshot and note down the number of SpecialSubscription objects as in step 3
  6. Delete the filter !foo from "My filters" and save the changes
  7. Add a new filter !bar to "My filters"
  8. Take another heap snapshot and note down the number of SpecialSubscription objects as in step 3

Observed behaviour

If the number of SpecialSubscription objects in memory in step 5 is N, the number of object in step 8 is N + 1. Neither reloading the extension nor restarting the browser changes anything. Over time, this can grow to a large number and likely slow down the performance of the extension.

Expected behaviour

The number of SpecialSubscription objects in memory in step 8 should be N. A subscription once removed should be deleted entirely.

Additional notes

The changes for #6855 do not fix this.

Hints for testers

This is still a work in progress, but for changeset c767cbd74705, this would be a good test:

  1. Fresh install of ABP
  2. On the options page, go to "My filters" and add a custom filter, and save the changes
  3. Now remove the filter and save the changes
  4. Add a new filter once again and save the changes
  5. Reload the extension and open the options page again

Expected: The last saved filter should still be there.

Change History (16)

comment:1 Changed 9 months ago by mjethani

  • Cc kzar jsonesen hfiguiere added

comment:2 Changed 9 months ago by mjethani

  • Cc sebastian added

comment:3 Changed 9 months ago by mjethani

For what it's worth, on my machine I have 46 SpecialSubscription objects.

comment:4 Changed 9 months ago by mjethani

  • Cc greiner added

comment:5 follow-up: Changed 9 months ago by mjethani

Why does the concept of filter "groups" exists at all? In the extension UI all of the user's filters are in one text box. In core they are split up into multiple "groups" (different SpecialSubscription objects for elemhide, blocking, and so on). What's worse is that if there is no default group for a filter type, such as a comment filter, then it ends up in an entirely new SpecialSubscription all by itself.

comment:6 in reply to: ↑ 5 Changed 9 months ago by mjethani

Replying to mjethani:

What's worse is that if there is no default group for a filter type, such as a comment filter, then it ends up in an entirely new SpecialSubscription all by itself.

Filed #6858 for this.

comment:7 Changed 9 months ago by mjethani

  • Review URL(s) modified (diff)

Added a tentative fix.

comment:8 Changed 9 months ago by greiner

Just for reference, I've posted my findings on the purpose of special subscription groups at 8:ticket:6859.

comment:9 Changed 9 months ago by jsonesen

  • Owner set to jsonesen

comment:10 Changed 8 months ago by jsonesen

Heh, I set myself as the owner but I also see you have the quick fix in review is that review blocked by something?

comment:11 Changed 8 months ago by mjethani

Jon, yes, I have a quick fix for this. It doesn't entirely fix the issue, so you could keep this assigned to yourself for the proper fix.

comment:12 Changed 8 months ago by abpbot

A commit referencing this issue has landed:
Issue 6857 - Do not serialize empty special subscriptions

comment:13 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:14 Changed 7 months ago by jsonesen

  • Owner jsonesen deleted

comment:15 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6857 - Do not serialize empty special subscriptions

comment:16 Changed 3 months ago by ukacar

  • Verified working set
Note: See TracTickets for help on using tickets.