759783 - Add identifying origin information to Javascript sandboxes

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nils Maier [:nmaier]]

JS sandboxes only get identifying information added if sandboxName is specified in the options to createSandbox.

So we should at least add identifying information to sandboxes where sandboxName is omitted.
sandboxName can be anything, so the information there is not necessarily "identifying".

There are two alternatives to address this that I can think of:
1.) Always add a reference to the script creating the Sandbox. This would allow tracing sandboxes back to their origin always.
E.g.
chrome://addon/content/module.jsm -> sandboxName
chrome://addon/content/module.jsm -> chrome://addon/content/sandbox.js
chrome://addon/content/module.jsm -> [anonymous sandbox]

2.) Only derive a sandboxName if omitted in the options.
E.g.
sandboxName
chrome://addon/content/sandbox.js
chrome://addon/content/module.jsm [anonymous sandbox]

I prefer 1.) because it would make sandboxes always identifiable.

Comment 1

[Justin Lebar (not reading bugmail)]

Maybe we could do (1) but flip the order (name then creator).  Compartment names in about:memory get cut off at a certain length, and if you bothered to give a name, we should try not to cut it off...

(We should also try harder not to cut off these names, but that's a separate issue.)

Comment 2

[Justin Lebar (not reading bugmail)]

Over to toolkit/about:memory because I suspect most of the people who care about this will be watching that component, not XPConnect.

Comment 3

[Nils Maier [:nmaier]]

Hmmm... Completely slipped my radar:
Bug 681556 added 2)
Still, I'd like to be able to see where the Sandbox was created, i.e. 1)

Comment 4

[Nils Maier [:nmaier]]

Attachment: patch v1, Add origin information to all sandboxes

Justin, is this what you had in mind when asking to switch origin and name?
The patch produces output like:
[System Principal], [anonymous sandbox] <- chrome://about-addons-memory/content/main.js:29
[System Principal], sandbox 1 <- chrome://about-addons-memory/content/main.js:25
[System Principal], jar:file:///Users/maierman/dev/p/extensions/[e-mail]!/bootstrap.js <- resource://gre/modules/XPIProvider.jsm:3618

Comment 5

[Justin Lebar (not reading bugmail)]

> Justin, is this what you had in mind when asking to switch origin and name?

Yes!  I might prefer

  [System Principal], [anonymous sandbox] (from chrome://about-addons-memory/content/main.js:29)

That's not a lot longer than "<-", but the meaning is more clear.  (Or, if you prefer, we could s/from/created by/.)

Comment 6

[Nils Maier [:nmaier]]

Attachment: Patch v2, Add identifying origin information to Javascript sandboxes

Using "sandbox (from: location:lineno)" now.

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 628899 [details] [diff] [review]
Patch v2, Add identifying origin information to Javascript sandboxes

Cool.

Comment 8

[Nicholas Nethercote [inactive]]

I just had a commenter on my blog (http://blog.mozilla.org/nnethercote/2012/05/30/memshrink-progress-week-49-50/#comment-6423) have an about:compartments entry like this:

  http://www.reddit.com/r/Games, resource://jid1-xufzosoflzsoxg-at-jetpack/api-utils/lib/cuddlefish.js -> resource://jid1-xufzosoflzsoxg-at-jetpack/api-utils/lib/sandbox.js [2]

Do we already have some places where we use " -> " in the compartment reporters?

Comment 9

[Nils Maier [:nmaier]]

Forgot to update the sandbox name test.

Comment 10

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> I just had a commenter on my blog
> (http://blog.mozilla.org/nnethercote/2012/05/30/memshrink-progress-week-49-
> 50/#comment-6423) have an about:compartments entry like this:
> 
>   http://www.reddit.com/r/Games,
> resource://jid1-xufzosoflzsoxg-at-jetpack/api-utils/lib/cuddlefish.js ->
> resource://jid1-xufzosoflzsoxg-at-jetpack/api-utils/lib/sandbox.js [2]
> 
> Do we already have some places where we use " -> " in the compartment
> reporters?

Hmmm... This might be a side effect of bug 418356, the only instance I could find where the -> is actually forced by the platform. The Add-on has a pretty complex system of Cu.Sandboxes interleaved with loadSubScripts.

Maybe Irakli knows, having written most of the stuff IIRC?

Comment 11

[Nils Maier [:nmaier]]

Attachment: Patch v3, As before + updating the test

Updated the sandboxName test.

Comment 12

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

(In reply to Nils Maier [:nmaier] from comment #10)
> (In reply to Nicholas Nethercote [:njn] from comment #8)
> > I just had a commenter on my blog
> > (http://blog.mozilla.org/nnethercote/2012/05/30/memshrink-progress-week-49-
> > 50/#comment-6423) have an about:compartments entry like this:
> > 
> >   http://www.reddit.com/r/Games,
> > resource://jid1-xufzosoflzsoxg-at-jetpack/api-utils/lib/cuddlefish.js ->
> > resource://jid1-xufzosoflzsoxg-at-jetpack/api-utils/lib/sandbox.js [2]
> > 
> > Do we already have some places where we use " -> " in the compartment
> > reporters?
> 
> Hmmm... This might be a side effect of bug 418356, the only instance I could
> find where the -> is actually forced by the platform. The Add-on has a
> pretty complex system of Cu.Sandboxes interleaved with loadSubScripts.
> 
> Maybe Irakli knows, having written most of the stuff IIRC?

Yes we use separate Cu.Sandbox for each module and load code into it via loadSubScripts. Although we do specify sandboxName, for all modules.

Comment 13

[Nicholas Nethercote [inactive]]

Nils, do you still need to update the test?  Or is this ready to land?

Comment 14

[Nils Maier [:nmaier]]

Patch v3 should be ready to land

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to Nils Maier [:nmaier] from comment #14)
> Patch v3 should be ready to land

It's bitrotted.  :(

Comment 16

[Nils Maier [:nmaier]]

Attachment: Patch v4, un-bitrotted

Alright, un-bitrotted.
Otherwise should be the same.

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 633605 [details] [diff] [review]
Patch v4, un-bitrotted

This is fine, but I don't like how "GetSandboxNameFromStack" doesn't actually get the sandbox name.  Maybe "FillInSandboxNameFromStack" or "AddCallerInfoToSandboxName" or something?

r=me with the name changed to something more sensible.  (If you want, you have even more options for function names if you restructure the code so that it gets the " (from: foo.js:42)" bit in this function, and you do the |sandboxName.IsEmpty()| check outside the function...)

Comment 18

[Nils Maier [:nmaier]]

How about something as simple as (Assemble|Build|Create|Generate)SandboxName?

Comment 19

[Justin Lebar (not reading bugmail)]

(In reply to Nils Maier [:nmaier] from comment #18)
> How about something as simple as (Assemble|Build|Create|Generate)SandboxName?

(...)SandboxMemoryReporterName?

Comment 20

[Nils Maier [:nmaier]]

Attachment: Patch v5, AssembleSandboxMemoryReporterName

Other than changing the name I also removed an obsolete comment and added a few new ones, so another review is in order /methinks.

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 633708 [details] [diff] [review]
Patch v5, AssembleSandboxMemoryReporterName

Want me to check this in for you?

Comment 22

[Nils Maier [:nmaier]]

(In reply to Justin Lebar [:jlebar] from comment #21)
> Want me to check this in for you?

Yes please

Comment 23

[Justin Lebar (not reading bugmail)]

Next time I'll check that it's a hg exported patch before volunteering to check it in.  :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/8798f7d6090a

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8798f7d6090a