Last Comment Bug 752357 - about:memory should show compartment names for non-system compartments
: about:memory should show compartment names for non-system compartments
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla15
Assigned To: Kevin Locke
:
Mentors:
Depends on: 752381
Blocks: 759966
  Show dependency treegraph
 
Reported: 2012-05-06 12:51 PDT by Kevin Locke
Modified: 2012-05-30 19:51 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Include compartment location (sandbox name) in compartment name when it differs from the principal name (2.56 KB, patch)
2012-05-06 12:51 PDT, Kevin Locke
no flags Details | Diff | Review
Include compartment location (sandbox name) in compartment name when it differs from the principal name (Version 2) (2.74 KB, patch)
2012-05-06 19:40 PDT, Kevin Locke
n.nethercote: feedback+
Details | Diff | Review
Include compartment location (sandbox name) in compartment name when it differs from the principal name (Version 3) (1.94 KB, patch)
2012-05-08 06:21 PDT, Kevin Locke
n.nethercote: review+
Details | Diff | Review

Description Kevin Locke 2012-05-06 12:51:14 PDT
Created attachment 621448 [details] [diff] [review]
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] 2012-05-06 14:49:56 PDT
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 [:njn] 2012-05-06 16:37:02 PDT
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 [:njn] 2012-05-06 18:04:00 PDT
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 2012-05-06 19:40:20 PDT
Created attachment 621489 [details] [diff] [review]
Include compartment location (sandbox name) in compartment name when it differs from the principal name (Version 2)
Comment 5 Kevin Locke 2012-05-06 19:49:43 PDT
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 [:njn] 2012-05-06 22:37:31 PDT
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 2012-05-08 06:21:48 PDT
Created attachment 621951 [details] [diff] [review]
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 [:njn] 2012-05-08 16:16:07 PDT
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 2012-05-08 16:27:28 PDT
Great!  I don't have commit access, so if you could land it for me, I would appreciate it.  Thanks!
Comment 10 Nicholas Nethercote [:njn] 2012-05-08 19:14:33 PDT
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] 2012-05-09 03:49:02 PDT
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 2012-05-09 07:39:04 PDT
It is indeed my first landed patch.  Hurrah!  Thanks (both for reviewing it, and the remarks)!
Comment 13 Nicholas Nethercote [:njn] 2012-05-30 19:39:32 PDT
Kevin, check out bug 759966 comment 3 -- that bug wouldn't have been filed without this change.  Great work! =)

Note You need to log in before you can comment on or make changes to this bug.