Add identifying origin information to Javascript sandboxes

RESOLVED FIXED in mozilla16

Status

()

Toolkit
about:memory
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nmaier, Assigned: nmaier)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
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.)
Component: XPConnect → about:memory
Product: Core → Toolkit
QA Contact: xpconnect → about.memory
Over to toolkit/about:memory because I suspect most of the people who care about this will be watching that component, not XPConnect.
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
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
Attachment #628384 - Flags: feedback?(justin.lebar+bug)
> 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/.)
Attachment #628384 - Flags: feedback?(justin.lebar+bug) → feedback+
(Assignee)

Updated

5 years ago
Summary: Add identifying information to anonymous Javascript sandboxes → Add identifying origin information to Javascript sandboxes
(Assignee)

Comment 6

5 years ago
Created attachment 628899 [details] [diff] [review]
Patch v2, Add identifying origin information to Javascript sandboxes

Using "sandbox (from: location:lineno)" now.
Attachment #628384 - Attachment is obsolete: true
Attachment #628899 - Flags: review?(justin.lebar+bug)
Comment on attachment 628899 [details] [diff] [review]
Patch v2, Add identifying origin information to Javascript sandboxes

Cool.
Attachment #628899 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Assignee: nobody → MaierMan
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?
(Assignee)

Comment 9

5 years ago
Forgot to update the sandbox name test.
Keywords: checkin-needed
(Assignee)

Comment 10

5 years ago
(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?
(Assignee)

Comment 11

5 years ago
Created attachment 630125 [details] [diff] [review]
Patch v3, As before + updating the test

Updated the sandboxName test.
Attachment #628899 - Attachment is obsolete: true
Attachment #630125 - Flags: review?(justin.lebar+bug)
Attachment #630125 - Flags: review?(justin.lebar+bug) → review+
(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.
Nils, do you still need to update the test?  Or is this ready to land?
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 14

5 years ago
Patch v3 should be ready to land
Keywords: checkin-needed
(In reply to Nils Maier [:nmaier] from comment #14)
> Patch v3 should be ready to land

It's bitrotted.  :(
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

5 years ago
Created attachment 633605 [details] [diff] [review]
Patch v4, un-bitrotted

Alright, un-bitrotted.
Otherwise should be the same.
Attachment #630125 - Attachment is obsolete: true
Attachment #633605 - Flags: review?(justin.lebar+bug)
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...)
Attachment #633605 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 18

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

(...)SandboxMemoryReporterName?
(Assignee)

Comment 20

5 years ago
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.
Attachment #633605 - Attachment is obsolete: true
Attachment #633708 - Flags: review?(justin.lebar+bug)
Comment on attachment 633708 [details] [diff] [review]
Patch v5, AssembleSandboxMemoryReporterName

Want me to check this in for you?
Attachment #633708 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 22

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

Yes please
Attachment #633708 - Attachment is patch: true
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
https://hg.mozilla.org/mozilla-central/rev/8798f7d6090a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.