680821 - Update Add-on sandbox to use new memory accounting options

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Josh Matthews [:jdm]]

The patch is already written here: https://bug673331.bugzilla.mozilla.org/attachment.cgi?id=550570

Comment 1

[:glob ✱]

the patched file is part of the add-on sdk, not part of tweaks, so that's where this bug belongs; moving.

i've applied the patch to my sdk environment, so any build of tweaks that i produce will include this change.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to Byron Jones ‹:glob› from comment #1)
> the patched file is part of the add-on sdk, not part of tweaks, so that's
> where this bug belongs; moving.

That's good news, because it means many add-ons will benefit from this change, not just one.

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Report sandbox filenames in about:memory

[23:53]	<glob>	i have a bugzilla tweaks xpi with the patch from bug 680821 which may help; http://dl.dropbox.com/u/16292140/bugzilla-tweaks-1.11b.xpi

[00:06]	<glob>	njn, with regards to the sandboxName property and tweaks; the file that needs to be updated is part of the sdk, not tweaks itself, so really that's where the fix belongs
[00:07]	<njn>	glob: ok, so maybe the componet of bug 680821 should be changed?
	<glob>	njn, doing it now
	<njn>	glob: thx
[00:08]	<glob>	njn, i'm not sure if the patch is generic enough for all add-ons, i trust the jetpack guys will do the right thing
	<njn>	glob: if they don't know, god help us all
	<glob>	:)

[00:20]	<KWierso>	njn: does that jetpack bug need to be done soonish rather than laterish?
[00:21]	<njn>	KWierso: it would be nice if it were done by the next add-on SDK build, wheenver that is
	<KWierso>	is that patch safe for all current firefox versions?
[00:24]	njn: ^, or does that only work for Nightly builds?
	<njn>	KWierso: I think it's ineffective but harmless for non-Nightly builds, but I'm not 100% certain
	<glob>	KWierso, it works in aurora
[00:25]	<njn>	glob: how?
	<glob>	where "works" = doesn't cause an end of the world event

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Myk, who would review this? 
The patch I attached is the same as the one linked in comment 0, just based off the SDK github master.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Filename? Shouldn't that be an addon name? With file names it will be bloated imo, it's much more interesting how much memory an addon uses than the module files it's using (since then you won't know which addon is loaded that module file) Also, I already planned to use that property to identify addons (see Bug 677294 ) which I can change ofc, just I'm not sure that this is what we want.

Comment 7

[Brian Warner [:warner :bwarner]]

Incidentally, when the loader says options.filename, it probably really contains the URI (something like resource://$JETPACKID-$PACKAGE-$SECTION/main.js). The on-disk filename (starting with /home/warner/.firefox/profiles/etc) only shows up in the target of the resProt mapping.

If the memory-tracking code makes it easy to display a hierarchy, then it'd be useful to see both the per-module and the per-addon memory usage. If it's not, then I agree that sticking to just the addon name will make the output easier to use.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to Brian Warner [:warner :bwarner] from comment #7)
> If the memory-tracking code makes it easy to display a hierarchy, then it'd
> be useful to see both the per-module and the per-addon memory usage.

The memory-tracking code does display a hierarchy, but it doesn't appear to be configurable in this way.


> If it's
> not, then I agree that sticking to just the addon name will make the output
> easier to use.

I agree; however, the memory-tracking code doesn't particularly accommodate this either, since it doesn't coalesce statistics for the compartments of sandboxes with identical names; it reports each separately.


So right now there isn't a way to configure about:memory to show the total amount of memory being used by an addon, which is the most useful information for casual readers.  Nevertheless, folks want stats, and per-module stats are presumably better than nothing, so I suggest we do the following:

1. land the patch in this bug, to give folks the stats we can currently give them;
2. fix bug 672443 once platform blocker bug 677294 is fixed, so all modules load in a single compartment;
3. make improvements to about:memory as appropriate.

Regarding step #3, note that addons load web pages--in widgets, panels, tabs, etc.--in addition to modules, and those will continue to have their own compartments even after we load all modules into a single compartment, so the improvement to about:memory I'm particularly interested in is coalescing stats for multiple compartments associated with the same addon (which'll mean figuring out how to identify content frames as being associated with an addon).

Brian: sound reasonable?

Comment 9

[Brian Warner [:warner :bwarner]]

sounds good to me!

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to Myk Melez [:myk] from comment #8)
> 
> I agree; however, the memory-tracking code doesn't particularly accommodate
> this either, since it doesn't coalesce statistics for the compartments of
> sandboxes with identical names; it reports each separately.

about:memory does coalesce multiple reporters that have the same path, and puts a number in square brackets to indicate how many there were.  E.g. in the following example there were 4 storage/sqlite/places.sqlite/cache-used reporters:

├───25,561,968 B (06.52%) -- storage
│   └──25,561,968 B (06.52%) -- sqlite
│      ├──21,040,048 B (05.36%) -- places.sqlite
│      │  ├──20,670,880 B (05.27%) -- cache-used [4]
│      │  ├─────291,488 B (00.07%) -- stmt-used [4]
│      │  └──────77,680 B (00.02%) -- schema-used [4]

For system compartments, bug 672439 deliberately added the compartment's address to to the path used for each system compartment's reporter, so that they weren't coalesced.

It wouldn't be hard to only add the address to compartments that lack a name.  Then all compartments with the same name would be coalesced.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Myk Melez [:myk] from comment #8)
> 2. fix bug 672443 once platform blocker bug 677294 is fixed, so all modules
> load in a single compartment;

Just one little correction for this. My (current) patch for bug 677294 will not put every module per add-on into the same compartment, only the ones that share the same principal. I do not think it would be a good idea to merge modules with system principals, with low privileged modules into the same compartment (I'm not even sure if it's possible).

Another thought that I will use another identifier string there probably (it is addonId now) so maybe that information can be used here as well. Basically each sandbox will 'know' which add-on they are belong to not just the path of it's module file.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 554795 [details] [diff] [review]
Report sandbox filenames in about:memory

Trivial style nit: add space between the curly braces and their enclosed property declaration.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> It wouldn't be hard to only add the address to compartments that lack a
> name.  Then all compartments with the same name would be coalesced.

Ah, useful info!  To my mind, the ideal view would actually use hierarchy but not coalescence to reveal both total addon memory usage and usage by individual modules and content frames, so something like this:

├───### B (##.##%) -- addon [e-mail]
│   │──### B (##.##%) -- module addon-kit/panel
│   │──### B (##.##%) -- module api-utils/content/worker
│   │──### B (##.##%) -- frame http://www.example.com/

(Partial compartment sharing between modules may complicate the picture.  Also, I don't yet know how to identify frames as belonging to addons.)


(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> Just one little correction for this. My (current) patch for bug 677294 will
> not put every module per add-on into the same compartment, only the ones
> that share the same principal. I do not think it would be a good idea to
> merge modules with system principals, with low privileged modules into the
> same compartment (I'm not even sure if it's possible).

For the purposes of about:memory reporting, that seems fine, as long as it is possible to identify both the privileged and the non-privileged compartments as belonging to the same addon.


> Another thought that I will use another identifier string there probably (it
> is addonId now) so maybe that information can be used here as well.
> Basically each sandbox will 'know' which add-on they are belong to not just
> the path of it's module file.

Yup, definitely.  Once it is possible to identify sandboxes not only by their individual identities (which in this case correspond to modules) but also by their membership in a group (which in this case is an addon), it should be possible to reflect that information usefully into about:memory.


Landed, and credited to Sander, who submitted the original patch over in bug 673331:

https://github.com/mozilla/addon-sdk/commit/544ab803ef894978a9d31a4418a5bd6397cc070e

Comment 14

[Myk Melez [:myk] [@mykmelez]]

https://github.com/mozilla/addon-sdk/commit/39830898891b13a2e4893dd49ecc76223b9ce02d

Comment 15

[Nickolay_Ponomarev]

This change seems to have been undone in the new loader:
 $ grep -r "sandboxName" * | wc -l
       0

Or was there another reason for that?

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323

Pointer to Github pull-request

Comment 17

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request

Comment 18

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request

Comment 19

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request

Comment 20

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Pointer to Github pull-request

Comment 21

[Nicholas Nethercote [inactive]]

I don't understand how the add-on team uses Github.  Is this bug fixed?  If not, what remains to be done?

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Nicholas Nethercote [:njn] from comment #21)
> I don't understand how the add-on team uses Github.  Is this bug fixed?  If
> not, what remains to be done?

It was fixed, then a recent change in the SDK's module loader undid the fix, and pull request 323 will put the fix back in.

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 587037 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/323#

Another patch has been submitted by myk in bug 587197 that will fix this and at the same time use `sameGroupAs` attribute in order to share compartments.
So I prefer to land his patch as it will conflict with this one.

Comment 24

[Alexandre Poirot [:ochameau]]

Fixed by recent landing in bug 587197

Comment 25

[Myk Melez [:myk] [@mykmelez]]

(In reply to Alexandre Poirot (:ochameau) from comment #24)
> Fixed by recent landing in bug 587197

Erm, actually it was the landing in bug 672443 that resolves this issue.