Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#5291 closed defect (rejected)

Possible memory leak in Adblock Plus

Reported by: kzar Assignee: mjethani
Priority: P2 Milestone:
Module: Platform Keywords:
Cc: sebastian, philll, BrentM, mjethani, mapx, trev Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Environment

  • Chrome 58
  • Adlbock Plus 1.13.2.1779
  • EasyList Germany+EasyList, Adblock Warning Removal List, AA

How to reproduce

  1. Enable those subscriptions, remove any others.
  2. Restart the extension from fresh.
  3. Take a heap snapshot for the Adblock Plus background page.
  4. Wait a while.
  5. Take another heap snapshot.

Observed behaviour

Memory usage appears to be growing. While I see 85mb initially, Phill saw around 310mb after some hours of very light usage.

Expected behaviour

Memory usage doesn't grow forever.

Attachments (2)

Screen Shot 2017-06-14 at 6.01.32 PM.png (412.0 KB) - added by mjethani 2 years ago.
Containment view
Screen Shot 2017-06-14 at 6.03.44 PM.png (218.6 KB) - added by mjethani 2 years ago.
Statistics view

Download all attachments as: .zip

Change History (27)

comment:1 Changed 2 years ago by kzar

We're still not sure that there is a memory leak, but I'm investigating in an attempt to find out.

comment:2 Changed 2 years ago by sebastian

BrentM observed that the memory usage, in his case, is going back to normal once he clicks on the browser action icon? Philip, can you check whether this is also true in your case?

comment:3 Changed 2 years ago by kzar

FWIW I couldn't reproduce that over the weekend, the memory usage didn't drop down for me after I opened the popup.

comment:4 Changed 2 years ago by philll

I can confirm that two of my three ABP instances dropped from ~200MB to ~150MB reported by Chrome's task manager when opening ABP options. The third instance was at around ~150MB and dropped to ~120MB.

comment:5 Changed 2 years ago by BrentM

I did another test last night, this time with AdBlock, ABP and uBlock Origin installed, and opened a page to YouTube. YouTube auto-played videos for several hours.

Both AdBlock's and ABP's memory started at about the same amount ~140 MB, and uBlock Origin was around ~58. The interesting thing is that both AdBlock's and ABP's memory climbed throughout the test (the most I saw was around ~430 MB). uBlock Origin's memory never increased beyound 65 MB. I wonder if it's specific extension API that we're using that uBlock Origin is not.

The above test was on a second Windows 10 PC with the stable version of Chrome.

I'll try a similar test on my Mac today, and post the results.

comment:6 Changed 2 years ago by sebastian

I was able to reproduce an increase in memory usage from ~120MB to ~160MB after one hour of playing a YouTube playlist, both with the current release version and development build of Adblock Plus. As reported by others the memory usage dropped back to normal after opening the browser action popup.

I took heap snapshots, before and after clicking the browser action icon, and the only noticeable difference I could spot was in the retained size of background page's window object, however, not in any of its contained objects. I filed a Chrome bug with more details.

I also tried to record the allocation timeline, however, the memory usage remained stable while the profiler was running. But perhaps somebody else has more luck.

comment:7 Changed 2 years ago by sebastian

For reference, Philip confirmed that the memory usage doesn't notably grow on Firefox using our WebExtensions build (which is essentially the same code), which is another indication that something is wrong in Chrome here.

comment:8 Changed 2 years ago by sebastian

  • Cc mjethani added

comment:9 Changed 2 years ago by mapx

  • Cc mapx added

comment:10 Changed 2 years ago by mapx

see this post on forum (could be related ?):
https://adblockplus.org/forum/viewtopic.php?p=169142#p169142

Memory usage issue after windows sleep

After I wake my windows 10 machine from sleep with chrome session still open, the memory usage of the plugin is huge.
Also, I'm using two chrome profiles at the same time and thus there are two adblockplus processes running.

You can see in the image below that it's taking more than 1GB for the two instances of adblock.

Chrome: Version 58.0.3029.110 (64-bit)
Windows 10 64-bit
Active lists: FR+EasyList - Malware Domains - EasyPrivacy

https://postimg.org/image/zat1f7xm9/

comment:11 Changed 2 years ago by mapx

courtesy of @gorhill:

About https://issues.adblockplus.org/ticket/5291, what I don't see being done in the comment is:

  • Open the dev tools
  • Go to Memory tab
  • Click the trash icon (I do 2-3 times, with a 2-second delay in between).

Yes, I have seen ABP grow to past 340 MB, and even leaving the browser on idle did not cause the memory to be garbage-collected. However, after accomplishing the above steps, there was memory being garbage-collected, and the memory snapped back to expected levels (keeping in mind fragmentation, js engine internals, etc.).

Any report of high memory usage should always be done after the steps above, including the equivalent ones on Firefox.

Last edited 2 years ago by kzar (previous) (diff)

comment:12 Changed 2 years ago by kzar

I haven't had too much luck investigating this so far. Using the memory debugging tools for the background page I don't see memory usage getting too crazy. For example I saved a heap snapshot which showed 95 megabytes of memory being used after the extension had been running for a few days / a week, restarted the extension and saved another which then showed 73 megabytes being used.

I haven't been able to reproduce the garbage collection problem by either opening the extension popup, nor by clicking the trash icon as Gorhill recommended.

Memory usage as listed by the Chrome task manager seems to fluctuate wildly, for example when using the debugging tools it sometimes doubles or worse. So I haven't been able to make much sense of that. I am starting to wonder if it also includes the memory usage of content scripts?

Using the allocation timeline tool I'm seeing memory being used, but not in unexpected ways. Prime culprits there seem to be CombinedMatcher.resultCache in adblockpluscore/lib/matcher.js and Filter.knownFilters in adblockpluscore/lib/filter.js.

So right now I'm trying to figure out what changed in my two snapshots in order to leak that 22 megabytes, but other than that I'm running out of ideas.

comment:13 follow-ups: Changed 2 years ago by kzar

  • Cc trev added

Some filters are extremely long, I wonder if we really need to use the whole thing for the knownFilters key? That seems to use a lot of memory, perhaps we could instead use a hash of the filter for the key instead? The parsed filter is stored as the value anyway.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 2 years ago by sebastian

Replying to kzar:

I haven't had too much luck investigating this so far. Using the memory debugging tools for the background page I don't see memory usage getting too crazy. For example I saved a heap snapshot which showed 95 megabytes of memory being used after the extension had been running for a few days / a week, restarted the extension and saved another which then showed 73 megabytes being used.

Also in my tests, the heap snapshots were much smaller than the memory usage indicated by the task manager, however there still was a notable difference in size of the snapshots, after running Adblock Plus for an extended period, and after triggering the popup. In my tests this difference clearly relates to the retained size of the window object, see above.

Using the allocation timeline tool I'm seeing memory being used, but not in unexpected ways. Prime culprits there seem to be CombinedMatcher.resultCache in adblockpluscore/lib/matcher.js and Filter.knownFilters in adblockpluscore/lib/filter.js.

These caches start off empty when the extension is loaded, and should cause the memory to notably grow after the first few filter matches, but once the caches are populated, the related memory usage should remain stable. I don't see anything suspicious there. But if you compare the memory usage to when the extension just have been loaded, you will obviously notice a difference here.

Replying to kzar:

Some filters are extremely long, I wonder if we really need to use the whole thing for the knownFilters key? That seems to use a lot of memory, perhaps we could instead use a hash of the filter for the key instead? The parsed filter is stored as the value anyway.

If we use a hash as key, the value would need to be an array, holding all filters for the same hash, which we'd need to iterate over and compare the actual filter text, during lookup. Otherwise, we will get (potential) collisions. This is not necessarily any more memory efficient, and also wouldn't explain a memory leak. Furthermore JavaScript engines generally don't copy strings as they are passed around, not exactly sure what happens when using them as key in an object though, but assuming they aren't copied in that scenario either, using a derived key would actually increase memory usage.

comment:15 in reply to: ↑ 14 Changed 2 years ago by kzar

Replying to sebastian:

If we use a hash as key, the value would need to be an array, holding all filters for the same hash, which we'd need to iterate over and compare the actual filter text, during lookup. Otherwise, we will get (potential) collisions. This is not necessarily any more memory efficient, and also wouldn't explain a memory leak.

You're right that this doesn't explain the memory leak.

But I think you might be wrong that it wouldn't save memory. I'm seeing over 70 such strings taking over 70kb each. I don't think we would have to always make the value an array, only when there actually was a collision would we have to. I've not tested this idea however, so who knows.

As discussed in IRC I see what you mean now, the Filter Object will have a reference to the string too, so unless a copy is being made this isn't going to help.

Last edited 2 years ago by kzar (previous) (diff)

comment:16 in reply to: ↑ 13 Changed 2 years ago by trev

Replying to kzar:

Some filters are extremely long, I wonder if we really need to use the whole thing for the knownFilters key? That seems to use a lot of memory, perhaps we could instead use a hash of the filter for the key instead? The parsed filter is stored as the value anyway.

This is what we are doing on the Emscripten branch, but I strongly suspect that V8 isn't very different - it shouldn't be creating a new string instance for the hash but rather reuse the same instance. A new instance would only be created if the string is modified which isn't the case here.

comment:17 Changed 2 years ago by kzar

  • Owner kzar deleted

I'm struggling to figure this one out, I still plan to investigate but if anyone else has any ideas I'd appreciate a hand.

comment:18 Changed 2 years ago by mjethani

  • Owner set to mjethani

Changed 2 years ago by mjethani

Containment view

Changed 2 years ago by mjethani

Statistics view

comment:19 Changed 2 years ago by mjethani

I've attached two screenshots from Developer Tools in Chrome 56.0.2924.0 (base commit a89f0dfb6025b3026dd43dcee02633e502b02b34) on macOS 10.12.5. They're called "Containment view" and "Statistics view". I describe them below.

First I modified the Adblock Plus extension to load the subscriptions every minute instead of after long intervals. I then added five subscriptions. After this I reloaded the extension.

Within a couple of minutes, Chromium's Task Manager showed the extension's memory consumption to be 1.8 GB.

I then took a heap snapshot and examined it in the Containment view. This is the first screenshot. It clearly shows that a lot of memory is being consumed by multiple arrays, all of the same size, and all pointing to the patterns.ini file. Based on my analysis of the code, I conclude that this is the chrome.storage.local.get API simply holding on to the memory it allocates for the file as it is read at regular intervals. This is simply a case of delayed garbage collection. When the popup is opened, the browser seems to run GC and clear up this space. The same effect can be observed by running GC manually.

The second screenshot is simply the same heap snapshot in the Statistics view. It shows clearly that most of the used memory in the heap is being consumed by "System Objects"

I have also found, multiple times, with different versions of Chrome, that eventually the browser does clean up this memory. It may take a long while to do so, but at least on my system it does get around to it eventually.

From this I conclude that this is simply a case of delayed garbage collection. There is no memory leak.

comment:20 Changed 2 years ago by mjethani

I should add that I was sent a heap snapshot by philll and that one too similarly implicated patterns.ini

comment:21 Changed 2 years ago by trev

Note that browsers generally don't guarantee running garbage collection unless there is memory pressure. However, in the Chrome Developer Tools you have a trash can symbol in the Performance tab. This one will let you force garbage collection which should allow validating your claims.

comment:22 Changed 2 years ago by mjethani

Yes, I did run GC manually (as I said in my previous comment) by clicking on the trash icon. It had the same effect as when opening the popup.

comment:23 Changed 2 years ago by rhana@…

Is there an update on this? Thanks.

comment:24 Changed 2 years ago by sebastian

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

It seems there is nothing we can do about this on our end, besides reducing memory usage in general, which would also mitigate the effects of delayed garbage collection. Our best attempt here is #4122.

comment:25 Changed 2 years ago by mjethani

For what it's worth, there's a trick that apparently seems to work for memory leaks caused due to V8's "sliced strings"

Note: See TracTickets for help on using tickets.