The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Kevin Locke, Assigned: Kevin Locke)

Tracking

Trunk
mozilla15
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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

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

Thanks Kevin!
Attachment #621448 - Flags: review?(bobbyholley+bmo)
Attachment #621448 - Flags: feedback?(n.nethercote)
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...".
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.
Depends on: 752381
(Assignee)

Comment 4

5 years ago
Created attachment 621489 [details] [diff] [review]
Include compartment location (sandbox name) in compartment name when it differs from the principal name (Version 2)
Attachment #621448 - Attachment is obsolete: true
Attachment #621448 - Flags: review?(bobbyholley+bmo)
Attachment #621448 - Flags: feedback?(n.nethercote)
(Assignee)

Comment 5

5 years ago
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 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.
Attachment #621489 - Flags: feedback+
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 7

5 years ago
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.
Attachment #621489 - Attachment is obsolete: true
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.
Attachment #621951 - Flags: review+
(Assignee)

Comment 9

5 years ago
Great!  I don't have commit access, so if you could land it for me, I would appreciate it.  Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/92cc1b470177

Kevin, AFAICT this is your first landed patch.  If so, congratulations!
Assignee: nobody → kevin
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Comment 12

5 years ago
It is indeed my first landed patch.  Hurrah!  Thanks (both for reviewing it, and the remarks)!
Kevin, check out bug 759966 comment 3 -- that bug wouldn't have been filed without this change.  Great work! =)
Blocks: 759966
You need to log in before you can comment on or make changes to this bug.