1296898 - Develop experimental WebExtension API for getting Firefox's memory usage

Status: RESOLVED WONTFIX

(WebExtensions :: Experiments, enhancement, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Prior art that we might want to adopt and then leapfrog: https://developer.chrome.com/extensions/system_memory

Additionally, note that a JSM is being developed in bug 1255843 which should add some useful shortcuts for getting memory measurements for content processes. This experimental API could perhaps use that JSM instead of talking to the nsIMemoryReporterManager directly.

Use cases for this API include:

1) Getting notifications about things like memory pressure so that WebExtensions can attempt to clear caches
2) Adding UI to Firefox to report detailed memory usage statistics

More futuristically, billm also talked at one point about a separate experimental WebExtension API for helping Firefox decide which content process to open particular tabs in for e10s-multi. Memory usage data might be useful for making such decisions in that theoretical API.

Comment 1

[Nicholas Nethercote [inactive]]

In case it's relevant: xpcom/base/nsIMemory.idl has some stuff related to memory pressure notifications.

Comment 2

[:shell escalante]

Hi Nick, is someone on your team interested in doing something in this area?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Note that this was an idea that billm proposed in London, and I wanted to make sure it wasn't forgotten.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to :shell escalante from comment #2)
> Hi Nick, is someone on your team interested in doing something in this area?

I'm not 100% sure this really was intended to be for me, but in case it was: sorry, not at the moment.

Comment 5

[Eric Rahm [:erahm]]

I put together a draft of a WebExtension experiment at: https://github.com/EricRahm/memory-api-experiment

It includes a `getInfo` method that's just a pass-through to `Memory.summary`, so that means it just includes rss and uss for all processes. What's nice is we can update Memory.jsm to add more info without messing the WebExtension API (I have a draft patch for adding ghost window counts).

Addtionally it has an `onLowMemory` event that hooks into the internal 'memory-pressure' event.

I explicitly didn't add 'total system memory' and 'system memory used' because a) we don't have a reporter for that currently and b) I'm not sure that'd pass the privacy test.

mconley would you mind providing some feedback on what I have implemented or helping me find a more appropriate reviewer?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I like it - I also like that we're considering exposing more than the Chromium API.

It gets a thumbs up from me - though you'll probably want to talk to the WebExtensions team in general to try to get this integrated. They're the deciders on what gets adopted as official APIs.

Tentatively redirecting to :aswan to see if he has any feedback.

Comment 7

[Andrew Swan [:aswan]]

From a quick glance, this looks good, thanks!  What's the longer-term plan here?  Should we land this directly in central so that extensions can use it without having to install a separate experiment?  (I or somebody else should take a more careful look through the implementation if we're going to do that, but from a high level there aren't any glaring problems)

Comment 8

[Florian Quèze [:florian]]

I removed Memory.jsm in bug 1474990, here is a link to the changeset for future reference if you need to re-add it here: https://hg.mozilla.org/mozilla-central/rev/6ee592ecc6ac

Comment 10

[Eric Rahm [:erahm]]

I'll see if we can revive this.

Comment 11

[Kris Wright :KrisWright]

Attachment: bug 1296898 - Memory api (pulled from erahm's experiment) as a part of the webextension api

Webextension api to get memory usage. Also brings back memory.jsm, as it is used by the api.

Comment 12

[Kris Wright :KrisWright]

Comment on attachment 9079115 [details]
bug 1296898 - Memory api (pulled from erahm's experiment) as a part of the webextension api

I've never looked at the web extension code before so I followed the example set for me by other apis in this directory, but in writing the test file it seems that I forgot to do something to implement the api. For reference, I tried creating an extension like this to test:

let testExtension = ExtensionTestUtils.loadExtension({
    background,
    manifest: {
      permissions: ["memory"],
    }
  });

And I got this warning, followed by a long list of permitted values/regexp ("memory" not included among them):

WARN	Loading extension '{6f5bc79b-8d6d-4242-84ed-e100654e69be}': Reading manifest: Error processing permissions.0: Value "memory" must either: ...

And then after it couldn't be loaded, any attempts to read into "memory" are undefined. I'm assuming I missed something or did something wrong when I added the file. Is there anything else I need to add to make the api usable?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 9079115 [details]
bug 1296898 - Memory api (pulled from erahm's experiment) as a part of the webextension api

Andrew should be better to get feedback from at the moment at least.

Comment 14

[Andrew Swan [:aswan]]

(In reply to Kris Wright :KrisWright from comment #12)

WARN Loading extension '{6f5bc79b-8d6d-4242-84ed-e100654e69be}': Reading manifest: Error processing permissions.0: Value "memory" must either: ...

Kris (M) covered this in his review:
https://phabricator.services.mozilla.com/D38536#inline-233381

Comment 15

[Shane Caraveo (:mixedpuppy)]

Is this meant to be a privileged api, or a public api? If the former, you need to make sure the patch does that (it doesn't currently). If the latter, FYI :Fallen

Comment 16

[Shane Caraveo (:mixedpuppy)]

IOW, the bug is titled that this is experimental, so be sure to make the permission privileged.

https://searchfox.org/mozilla-central/rev/4436573fa3d5c4311822090e188504c10c916c6f/toolkit/components/extensions/Extension.jsm#148

Comment 17

[Kris Wright :KrisWright]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)

IOW, the bug is titled that this is experimental, so be sure to make the permission privileged.

https://searchfox.org/mozilla-central/rev/4436573fa3d5c4311822090e188504c10c916c6f/toolkit/components/extensions/Extension.jsm#148

Once the extension was made privileged, I got this fail from the test_ext_permissions.js xpcshell test:
Permission 'memory' intentionally granted without prompting the user - false == true

I'm guessing I need to update this list here: https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/toolkit/components/extensions/test/xpcshell/test_ext_permissions.js#562
And then have a WebExtension peer review it. Is that correct?

Comment 18

[Shane Caraveo (:mixedpuppy)]

Yes, you will need to add it there. You should also include a small permission test specific to your api, there are several examples, here's one[1]

You can r? me for the extension api parts, and/or continue with :kmag who has already made review comments.

[1] https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/toolkit/components/extensions/test/xpcshell/test_ext_networkStatus.js#174

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Hi Folks, thanks for making some progress here. I have a few product questions before we push this API:

• Given the target now seems to be privileged extensions only, what internal add-ons will be using this?
• What team will be on the hook for maintaining this API?

Going back to the use cases in comment 0, it sounds like this was originally targeted as a public API. If that is the case, with the current implementation I'm concerned that a way for extensions to trigger the memory pressure event would be used by a bunch of add-ons thinking they can use this to "save memory" in Firefox.

Exposing detailed information about the memory configuration to add-ons publicly, I'd like to make sure we have use cases for that. The obvious one would be to allow an add-on to show a memory manager of sorts, but I rather think this is where about:memory should shine. Are there any additional cases? A low memory event would make sense publicly.

If there are no specific add-ons using this and there isn't a strong case for a public API, I'd prefer if we could keep this an experiment and not push it to m-c. Developers interested in fiddling with this can include that experiment within their extension.

Comment 20

[Eric Rahm [:erahm]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #19)

Exposing detailed information about the memory configuration to add-ons publicly

Unlike the Chrome version, this explicitly does not expose system memory information, just Firefox's memory usage.

If there are no specific add-ons using this and there isn't a strong case for a public API, I'd prefer if we could keep this an experiment and not push it to m-c. Developers interested in fiddling with this can include that experiment within their extension.

This was originally intended as public, although I'd expect there to be a memory permission that needs to be granted. There's a use case in bug 1351346, I'd also imagine it would be useful for various tab unloading add-ons.

re: GC, mccr8 weighed in on this previously in bug 1351346, comment 2:

I understand your concern about forceGC/forceCC, but I think it is mostly harmless in practice. I don't know how useful it actually is, though. Somebody can trigger GC and CC from about:memory if they really want to, but it isn't something you'll be doing all of the time.

So it's probably fine. I can see it being useful for folks writing fuzzers and experimenting with different memory reduction heuristics (like triggering a GC before moving on to unloading a tab).

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e400446aedc
Memory api (pulled from erahm's experiment) as a part of the webextension api r=mixedpuppy

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8e400446aedc

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

I'm sorry I didn't respond earlier, this bug went off my radar. These questions are still open:

• Given the target now seems to be privileged extensions only, what internal add-ons will be using this?
• What team will be on the hook for maintaining this API?

I understand how the API could be useful, though I do not understand why it needs to be in-tree, especially now that this is a privileged API. The extension could just as well include it as an experiment API. If Mozilla is developing an add-on like the gecko profiler that would need this kind of API I would reconsider, but keeping it around just because it could be useful doesn't seem like a good idea to me.

Given what I know now I'd prefer if we could back this out and wontfix the bug, let me know if there is something I am missing.

Comment 24

[Dorel Luca [:dluca]]

Backed out changeset 8e400446aedc (bug 1296898) at dev's request.

Backout:
https://hg.mozilla.org/integration/autoland/rev/976a5aa452f9dbf252135ad1617c01d2fd1fd253

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merge: https://hg.mozilla.org/mozilla-central/rev/976a5aa452f9

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

Thanks for the backout. I'm going to close this bug WONTFIX now, if there is an update on the questions from comment 23 I'm happy to reconsider.