Fix WebExtension Excessive Memory Usage

[cschanaj]

Type: other

Re-open of #1775, based on #1775 (comment) ordered by priority (personal)

GOAL

???

PROPOSAL

Proof of concept PRs, may or may not be get implemented for various reasons. Discussions should be made in their respective PRs.

• Try using a Prefix Tree for RuleSets.targets (Try using a prefix tree for RuleSets.targets #12530)
• Try using a LRU Cache for caching (WIP: Try using a LRU Cache for caching #12591) See Simplify caching using ES6 Map() #3952

TODO

Core

• Use Map instead of Object for RuleSets.targets (Use Map instead of Object for RuleSets.targets #12160) (2017.8.31)
  • improve performance
• Use JSON for default.rulesets instead of XML (Use JSON for default.rulesets instead of XML #12273) (2017.9.12)
  • reduce the startup time to below 1.5 seconds
  • reduce the memory usage to below 100mb (on Chromium task manager on startup after GC).
  • reduce the size of default.rulesets from 8.4mb to 6.4mb and thus smaller extension size
• Use Map instead of Object for redirectCounter and Fix Memory Leak (Use Map instead of Object for redirectCounter and Fix Memory Leak #12422) (2017.9.12)
  • fix a small memory leak for long lasting browsing session
  • improve performance
• Use null instead of Empty Set for in RuleSets.ruleCache (Use null instead of new Set in RuleSets.ruleCache #12494, Use nullIterable instead of new Set in RuleSets.ruleCache #12577) (2017.10.24)
  • avoid unnecessary memory allocation
• Merge exclusions into a single pattern in default.rulesets (Slightly reduce default.rulesets size and memory usage #16092, related Measure and slightly improve memory usage #15448) (2018.7.8)
  • reduce memory overhead
• Trivialize rulesets with static securecookie, thanks @RReverser (Detect static securecookies to trivialize more rules #16029, Batch trivialize rulesets with static securecookies #16378) (2018.9.19)
  • removed 300+ wildcard targets, improve lookup speed and memory usage
  • related Add a script to trivialize a special form of securecookies #16540, Add patterns to trivialize additional forms of securecookies #16634
• Merge securecookies which match any cookie.name in each rulesets (2019.5.2)
  • Refactor and enhance trivialize-cookie-rules.js (Refactor and enhance trivialize-cookie-rules.js #17438, Run node utils/trivialize-rules/trivialize-cookie-rules.js #17578)
  • Update utils/merge-rulesets.py (Update utils/merge-rulesets.py #17492)
  • Remove/ Update some securecookie with no coverage (Update some securecookie with no coverage #17565)
• Drop support for middle wildcard targets in potentiallyApplicableRulesets (2019.5.2)
  • Forbid and remove wildcards in the middle (Forbid * in the middle of the target host #4369, Remove support for wildcard in the middle #12319 thanks @Bisaloo)
  • Remove middle wildcard support in rules.js (Remove middle wildcard support in rules.js #17715)
  • reduce runtime overhead (lookup all_rules) significantly
• Triviailize rulesets with right-wildcard targets and trivialize rewrites (Add script to trivialize some left-wildcard targets #17855, Run node utils/trivialize-rules/trivialize-targets.js #18068) (2019.6.27)
  • Removed around 1600+ wildcards target, improve lookup speed and memory usage
• Simplify shouldSecureCookie and safeToSecureCookie (Do not call safeToSecureCookie until necessary #17742) (2019.6.27)
• Refactor code for the browser session storage (Refactor code for the browser session storage #18153) (2019.11.7)
• Async addFromJson, parseOneJsonRuleset
  • avoid blocking code to improve performance
• Abstraction for Rule and Exclusion
  • avoid redundant array of similar objects
  • better API (?)

UI

• Improve popup page performance and reduce memory usage (Improve popup page performance and reduce memory usage #15532) (2018.6.21)
  • Avoid overhead with document.createElement
  • Avoid unnecessary listeners with event delegations

PENDING FIX

• Point RuleSets.targets.get(host) to a Ruleset instead of Array of Ruleset
  • REQUIRE forbid and remove duplicated targets (Include rulesets with different platforms in duplicate checks #11440)
  • improve heap performance and simplify lookup operations
• Use a Radix Trie instead of Map for RuleSets.targets with reversed hostname
  • REQUIRE forbid and remove wildcards at the right (Remove right-wildcard targets (if possible) #11203)
  • efficient memory structure

WON'T FIX

• Compile RegExp only when it is neccessary
  • RegExp are already lazily compiled on Chromium and Firefox improve browser startup overhead
• Update rules.js to limit cache size to 256 entries (Update rules.js to limit cache size to 256 entries #13328)
  • Caching has a small memory overhead as compared to the extension memory usage
• Avoid using chrome.runtime.sendMessage() whenever possible
  • Explained in Fix WebExtension Excessive Memory Usage #12232 (comment)
  • REF Avoid using chrome.runtime.sendMessage() wherever possible gorhill/httpswitchboard#345
  • REF https://bugs.chromium.org/p/chromium/issues/detail?id=320723
  • REF https://github.com/gorhill/uBlock/blob/master/src/js/messaging.js## MISC ISSUES
• Use ES6 generator functions to reduce run-time overhead in search & lookup (Use ES6 generator functions to reduce run-time overhead in search & lookup #16951)
  • Show only rulesets which performed an upgrade (Show only rulesets which performed an upgrade #17047)

Bugs or unexpected features found when reading through the code

• Credentials in URLs are not restored upon rewrites (Credentials in URL are not Restored #12377, Update background.js (fixes #12377) #12380)
• Clear domainBlacklist and urlBlacklist when all Incognito sessions are destroyed (Update chromium/incognito-cache-clearing.js #12452)
• Inconsistent behavior for .onion and .onion. (with tailing dot) hosts (Inconsistent behavior for .onion and .onion. (with tailing dot) hosts #13568, Update chromium/background.js , Fix #13568 #13569)
• Disabled site (with/ without tailing dots) are treated differently (Disable on site feature does not work on same site with/ without tailing dots  #18451, Normlise disabled site hostname and migrate stored list, fix #18451 #18452)

[ghost]

@cschanaj See https://github.com/koops76/https-everywhere/projects/1 for more ideas.

[RReverser]

Compile RegExp only when it is neccessary

Not sure about SpiderMonkey, but in V8 regexps are anyway compiled lazily only on the first use and not when regexp literal is met or new RegExp is called.

[cschanaj]

Using the constructor function provides runtime compilation of the regular expression.

Firefox only mention that new RegExp() is compiled in the runtime. Not really sure if it is lazily compiled or not. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions

@RReverser do you have a source confirming for V8's behavior? Confirmed this behavior on Chromium browser using the console and taking heap snapshot. It also seems to me that Firefox does lazy compilation as well. thanks!!

[cowlicks]

RE: radix trie with reversed hostnames. I've been wanting to use something like this for privacy badger's storage datastructure. I have a pending PR with an implementation here https://github.com/EFForg/privacybadger/pull/1568/files#diff-930ddd92cefc0f8bc3a8722e10b3e05e

I'm not using Map, but I probably should be.

cc @Hainish

[Bisaloo]

Several of those points will require a major rewrite of google rulesets (duplicated multiple times, right wildcards, etc.). I don't really like the way targets and rules are split right now anyways. I was already thinking of a major overhaul of these rulesets. I will probably open a new issue about it soon.

[ghost]

@cschanaj The link you gave for " Avoid using chrome.runtime.sendMessage() whenever possible " no longer works

[cschanaj]

It seems that Gorhill has disabled the "Issues" in his repo. Here is some comments made by Gorhill that we might be interested in

18 Jun 2014

The status of the issue:

https://code.google.com/p/chromium/issues/detail?id=320723

is ambiguous. It's not marked as fixed "verified", on the other hand it's labeled M32. Quite unclear whether the memory leak was solved or not.

In any case, there are many places in HTTPSB where using a long-lived connection makes more sense than using a one time message, which comes with a good overhead if I read correctly.

Note: It is marked as fixed "verified" AND labeled M32 now.

18 Jun 2014

Well that is interesting.. Not using chrome.runtime.sendMessage() reduces markedly memory footprint of the loaded web page when loading the vim test page, and it does appear it is easier on the CPU -- so finding is significantly less overhead using Port.postMessage() than chrome.runtime.sendMessage().

18 Jun 2014

Yes, but it's not as simple as replacing one string with another. I need to write my own sendMessage()-like code, but instead of creating a port every time like the chrome API does, I reuse the one instance.

20 Jun 2014

Finally got rid of chrome.runtime.sendMessage() from everywhere, now using ports. For each message sent, chrome.runtime.sendMessage() creates a port, so this is an improvement. I can't measure for sure though. If there are still memory leaks with using sendMessage() as reported in the above-linked chromium issue, then they should be fixed as far as HTTPSB is concerned.

I did notice in some previous memory tests using heap snapshot diffs that there seemed to be a lot of instances of objects which I believe might have been related to message event objects.

In any case, even if sendMessage() wasn't leaking, using a longer-lived port rather than one-per-message is saner. Currently ports are created when an extension page or a content script is created, and the same port is used until the page or content script is destroyed. Much less overhead.

[Hainish]

The most common use of chrome.runtime.sendMessage() in the codebase currently is to communicate which rulesets are active in a tab to the popup, upon creation. Other usages include communicating options toggles, importing a json settings blob from the options UX, as well as adding and deleting new rules. These currently all must necessarily go through sendMessage since addon HTML pages have to communicate with the background context.

Profiling the extension memory usage after subsequent clicks on the popup after visiting boingboing.net (which has lots of active rulesets communicated), I do see an increase overall, but I'm unsure what to look for in a heap analysis to see how much is due to this particular leak. And the overall memory usage in Chrome vacillates for me between 75 and 86M, settling at 72M.

I wonder if we can safely ignore this problem until the upstream leak is patched. It seems to me that any minimization we do with sendMessage will come at a cost of usability or presentation to the user of ruleset data.

[jhpratt]

Any updates on this? A quick check by toggling this extension shows that it's using between 40 and 80 MB of memory (uncertainty because it's not the only extension).

[jhpratt]

@Giltyhub2 I actually saw that pull elsewhere and reinstalled — I imagine wasm will help out as well. I'll certainly report back if I notice any issues.