752357 - about:memory should show compartment names for non-system compartments

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Kevin Locke]

Attachment: Include compartment location (sandbox name) in compartment name when it differs from the principal name

For extensions which make use of sandboxes with null principal, rather than system principal, it would be very helpful if about:memory included the sandbox name/compartment location of in addition to the principal name/codebase in its output.  Without the sandbox name/compartment location in the output, once multiple compartments are created with the null principal it becomes very difficult to determine which is associated with each entry in about:memory and to use it to useful effect.

Since I imagine this could be the case for other principals as well, the attached patch includes the sandbox name/compartment location in the output whenever it differs from the principal name/codebase.  If there are cases where this is unhelpful, feel free to limit the behavior to the only the null principal (which is the only case I am concerned with at the moment).

Comment 1

[Josh Matthews [:jdm]]

Comment on attachment 621448 [details] [diff] [review]
Include compartment location (sandbox name) in compartment name when it differs from the principal name

Thanks Kevin!

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 621448 [details] [diff] [review]
Include compartment location (sandbox name) in compartment name when it differs from the principal name

Review of attachment 621448 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine to me, but can you give an example of what it looks like before and after this change?  That'll make evaluating it much easier.  Thanks!

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1220,3 @@
>          // And we append the address if |getAddress| is true, so that
>          // multiple system compartments (and there can be many) can be
>          // distinguished.

Change to "the address to system compartments if...".

Comment 3

[Nicholas Nethercote [inactive]]

BTW, bug 752381 is going to conflict with this.  If you don't mind I'd like to land the patch from that bug before this one, because that one is fixing a problem that currently makes about:memory almost unusable.  It should be very easy for you to rebase after it lands.

Comment 4

[Kevin Locke]

Attachment: Include compartment location (sandbox name) in compartment name when it differs from the principal name (Version 2)

Comment 5

[Kevin Locke]

Sounds good.  I've attached an updated patch with the comment corrections.  I'll rebase and resubmit after the patch for bug 752381 lands.  Thanks!

To clarify how it will change the output in about:memory, assume we create 3 compartments/sandboxes named "Sandbox Name X" (for X = {1,2,3}), one for the system principal and two for the null principal.  The output would currently show the compartments as follows:

compartment([System Principal], Sandbox Name 1, 0xa123738)
compartment(moz-nullprincipal:{3a0f875d-a922-4d85-9edc-ef9b048ba41a})
compartment(moz-nullprincipal:{f2fd83b0-c905-4517-9e6d-4a2125731a18})

After the patch they would appear as: 

compartment([System Principal], Sandbox Name 1, 0xa123738)
compartment(moz-nullprincipal:{3a0f875d-a922-4d85-9edc-ef9b048ba41a}, Sandbox Name 2)
compartment(moz-nullprincipal:{f2fd83b0-c905-4517-9e6d-4a2125731a18}, Sandbox Name 3)

Compartments which are created for a principal that shares its name/codebase with the compartment location/sandbox name would be unaffected and remain as:

compartment(http://www.google.com/)

Is that somewhat more clear?

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 621489 [details] [diff] [review]
Include compartment location (sandbox name) in compartment name when it differs from the principal name (Version 2)

Review of attachment 621489 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation.  Looks good.  I just landed the patch from bug 752381.

Comment 7

[Kevin Locke]

Attachment: Include compartment location (sandbox name) in compartment name when it differs from the principal name (Version 3)

Attached is the post-bug 752381 version of the patch.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 621951 [details] [diff] [review]
Include compartment location (sandbox name) in compartment name when it differs from the principal name (Version 3)

Review of attachment 621951 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!  I don't think this needs r+ from bholley, because it's a small change to code that I wrote.  I assume you don't have commit access -- if not, please let me know and I can land this for you.

Comment 9

[Kevin Locke]

Great!  I don't have commit access, so if you could land it for me, I would appreciate it.  Thanks!

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92cc1b470177

Kevin, AFAICT this is your first landed patch.  If so, congratulations!

Comment 11

[Ed Morley [:emorley]]

Congrats on the patch! Look forward to seeing you on IRC (https://wiki.mozilla.org/IRC) in #developers :-)

https://hg.mozilla.org/mozilla-central/rev/92cc1b470177

Comment 12

[Kevin Locke]

It is indeed my first landed patch.  Hurrah!  Thanks (both for reviewing it, and the remarks)!

Comment 13

[Nicholas Nethercote [inactive]]

Kevin, check out bug 759966 comment 3 -- that bug wouldn't have been filed without this change.  Great work! =)