1420485 - Clear the tabs.insertCSS cssCode once it has been converted into a data url

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

While reviewing the amount of the memory used in the content process by a couple of the commonly used adblocking WebExtensions (e.g. adblock plus and ublock origin), I noticed that a good chunk of it is related to the css injected by these extensions into the webpages running in the tab (and their subframes) using browser.tabs.insertCss.

These stylesheets seems to be computed in the background page (based on some data received from the content scripts, e.g. the selectors related to the elements to hide) and then injected as strings of css code passed to the browser.tabs.insertCSS API call.

After being sent to the content process, this cssCode property is converted (in ExtensionContent.jsm) into a data url, so that the related stylesheet can be preloaded and cached as the other content scripts' stylesheets created from the extension urls:

- https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/toolkit/components/extensions/ExtensionContent.jsm#376-378

Once we convert the cssCode property into a data url, we could clear the cssCode so that it can be garbage collected and then reduce the amount of memory used in the content process while the content script has not been removed yet

(e.g. each one of these cssCode strings injected by adblock plus seems to be between 400Kb and 600Kb, and so with an higher number of tabs, each with a certain number of subframes, the sum of these string sizes can become an interesting amount of memory which we could spare).

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review commit: https://reviewboard.mozilla.org/r/202882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202882/

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

Review commit: https://reviewboard.mozilla.org/r/202884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202884/

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931755 [details]
Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

https://reviewboard.mozilla.org/r/202884/#review208210

::: browser/components/extensions/test/browser/browser_ext_tabs_insertCSS.js:55
(Diff revision 1)
>            // User has higher importance
>            return browser.tabs.insertCSS({
>              code: "* { background: rgb(100, 100, 100) !important }",
>              cssOrigin: "user",
>            }).then(r => browser.tabs.insertCSS({
> -            code: "* { background: rgb(42, 42, 42) !important }",
> +            code: "* { background: rgb(44, 44, 44) !important }",

The changes applied to this test are not needed by the other patch attached to this mozreview request,
but I noticed it while running these tests with the other patch applied. 

It seems that this test was already raising the following exception (but this exception doesn't make the test to fail), e.g. here is the exception raised by this test on try (without any of the changes applied in this mozreview request):

- https://treeherder.mozilla.org/logviewer.html#?job_id=147487092&repo=try&lineNumber=2728-2758

and it seems to be due to trying to inject the same preloaded stylesheet twice with the same stylesheet type on the same document.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931755 [details]
Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

https://reviewboard.mozilla.org/r/202884/#review208224

::: browser/components/extensions/test/browser/browser_ext_tabs_insertCSS.js:55
(Diff revision 1)
>            // User has higher importance
>            return browser.tabs.insertCSS({
>              code: "* { background: rgb(100, 100, 100) !important }",
>              cssOrigin: "user",
>            }).then(r => browser.tabs.insertCSS({
> -            code: "* { background: rgb(42, 42, 42) !important }",
> +            code: "* { background: rgb(44, 44, 44) !important }",

This test fails 100% for me when I run it locally.

Based on the error value[1] this may actually be NS_ERROR_INVALID_ARG returned from addSheet[2].  Without fully understanding this, it looks like a sheet can only be associated with a single document, but that our cssCache code may allow using a sheet with multiple documents.  If that is the case, I wonder if there is some logic issue in our cssCache code that maybe should be fixed.  I'd like to get clearer on that, maybe a followup bug to look into it.

For now, I'm find landing a fix for the test.

[1] https://searchfox.org/mozilla-central/source/xpcom/base/ErrorList.py#137
[2] https://searchfox.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#3769

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

https://reviewboard.mozilla.org/r/202882/#review208226

::: toolkit/components/extensions/ExtensionContent.jsm:227
(Diff revision 1)
>      if (matcher.wantReturnValue) {
>        this.compileScripts();
>        this.loadCSS();
>      }
>  
>      this.requiresCleanup = !this.removeCss && (this.css.length > 0 || matcher.cssCode);

I think this change is going to break this code path in an unexpected and silent way.  In loadCSS above, which uses the getter, you would nullify matcher.cssCode.  Also see Script.cleanup below.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/1-2/

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931755 [details]
Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202884/diff/1-2/

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1420485 - Added new tabs.insertCSS test case to ensure that the injected CSS are cleaned up.

Review commit: https://reviewboard.mozilla.org/r/203140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203140/

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1420485 - Fix browser.test.notifyFailure is not defined in tabs.insertCSS/removeCSS tests.

This patch fix a type in the tests related to tabs.insertCSS/removeCSS,
the test function is actually named browser.test.notifyFail.

Review commit: https://reviewboard.mozilla.org/r/203142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203142/

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1420485 - Fix ProxyMessenger not cleaned up when all the messageManagers listeners have been removed.

This patch ensures that the ProxyMessenger is cleaned up when the extension unloaded and the
targets without any listeners cleared from the MessageChannel's messageManagers map.

The browser_ext_tabs_insertCSS.js test file contains assertions that checks that the MessageChannel's
messageManagers map have been cleared when the test extension is unloaded, but these assertions
are always failing when the test file is the only executed test, but the assertions are verified
when the test file is running after any other test file, the reason is that the MessageChannel.messageManagers.size is initially 1, but it is going to be 3 as soon as the ProxyMessenger is initialized.

This patch ensures that we cleanup the ProxyMessenger when it is unused, and that the MessageChannel
clears from its map the targets that do not have any subscribed listeners anymore.

Review commit: https://reviewboard.mozilla.org/r/203144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203144/

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931755 [details]
Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202884/diff/2-3/

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/2-3/

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/3-4/

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932090 [details]
Bug 1420485 - Fix ProxyMessenger not cleaned up when all the messageManagers listeners have been removed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203144/diff/1-2/

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932088 [details]
Bug 1420485 - Added new tabs.insertCSS test case to ensure that the injected CSS are cleaned up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203140/diff/1-2/

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931755 [details]
Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202884/diff/3-4/

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932089 [details]
Bug 1420485 - Fix browser.test.notifyFailure is not defined in tabs.insertCSS/removeCSS tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203142/diff/1-2/

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/4-5/

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8932089 [details]
Bug 1420485 - Fix browser.test.notifyFailure is not defined in tabs.insertCSS/removeCSS tests.

https://reviewboard.mozilla.org/r/203142/#review208676

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8932088 [details]
Bug 1420485 - Added new tabs.insertCSS test case to ensure that the injected CSS are cleaned up.

https://reviewboard.mozilla.org/r/203140/#review208688

::: browser/components/extensions/test/browser/browser_ext_tabs_insertCSS.js:153
(Diff revision 2)
> +  is(unloadedStyles[0], "rgba(0, 0, 0, 0)", "The injected CSS code has been removed as expected");
> +  is(unloadedStyles[1], "rgb(0, 0, 0)", "The injected CSS file has been removed as expected");

seems like this part could just be added to the test above.

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/5-6/

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

https://reviewboard.mozilla.org/r/202882/#review209116

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Just noting that users are reporting problems in release 57 due to this.  Not sure if its an uplift candidate for point release or not.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Personally I'm fine with getting it into 58.  If someone wants to push this for a point release on 57, I'd like a second reviewer.  It may also need a 57-specific patch (not sure).

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

[Tracking Requested - why for this release]:

I guess we can ask release management for their opinion.  Normally I would think beta-58 would be fine, but since we're pushing the fast-with-low-memory angle in quantum maybe there is a greater desire to ship this kind of fix.

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/6-7/

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

Here is some numbers collected from about:memory on Firefox running with "AdBlock Plus 3.0.2" installed (without any custom configuration, only the standard filters enabled) and two tabs with "https://nytimes.com" loaded and Firefox executed using './mach run --setpref "dom.ipc.processCount=1"' (to force the Firefox instance to have a single webcontent process and make it easier to compare the memory usage of the two instances, with and without the patches attached to this issue). 

Follows a summary from the "Explicit Allocations" on the WebContent process, with and without the attached patches applied.

WebContent "Explicit Allocations" without Bug 1420485 patches:

404.32 MB (100.0%) -- explicit
├──202.61 MB (50.11%) -- window-objects
│  ├──102.15 MB (25.26%) ++ top(https://www.nytimes.com/, id=2147483711)
│  ├───95.60 MB (23.64%) ++ top(https://www.nytimes.com/?WT.z_jog=1&hF=t&vS=undefined, id=2147483649)
│  └────4.87 MB (01.20%) ++ top(about:newtab, id=2147483708)
├──102.98 MB (25.47%) -- js-non-window
│  ├───74.34 MB (18.39%) -- zones
│  │   ├──62.81 MB (15.54%) -- zone(0x7f7969fcb000)
│  │   │  ├──52.00 MB (12.86%) -- strings
│  │   │  │  ├──24.00 MB (05.94%) -- string(length=422394, copies=48, "data:text/css;charset=utf-8,.abp_ob_exist%2C%20..." (truncated))
│  │   │  │  │  ├──24.00 MB (05.94%) ── malloc-heap/latin1
│  │   │  │  │  └───0.00 MB (00.00%) ── gc-heap/latin1
│  │   │  │  ├──14.81 MB (03.66%) -- string(length=320526, copies=48, ".abp_ob_exist, ..." (truncated))
│  │   │  │  │  ├──14.81 MB (03.66%) ── malloc-heap/latin1
│  │   │  │  │  └───0.00 MB (00.00%) ── gc-heap/latin1
│  │   │  │  ├───8.68 MB (02.15%) ++ (7 tiny)
│  │   │  │  └───4.50 MB (01.11%) -- string(length=422121, copies=9, "data:text/css;charset=utf-8,.abp_ob_exist%2C%20..." (truncated))
│  │   │  │      ├──4.50 MB (01.11%) ── malloc-heap/latin1
│  │   │  │      └──0.00 MB (00.00%) ── gc-heap/latin1
│  │   │  └──10.82 MB (02.67%) ++ (18 tiny)
│  │   ├───6.92 MB (01.71%) ++ (3 tiny)
│  │   └───4.60 MB (01.14%) ++ zone(0x7f796d434000)
│  ├───25.14 MB (06.22%) ++ runtime
│  └────3.50 MB (00.87%) ++ gc-heap
├───44.33 MB (10.96%) ── heap-unclassified
├───33.38 MB (08.26%) ++ heap-overhead
├───10.79 MB (02.67%) ++ images
└───10.22 MB (02.53%) ++ (18 tiny)

WebContent "Explicit Allocations" with Bug 1420485 patches:

263.09 MB (100.0%) -- explicit
├──131.83 MB (50.11%) -- window-objects
│  ├───64.36 MB (24.46%) ++ top(https://www.nytimes.com/?WT.z_jog=1&hF=t&vS=undefined, id=2147483649)
│  ├───63.05 MB (23.96%) ++ top(https://www.nytimes.com/, id=2147483711)
│  └────4.42 MB (01.68%) -- top(about:newtab, id=2147483708)
│       ├──2.93 MB (01.11%) ++ active/window(about:newtab)
│       └──1.49 MB (00.57%) ++ js-zone(0x7f2211a72000)
├───42.14 MB (16.02%) -- js-non-window
│   ├──21.01 MB (07.99%) ++ runtime
│   ├──18.71 MB (07.11%) ++ zones
│   └───2.42 MB (00.92%) ++ gc-heap
├───38.49 MB (14.63%) ── heap-unclassified
├───29.39 MB (11.17%) ++ heap-overhead
├───10.70 MB (04.07%) ++ images
├────7.40 MB (02.81%) ++ (17 tiny)
└────3.15 MB (01.20%) ++ atom-tables

I pasted an expanded version of the memory report for the Firefox instances without the Bug 1420485 patches because it shows where the memory for the cssCode strings and their related generated data URIs more clearly (while in the second memory report with the patches applies, they are not really visible anymore, and so expanding the js-non-window sections wasn't that useful).

I'm also going to attach an anonymized memory report collected and saved from the above two instances (with and without the attached patches applied) on the same scenario (AdBlock Plus 3.0.2 installed and two https://nytimes.com opened).

Comment 29

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: memory-report-without-Bug1420485-patches.json.gz

Comment 30

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: memory-report-with-Bug1420485-patches.json.gz

Comment 31

[Andrew McCreight [:mccr8]]

FWIW, a user in bug 1425205 seems to be seeing almost 7GB (!) of strings from this issue (or a similar one).

Comment 33

[Kris Maglione [:kmag]]

Not sure why I'm needinfoed. What exactly is the question?

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Once this lands let's be sure to verify the fix in nightly and beta. Tracking for 59/60.

Comment 36

[Kris Maglione [:kmag]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

https://reviewboard.mozilla.org/r/202882/#review221386

::: toolkit/components/extensions/ExtensionContent.jsm:49
(Diff revision 7)
>    DefaultMap,
>    DefaultWeakMap,
>    defineLazyGetter,
>    getInnerWindowID,
>    getWinUtils,
> +  stringToCryptoHash,

Nit: Please keep sorted.

::: toolkit/components/extensions/ExtensionContent.jsm:203
(Diff revision 7)
> +  set(md5sum, cssCode) {
> +    if (this.has(md5sum)) {
> +      // This cssCode have been already cached, no need to create it again.
> +      return;
> +    }

This is a *really* weird way to do this. Aside from the fact that this is a DefaultMap that has no getter function defined, you're calling `.set()` with a string value, and winding up setting it to a promise instead (or not changing its value at all).

Please add a function with a name like `getChecksum` that takes a CSS string, hashes it, loads the CSS if necessary, and then returns the checksum.

And please add a default getter function that throws when called rather than overriding `get`.

::: toolkit/components/extensions/ExtensionContent.jsm:203
(Diff revision 7)
> +    super(CSSCODE_EXPIRY_TIMEOUT_MS);
> +    this.sheetType = sheetType;
> +    this.extension = extension;
> +  }
> +
> +  set(md5sum, cssCode) {

Please just call this `checksum`, or `hash`, or something like that. Whether it's MD5 or some other hash function shouldn't matter.

::: toolkit/components/extensions/ExtensionContent.jsm:217
(Diff revision 7)
> +    });
> +
> +    // Save the uri on the promise, so that the preloaded stylesheet can be cleared
> +    // synchronously during the extension shutdown (without awaiting the cached promise to get the
> +    // generated data url).
> +    value.uri = uri;

This is not ideal. It means we need to keep another copy of the source URI alive.

::: toolkit/components/extensions/ExtensionContent.jsm:246
(Diff revision 7)
> +      let docs = ChromeUtils.nondeterministicGetWeakSetKeys(sheetCacheDocuments.get(promise));
> +      if (docs.length) {
> +        return;
> +      }

I'd really rather we don't duplicate this code. All three of these functions can be shared with CSSCache, so please just extend that class instead.

::: toolkit/components/extensions/ExtensionContent.jsm:291
(Diff revision 7)
> +defineLazyGetter(BrowserExtensionContent.prototype, "cryptoHashMD5", () => {
> +  return new CryptoHash("MD5");
> +});

CryptoHash objects are not meant to be reused. Please don't try to reuse them.

::: toolkit/components/extensions/ExtensionContent.jsm:311
(Diff revision 7)
>      this.cssOrigin = this.matcher.cssOrigin;
>  
>      this.cssCache = extension[this.cssOrigin === "user" ? "userCSS"
>                                                          : "authorCSS"];
> +    this.cssCodeCache = extension[this.cssOrigin === "user" ? "userCSSCode"
> +                                                        : "authorCSSCode"];

Nit: Please fix indentation.

::: toolkit/components/extensions/ExtensionContent.jsm:336
(Diff revision 7)
> +    const cryptoHashMD5 = this.extension.cryptoHashMD5;
> +    // NOTE: cryptoHash instance is reused, but it has to be re-initialized
> +    // before using it on a new string (on the contrary it will raise NS_ERROR_NOT_INITIALIZED
> +    // error).
> +    cryptoHashMD5.initWithString("MD5");
> +    this.cssCodeMD5 = stringToCryptoHash(cssCode, cryptoHashMD5);

Again, the algorithm doesn't matter here.

Also, this property should be set to null in the constructor so that we don't change the shape of the object when we define it, and we should throw if we call this function when it already exists.

::: toolkit/components/extensions/ExtensionUtils.jsm:655
(Diff revision 7)
> +  cryptoHash.updateFromStream(new StringInputStream(text, text.length), -1);
> +  return cryptoHash.finish(true);

This isn't going to work correctly if the string has multi-byte characters. You should be using TextEncoder() to create an ArrayBuffer, and then calling .update() with that array buffer.

Also, cryptoHash instances aren't meant to be re-used. If you want a utility function that hashes a string, it should just take a string and the name of a hash function, and do the rest itself. Whether it's implemented using nsICryptoHash shouldn't matter to the caller.

And, that being said, it really *shouldn't* use nsICryptoHash. WebCrypto would be better: https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest

That API is asynchronous, but since we can actually handle asynchrony here, that's actually a bonus.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Seems that it is a bit too late for 58.

Comment 38

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932088 [details]
Bug 1420485 - Added new tabs.insertCSS test case to ensure that the injected CSS are cleaned up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203140/diff/2-3/

Comment 39

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931755 [details]
Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202884/diff/4-5/

Comment 40

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932089 [details]
Bug 1420485 - Fix browser.test.notifyFailure is not defined in tabs.insertCSS/removeCSS tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203142/diff/2-3/

Comment 41

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/7-8/

Comment 42

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1420485 - Fix 'message manager count' failure when browser_ext_tabs_insertCSS.js is executed first.

Review commit: https://reviewboard.mozilla.org/r/216256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216256/

Comment 43

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/8-9/

Comment 44

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

https://reviewboard.mozilla.org/r/202882/#review221386

Thanks Kris for the feedback comments, in the updated patch all the suggested changes have been applied. 

(and I've also added a new small patch to fix a 'message manager count' failure when browser_ext_tabs_insertCSS.js is executed first, which is due to the MessageChannel's messageManager listeners subscribed by ProxyMessenger.init when the first extension is being started and never unsubscribed, otherwise TV is going to fail when the patch lands because only the modified tests are executed with "./mach test --verify").

Follows some additional comments related to the applied changes.

> This is a *really* weird way to do this. Aside from the fact that this is a DefaultMap that has no getter function defined, you're calling `.set()` with a string value, and winding up setting it to a promise instead (or not changing its value at all).
> 
> Please add a function with a name like `getChecksum` that takes a CSS string, hashes it, loads the CSS if necessary, and then returns the checksum.
> 
> And please add a default getter function that throws when called rather than overriding `get`.

I've renamed into `addCSSCode`the method which adds a CSSCode stylesheet to the cache map (it was named `set` in the previous version.

I've also refactored the two CSS cache classes so that they inherits from a BaseCSSCache class which provides the methods shared between CSSCache and CSSCodeCache (and redefined CSSCodeCache to inherits from BaseCSSCache and provide a default getter which throws when called for a non cached CSSCode stylesheet).

> This is not ideal. It means we need to keep another copy of the source URI alive.

It seems that we can't currently remove the preloaded stylesheet without its uri, in the meantime I've applied some change so that uri is resolved by the promise along with the preloaded sheet (so that, if in a followup we make it possible to remove the stylesheet using the preloaded stylesheet instance, there should be a smaller amount of changes needed to achieve it).

(as a side note, if I'm not reading it wrong, the preloaded stylesheet also keeps a reference to the uri: https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/layout/style/PreloadedStyleSheet.cpp#21 https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/layout/style/PreloadedStyleSheet.h#80)

> I'd really rather we don't duplicate this code. All three of these functions can be shared with CSSCache, so please just extend that class instead.

Yeah, I wasn't happy about this duplication, these methods are now shared between the two classes (by extending the BaseCSSCache class).

> This isn't going to work correctly if the string has multi-byte characters. You should be using TextEncoder() to create an ArrayBuffer, and then calling .update() with that array buffer.
> 
> Also, cryptoHash instances aren't meant to be re-used. If you want a utility function that hashes a string, it should just take a string and the name of a hash function, and do the rest itself. Whether it's implemented using nsICryptoHash shouldn't matter to the caller.
> 
> And, that being said, it really *shouldn't* use nsICryptoHash. WebCrypto would be better: https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest
> 
> That API is asynchronous, but since we can actually handle asynchrony here, that's actually a bonus.

I've rewritten this helper to use WebCrypto (and turned it into an async function as part of the change).

Comment 45

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8946291 [details]
Bug 1420485 - Fix 'message manager count' failure when browser_ext_tabs_insertCSS.js is executed first.

https://reviewboard.mozilla.org/r/216256/#review222066

::: browser/components/extensions/test/browser/browser_ext_tabs_insertCSS.js:108
(Diff revision 1)
>  
>    await extension.unload();
>  
>    await BrowserTestUtils.removeTab(tab);
>  
> +  // ProxyMessager.init adds MessageChannel listeners for Services.mm and Services.ppmm,

The long sentences are a bit hard to digest, here's something shorter:

When the first extension is started, ProxyMessenger.init adds MessageChannel listeners for Services.mm and Services.ppmm, and they are never unsubscribed.  We have exclude them after the extension has been unloaded to get an accurate test.

Comment 46

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8946291 [details]
Bug 1420485 - Fix 'message manager count' failure when browser_ext_tabs_insertCSS.js is executed first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216256/diff/1-2/

Comment 47

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/9-10/

Comment 48

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

https://reviewboard.mozilla.org/r/202882/#review222408

::: toolkit/components/extensions/ExtensionContent.jsm:721
(Diff revisions 6 - 10)
>        let context = extensions.get(extension);
>        if (context) {
> -        context.close();
> +        // We are using `context.close(true)`, so that the context
> +        // knows that the context is being closed because the
> +        // extension is shutting down and it clear everything that
> +        // have been cached for it (e.g. the CSS and Script caches).

structure is weird.  Is something about this also in the jsdoc for context.close?

"Passing true to context.close causes the caches for this context to be cleared."

::: toolkit/components/extensions/ExtensionContent.jsm:324
(Diff revision 10)
>    preload() {
>      this.loadCSS();
>      this.compileScripts();
>    }
>  
> -  cleanup(window) {
> +  cleanup(window, isExtensionShutdown = false) {

I think I'd prefer forceCacheClear rather than isExtensionShutdown.  That would be more understandable.  The arguement should describe *what it does* rather than *why it is happening*.

::: toolkit/components/extensions/ExtensionContent.jsm:616
(Diff revision 10)
>      if (script.requiresCleanup) {
>        this.scripts.push(script);
>      }
>    }
>  
> -  close() {
> +  close(isExtensionShutdown) {

same here with the argument.   While it is used during shutdown, what it does is forces the cache to clear.

Comment 49

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #45)
> Comment on attachment 8946291 [details]

> The long sentences are a bit hard to digest, here's something shorter:
> 
> When the first extension is started, ProxyMessenger.init adds MessageChannel
> listeners for Services.mm and Services.ppmm, and they are never
> unsubscribed.  We have exclude them after the extension has been unloaded to
> get an accurate test.

And there I go correcting your comments with my wonderful grammar.

"We have to exclude"

Comment 50

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8946291 [details]
Bug 1420485 - Fix 'message manager count' failure when browser_ext_tabs_insertCSS.js is executed first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216256/diff/2-3/

Comment 51

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/10-11/

Comment 52

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

https://reviewboard.mozilla.org/r/202882/#review222408

> structure is weird.  Is something about this also in the jsdoc for context.close?
> 
> "Passing true to context.close causes the caches for this context to be cleared."

Thanks for this comment! Besides applying the suggested tweak to the inline comment, I also moved the "script cleanup with forceCacheClear" part into its own method (which is used inside close without the forceCacheClear parameter, and it is also used here with forceCacheClear true to ensure that the cached are cleared when the extension shutdown).

I preferred to move this part into its own method because none of the classes that inherits from BaseContext takes any parameter, and so `ContentScriptContextChild` would be the only one and it could become confusing.

> I think I'd prefer forceCacheClear rather than isExtensionShutdown.  That would be more understandable.  The arguement should describe *what it does* rather than *why it is happening*.

`forceCacheClear` sounds great to me, change applied.

Comment 53

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932088 [details]
Bug 1420485 - Added new tabs.insertCSS test case to ensure that the injected CSS are cleaned up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203140/diff/3-4/

Comment 54

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931755 [details]
Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202884/diff/5-6/

Comment 55

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932089 [details]
Bug 1420485 - Fix browser.test.notifyFailure is not defined in tabs.insertCSS/removeCSS tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203142/diff/3-4/

Comment 56

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8946291 [details]
Bug 1420485 - Fix 'message manager count' failure when browser_ext_tabs_insertCSS.js is executed first.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216256/diff/3-4/

Comment 57

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202882/diff/11-12/

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66016885b0e9
Added new tabs.insertCSS test case to ensure that the injected CSS are cleaned up. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/b3352470d670
Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/e96652c3f61d
Fix browser.test.notifyFailure is not defined in tabs.insertCSS/removeCSS tests. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c79f381e64d6
Fix 'message manager count' failure when browser_ext_tabs_insertCSS.js is executed first. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/5a25d11f62db
Reduce memory usage related to the tabs.insertCSS cssCode urls. r=mixedpuppy

Comment 59

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/66016885b0e9
https://hg.mozilla.org/mozilla-central/rev/b3352470d670
https://hg.mozilla.org/mozilla-central/rev/e96652c3f61d
https://hg.mozilla.org/mozilla-central/rev/c79f381e64d6
https://hg.mozilla.org/mozilla-central/rev/5a25d11f62db

Comment 60

[Matthew Kogan]

I am the user who reported https://bugzilla.mozilla.org/show_bug.cgi?id=1425205, which was eventually marked as a duplicate of this bug. I am currently having to restart Firefox at least twice a day due to memory usage climbing out of control. Can I please ask that the fix be backported to at least Firefox 59, and preferably to a 58 point release. If I weren't such a fan of Firefox, I would certainly have switched browser by now because of this issue, and I really don't want others to do so.

Comment 61

[Eric Rahm [:erahm]]

(In reply to Matthew Kogan from comment #60)
> I am the user who reported
> https://bugzilla.mozilla.org/show_bug.cgi?id=1425205, which was eventually
> marked as a duplicate of this bug. I am currently having to restart Firefox
> at least twice a day due to memory usage climbing out of control. Can I
> please ask that the fix be backported to at least Firefox 59, and preferably
> to a 58 point release. If I weren't such a fan of Firefox, I would certainly
> have switched browser by now because of this issue, and I really don't want
> others to do so.

Luca, can you nom this for beta after it bakes for a bit?

As Matthew noted it's a pretty huge memory regression for some folks and I'd like to see it in release if we happen to do another dot release. Maybe nom it for release as well and see what rel-man has to say? I can understand if it's too big of a change to warrant that, but it's worth a shot.

Comment 62

[Andy McKay]

I don't think we should pushing this to release, it seems like a pretty big change. I'm really keen to see how it bakes on Nightly and suggest if nothing blows up, we uplift to Beta ASAP.

Matthew, if you could give it a try on Nightly and let us know how it goes, that would give us more confidence that the patch works.

Comment 63

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #61)
> Luca, can you nom this for beta after it bakes for a bit?

Sure, this bug is already queued to be verified by QA asap, and once it bakes on Nightly a bit and 
we have verified that the change is having the expected impact of this memory usage issue (and that
the adblockers affected are working as expected) I'm going to nominate it for an uplift to beta.

> As Matthew noted it's a pretty huge memory regression for some folks and I'd
> like to see it in release if we happen to do another dot release. Maybe nom
> it for release as well and see what rel-man has to say? I can understand if
> it's too big of a change to warrant that, but it's worth a shot.

I agree with Andy on this, let this change to bake a bit on Nightly (so that we can also ensure that it doesn't introduce regressions), be verified by QA and aim to uplift it to beta asap first.

I'm going to add a proposed set of steps for the QA test plan, so that QA can start to verify this change asap as planned.

Comment 64

[Luca Greco [:rpl] [:luca] [:lgreco]]

Follows a proposed set of steps to reproduce the issue and verify that the changes applied by the patches from this issue are being able to improve the memory usage related to the stylesheets injected using tabs.insertCSS:

- install Firefox (a Nightly with the patches applied and a Beta without these changes)

- set dom.ipc.processCount set to 1 (to get more easily comparable memory reports, 
  and then repeat the tests with 4, the default value)

- install AdBlock Plus / install uBlock Origin / (other common extensions/adblockers that presented the same issue, 
  especially if mentioned in the comments and memory reports from the bugzilla issues marked as duplicate of this one)

- open a new tab on about:memory and collect a memory report

- open a reasonably high number of tabs on websites that seems to trigger the issue more easily
  and wait them to be fully loaded 
  (e.g. searches on bing.com have been reported as something that trigger the issue in Bug 1430599, 
  and/or other websites with an high number of ads and iframes, especially the ones mentioned in the
  comments and memory reports from the bugzilla issues marked as duplicates of this one)

- collect a new memory report from about:memory

Notes from some of the issues marked as duplicates:

- Bug 1425205 comment 0 mentions that "Left my Firefox windows and tabs open overnight" is what usually triggers the issue for the reporter (addons installed AdBlock Plus, Flagfox, NoScript and Video DownloadHelper)

- Bug 1430599 comment 0 mentions that "Just go to bing.com and do repeated searches" is what usually triggers the issues for the reporter (uBlock Origin, and other 3 extensions)

Comment 65

[marius.santa]

Attachment: ublock origins.PNG

Comparison between Firefox 59.0a1 (20171124220317) and Firefox 60.0a1 (20180201100326) on Windows 10 64 Bit.
The difference in memory allocated is visible across multiple websites and multiple add-blocker extensions.

Comment 66

[marius.santa]

Attachment: adblocker plus.PNG

Comparison between Firefox 59.0a1 (20171124220317) and Firefox 60.0a1 (20180201100326) on Windows 10 64 Bit.
The difference in memory allocation difference is higher than with uBlock Origin but some error messages are displayed, Is this due to the nature of the site or is  this a related bug?

Comment 67

[Luca Greco [:rpl] [:luca] [:lgreco]]

The values related to the about:memory warning message don't seem to be visible in the screenshot,
do you mind to dump this report to a file (from about:memory) and attach it to this issue so that I can take a look?

Is the warning message still there if you collect another memory report from the same Firefox instance after a while?
(I'm wondering if the issue can be related to the timing of the memory usage data collection, or if it is consistent and it produces the same message even when collecting the memory usage data multiple times).

Comment 68

[marius.santa]

Attachment: memory-report.json.gz

Redone the measurements and attached the logs.

Comment 69

[marius.santa]

Please let me know if the issue from the logs is a separate issue or an issue at all so that I can mark the bug as verified for Nightly.

Comment 70

[Luca Greco [:rpl] [:luca] [:lgreco]]

The attached memory report doesn't seem to present the warning message mentioned in comment 66, and so it seems to confirm that is something that happened because of the timing of the memory usage data collection.

I briefly looked at the memory report content and most of the memory usage in the content processes is coming from the 'explicit/window-objects' section (which is related to the of the loaded webpages, and so it is not coming from the huge amount of strings from 'explicit/js-non-window' as in the memory reports attached to the other issues that have been marked as duplicate of this one), which seems to confirm that the patches had the expected impact on the memory usage, e.g. from 'Web Content (pid 16208)' part of the attached memory report:

1,165.73 MB (100.0%) -- explicit
├────790.48 MB (67.81%) -- window-objects
│    ├──198.71 MB (17.05%) ++ top(... id=4294967485)
│    ├──197.89 MB (16.98%) ++ top(..., id=4294967313)
│    ├──195.21 MB (16.75%) ++ top(..., id=4294967347)
│    ├──194.94 MB (16.72%) ++ top(..., id=4294967411)
│    └────3.73 MB (00.32%) ++ top(none)/detached/window(...)
├────191.07 MB (16.39%) ++ js-non-window
├─────84.50 MB (07.25%) ── heap-unclassified
├─────68.18 MB (05.85%) -- heap-overhead
│     ├──55.56 MB (04.77%) ── bin-unused
│     ├──11.77 MB (01.01%) ── bookkeeping
│     └───0.85 MB (00.07%) ── page-cache
└─────31.51 MB (02.70%) ++ (20 tiny)

By collecting and attaching a similar memory report from a Firefox instance which doesn't have the fix applied, I can also compare the two memory reports. 

In comment 64 I was also suggesting to collect the reports on Firefox profiles with dom.ipc.processCount set to 1 in about:config, mainly because it forces Firefox to use only 1 child process for the webpages, which would make it easier to compare the collected memory reports (because all the opened tabs will be loaded in a single process instead of being distributed over 4 child processes).

Comment 71

[Matthew Kogan]

(In reply to Andy McKay [:andym] from comment #62)
> Matthew, if you could give it a try on Nightly and let us know how it goes,
> that would give us more confidence that the patch works.

It's a lot better. I've had it running for about 20 hours now without restarting, with the same extensions and tabs, and the memory usage is much more reasonable. There's still a zig-zag pattern on the Task Manager memory usage graph, as I attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1425205, but it's between about 6.5GB and about 7.5GB now, on a machine with 8GB of RAM. That could well be a separate bug, though.

Comment 72

[marius.santa]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1420485#c65 and retesting 60.0a1 (20180201100326) on Mac OS  X 10.13.2 marking verified for firefox60.

Comment 73

[Andy McKay]

Thank you for testing that out Matthew and Marius. Luca, can you ask for beta uplift please?

Comment 74

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Approval Request Comment

[Feature/Bug causing the regression]:
browser.tabs.insertCSS WebExtensions API,
which is a feature pretty often used by adblocking extensions to hide
the part of the webpage that is related to the blocked ads (by generating
a pretty big CSS string based on the active filters and the content of the
page and then inject it into the target tabs using browser.tabs.insertCSS).

The stylesheets related to this CSS strings are preloaded and then cached,
this css cache was using the stylesheet url as a key to retrieve the
preloaded stylesheet from the Map of the preloaded stylesheet,
and for a CSS injected as a string this url was the data url 
generated from the content of the stylesheet, and so these strings
were using at least twice the amount of memory occupied by the CSS content itself.

[User impact if declined]:
If declined the users of adblockers that are experiencing "high memory usage" issues will still experience the same issue (and the adblockers extensions are
usually one of the extension types commonly used by users with installed extensions).

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, it would helpful to also explicitly verify that the fix has the expected impact and that the adblockers are working as expected, using the same steps used to verify it on Nightly.

[List of other uplifts needed for the feature/fix]:
Just the patches part of this bugzilla issue. 

[Is the change risky?]:
Medium/Low risk

[Why is the change risky/not risky?]:
Most of the patches attached are applied on tests and so they are not risky, 
but the one that contains the actual fix contains changes applied to WebExtensions internals that implements the content script injection, 
which is a feature that is used by many of the WebExtensions.

For this reason the change has a Medium/Low risk level ("low" because it can only affects extensions and users with extensions installed, "medium" because many extensions uses the content scripts and so it can potentially effect an high number of extensions in case of a regression introduced by this change).

[String changes made/needed]:
None

Comment 75

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931755 [details]
Bug 1420485 - Fix an exception raised by browser_ext_tabs_insertCSS.js on injecting the same cached preloaded stylesheet twice.

Approval Request Comment

Same as Comment 74

Comment 76

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932088 [details]
Bug 1420485 - Added new tabs.insertCSS test case to ensure that the injected CSS are cleaned up.

Approval Request Comment

Same as Comment 74

Comment 77

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8932089 [details]
Bug 1420485 - Fix browser.test.notifyFailure is not defined in tabs.insertCSS/removeCSS tests.

Approval Request Comment

Same as Comment 74

Comment 78

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8946291 [details]
Bug 1420485 - Fix 'message manager count' failure when browser_ext_tabs_insertCSS.js is executed first.

Approval Request Comment

Same as Comment 74

Comment 79

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8931754 [details]
Bug 1420485 - Reduce memory usage related to the tabs.insertCSS cssCode urls.

Seems like a high-priority fix that we'll want as much bake time as possible, so let's make sure this get into tomorrow's 59b8 build.

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/24e9a78c27eb
https://hg.mozilla.org/releases/mozilla-beta/rev/dbc6d70aff87
https://hg.mozilla.org/releases/mozilla-beta/rev/a20b24dc9389
https://hg.mozilla.org/releases/mozilla-beta/rev/05fd88206d24
https://hg.mozilla.org/releases/mozilla-beta/rev/169702665656

Comment 81

[marius.santa]

Attachment: good mem.PNG

Tested and verified on Firefox 59.0b8 (20180208193705) in Windows 10 64Bit and Mac OS  X 10.13.2, using Adblock Plus, uBlock, AdBlocker Ultimate, etc.

Comment 82

[marius.santa]

Attachment: memory-report0209.json.gz

Attached the memory logs.

Comment 83

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5c3153701c6c4245499dbcfc833a1ed6b45ca3c
Bug 1420485: Follow-up: Remove unnecessary single-use stringToCryptoHash function. r=me

Comment 84

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d5c3153701c6