Opened 15 months ago

Closed 2 months ago

#6857 closed defect (rejected)

SpecialSubscription objects are never removed

Reported by: mjethani Assignee:
Priority: P3 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
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 (17)

comment:1 Changed 15 months ago by mjethani

  • Cc kzar jsonesen hfiguiere added

comment:2 Changed 15 months ago by mjethani

  • Cc sebastian added

comment:3 Changed 15 months ago by mjethani

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

comment:4 Changed 15 months ago by mjethani

  • Cc greiner added

comment:5 follow-up: Changed 15 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 15 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 15 months ago by mjethani

  • Review URL(s) modified (diff)

Added a tentative fix.

comment:8 Changed 15 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 15 months ago by jsonesen

  • Owner set to jsonesen

comment:10 Changed 14 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 13 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 13 months ago by abpbot

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

comment:13 Changed 13 months ago by mjethani

  • Description modified (diff)

comment:14 Changed 13 months ago by jsonesen

  • Owner jsonesen deleted

comment:15 Changed 9 months ago by abpbot

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

comment:16 Changed 9 months ago by ukacar

  • Verified working set

comment:17 Changed 2 months ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from new to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Note: See TracTickets for help on using tickets.