Last Comment Bug 759783 - Add identifying origin information to Javascript sandboxes
: Add identifying origin information to Javascript sandboxes
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nils Maier [:nmaier]
:
: Nicholas Nethercote [:njn]
Mentors:
Depends on: 673331 754771
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-30 08:56 PDT by Nils Maier [:nmaier]
Modified: 2012-06-19 01:20 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1, Add origin information to all sandboxes (2.44 KB, patch)
2012-05-30 10:44 PDT, Nils Maier [:nmaier]
justin.lebar+bug: feedback+
Details | Diff | Splinter Review
Patch v2, Add identifying origin information to Javascript sandboxes (2.50 KB, patch)
2012-05-31 14:19 PDT, Nils Maier [:nmaier]
justin.lebar+bug: review+
Details | Diff | Splinter Review
Patch v3, As before + updating the test (3.23 KB, patch)
2012-06-05 03:44 PDT, Nils Maier [:nmaier]
justin.lebar+bug: review+
Details | Diff | Splinter Review
Patch v4, un-bitrotted (3.70 KB, patch)
2012-06-15 11:46 PDT, Nils Maier [:nmaier]
justin.lebar+bug: review+
Details | Diff | Splinter Review
Patch v5, AssembleSandboxMemoryReporterName (4.00 KB, patch)
2012-06-15 15:57 PDT, Nils Maier [:nmaier]
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Nils Maier [:nmaier] 2012-05-30 08:56:35 PDT
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) 2012-05-30 09:24:53 PDT
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) 2012-05-30 09:56:27 PDT
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] 2012-05-30 10:13:54 PDT
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] 2012-05-30 10:44:55 PDT
Created attachment 628384 [details] [diff] [review]
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/restartless.restart@erikvold.com.xpi!/bootstrap.js <- resource://gre/modules/XPIProvider.jsm:3618
Comment 5 Justin Lebar (not reading bugmail) 2012-05-30 12:06:37 PDT
> 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] 2012-05-31 14:19:43 PDT
Created attachment 628899 [details] [diff] [review]
Patch v2, Add identifying origin information to Javascript sandboxes

Using "sandbox (from: location:lineno)" now.
Comment 7 Justin Lebar (not reading bugmail) 2012-05-31 22:56:30 PDT
Comment on attachment 628899 [details] [diff] [review]
Patch v2, Add identifying origin information to Javascript sandboxes

Cool.
Comment 8 Nicholas Nethercote [:njn] 2012-06-04 19:06:13 PDT
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] 2012-06-05 03:00:41 PDT
Forgot to update the sandbox name test.
Comment 10 Nils Maier [:nmaier] 2012-06-05 03:42:18 PDT
(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] 2012-06-05 03:44:31 PDT
Created attachment 630125 [details] [diff] [review]
Patch v3, As before + updating the test

Updated the sandboxName test.
Comment 12 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-06-05 12:37:45 PDT
(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 [:njn] 2012-06-12 16:21:57 PDT
Nils, do you still need to update the test?  Or is this ready to land?
Comment 14 Nils Maier [:nmaier] 2012-06-15 08:48:03 PDT
Patch v3 should be ready to land
Comment 15 Justin Lebar (not reading bugmail) 2012-06-15 08:55:45 PDT
(In reply to Nils Maier [:nmaier] from comment #14)
> Patch v3 should be ready to land

It's bitrotted.  :(
Comment 16 Nils Maier [:nmaier] 2012-06-15 11:46:19 PDT
Created attachment 633605 [details] [diff] [review]
Patch v4, un-bitrotted

Alright, un-bitrotted.
Otherwise should be the same.
Comment 17 Justin Lebar (not reading bugmail) 2012-06-15 14:53:12 PDT
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] 2012-06-15 15:19:15 PDT
How about something as simple as (Assemble|Build|Create|Generate)SandboxName?
Comment 19 Justin Lebar (not reading bugmail) 2012-06-15 15:24:09 PDT
(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] 2012-06-15 15:57:45 PDT
Created attachment 633708 [details] [diff] [review]
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) 2012-06-18 07:26:32 PDT
Comment on attachment 633708 [details] [diff] [review]
Patch v5, AssembleSandboxMemoryReporterName

Want me to check this in for you?
Comment 22 Nils Maier [:nmaier] 2012-06-18 09:04:05 PDT
(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) 2012-06-18 10:09:59 PDT
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] 2012-06-19 01:20:05 PDT
https://hg.mozilla.org/mozilla-central/rev/8798f7d6090a

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