Closed Bug 759783 Opened 8 years ago Closed 8 years ago

Add identifying origin information to Javascript sandboxes

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: nmaier, Assigned: nmaier)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 4 obsolete files)

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.
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)
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+
Summary: Add identifying information to anonymous Javascript sandboxes → 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+
Keywords: checkin-needed
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?
Forgot to update the sandbox name test.
Keywords: checkin-needed
(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?
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]
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.  :(
Keywords: checkin-needed
Attached patch Patch v4, un-bitrotted (obsolete) — Splinter Review
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+
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?
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+
(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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.