Closed
Bug 634156
Opened 14 years ago
Closed 13 years ago
ConsoleAPI.js and InstallTrigger.js should not use sandboxes
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla8
Tracking | Status | |
---|---|---|
status2.0 | --- | wanted |
People
(Reporter: gal, Assigned: mrbkap)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [MemShrink:P2] fixed-in-tracemonkey)
Attachments
(2 files, 2 obsolete files)
14.10 KB,
patch
|
gal
:
review+
mossop
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
6.84 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
We currently use a sandbox to bind chrome functions to content objects. We could just do contentWindow.wrappedJSObject.Function.bind.call instead. Content can overwrite bind and intercept the wrapped chrome objects, but thats fine. We have __exposedProps__ in place so thats safe. This would be much faster and saves memory. Patrick, want to give this a try? Jonas and mrbkap should review.
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → pwalton
Reporter | ||
Comment 1•14 years ago
|
||
I would take an approved patch to avoid any further runaway situations.
Comment 2•14 years ago
|
||
Like this?
Reporter | ||
Comment 3•14 years ago
|
||
Comment on attachment 522698 [details] [diff] [review]
Patch (v1)
Yeah, this is roughly what we had in mind. However, mrbkap was concerned that a malicious page might overwrite Function.bind, and then trick us into calling that. To be honest, I don't think anything bad would happen here. We are passing the chromeObject to bind, but its wrapped and you can't do much with it, so I would be ok with it, but mrbkap should make the final call.
Attachment #522698 -
Flags: review?(gal) → review?(mrbkap)
Comment 4•14 years ago
|
||
This breaks numerous tests:
1 in mochitest-4:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1301429260.1301433190.17280.gz
1 in mochitest-5:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1301429520.1301430563.3261.gz
324 in mochitest-o (debug):
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1301429271.1301434384.23735.gz
516 in mochitest-o (opt):
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1301434202.1301437331.7956.gz
I don't really know what I should do to address these, so I'm just unassigning myself, but hopefully somebody can pick up where I left off.
Assignee: ehsan → nobody
Updated•14 years ago
|
Status: ASSIGNED → NEW
OS: Mac OS X → All
Hardware: x86 → All
Updated•14 years ago
|
Attachment #522698 -
Flags: review?(mrbkap)
Updated•14 years ago
|
Updated•14 years ago
|
Whiteboard: [MemShrink:P2]
Comment 6•14 years ago
|
||
Blake, any progress on this? What situations will it help in, and how much memory might it save?
Assignee | ||
Comment 7•13 years ago
|
||
I have a mostly-working patch (still a bit to do, though). I think this might end up fixing bug 664067.
Reporter | ||
Comment 8•13 years ago
|
||
I noticed that console.log is ridiculously slow. Is that related to sandboxes?
Assignee | ||
Comment 9•13 years ago
|
||
I would assume not. The sandbox is a one-time creation deal. You do end up going through a couple of cross compartment wrappers because of it, but that shouldn't make it ridiculously slow.
Please file a separate bug on that issue.
Assignee | ||
Comment 11•13 years ago
|
||
This seems to work, but I haven't tested it very heavily yet.
I took the easy way out with this patch by splitting the work into two functions and letting JS code do most of the heavy lifting of figuring out which properties to expose. I can move it back into C++ now if people feel that it's worth it, though.
Attachment #522698 -
Attachment is obsolete: true
Assignee | ||
Comment 12•13 years ago
|
||
Andreas, what do you think of this approach?
Attachment #539670 -
Attachment is obsolete: true
Attachment #539856 -
Flags: review?(gal)
Reporter | ||
Comment 13•13 years ago
|
||
Comment on attachment 539856 [details] [diff] [review]
patch v1
Looks great. Too bad we need C++ here. dherman's modules will fix that.
Attachment #539856 -
Flags: review?(gal) → review+
Assignee | ||
Comment 14•13 years ago
|
||
Comment on attachment 539856 [details] [diff] [review]
patch v1
Johnny, mind commenting on the API? Also requesting review from a JS hacker for his comments on the API.
Attachment #539856 -
Flags: superreview?(jst)
Attachment #539856 -
Flags: review?(dtownsend)
Comment 15•13 years ago
|
||
Comment on attachment 539856 [details] [diff] [review]
patch v1
Looks good from an API point of view, and in general too! r=jst
Attachment #539856 -
Flags: superreview?(jst) → superreview+
Comment 16•13 years ago
|
||
Comment on attachment 539856 [details] [diff] [review]
patch v1
Review of attachment 539856 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/xpconnect/idl/xpccomponents.idl
@@ +232,5 @@
> + * To be called from JS only.
> + *
> + * Returns an object created in |scope|'s compartment.
> + */
> + void /* JSObject */ createObjectIn(/*in JSObject scope */);
Could this use jsval for the argument and the return value and implicit_jscontext to get the JSContext? Might make the implementation a little shorter.
@@ +240,5 @@
> + *
> + * Ensures that all functions come from obj's scope (and aren't cross
> + * compartment wrappers).
> + */
> + void makeObjectPropsNormal(/* in JSObject obj */);
Same here.
Updated•13 years ago
|
Attachment #539856 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 17•13 years ago
|
||
This is an interdiff addressing comment 16.
Attachment #541522 -
Flags: review?(peterv)
Comment 18•13 years ago
|
||
Comment on attachment 541522 [details] [diff] [review]
Addressing peterv's comment
(stealing peterv's review), r=jst. I like the - to + ratio here :)
Attachment #541522 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 19•13 years ago
|
||
Whiteboard: [MemShrink:P2] → [MemShrink:P2] fixed-in-tracemonkey
Assignee | ||
Comment 20•13 years ago
|
||
And http://hg.mozilla.org/tracemonkey/rev/208160c856b7 to fix bustage.
Comment 21•13 years ago
|
||
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/ef1c0626aa25
http://hg.mozilla.org/mozilla-central/rev/208160c856b7
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Comment 22•13 years ago
|
||
Comment on attachment 541522 [details] [diff] [review]
Addressing peterv's comment
Asking for Aurora approval. This was fixed-in-tracemonkey 9 days before the Aurora uplift, but missed out because TM was merged to m-c just too late.
I think it should be granted Aurora approval. It would have gone in anyway if it wasn't for the late TM merge, so the risk doesn't seem higher than normal bugs. The benefit is that it fixed 664067 which was a very good thing -- there are a lot of Facebook like buttons and Google +1 buttons on the web, and this makes at least some of them more efficient.
Attachment #541522 -
Flags: approval-mozilla-aurora?
Comment 24•13 years ago
|
||
Comment on attachment 541522 [details] [diff] [review]
Addressing peterv's comment
Aurora approval no longer required, thanks to the special merge of late-landing TM things (http://hg.mozilla.org/releases/mozilla-aurora/rev/75552dfd25c2).
Attachment #541522 -
Flags: approval-mozilla-aurora?
Comment 25•13 years ago
|
||
Documentation has been written. A review would be appreciated (especially of the code example, which was admittedly pieced together from the patch without expressly being tested):
https://developer.mozilla.org/en/Components.utils.createObjectIn
https://developer.mozilla.org/en/Components.utils.makeObjectPropsNormal
These pages have been updated to link to the new content:
https://developer.mozilla.org/en/Components_object
https://developer.mozilla.org/en/Components.utils
https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_NewObject
Also linked from Firefox 8 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Updated•12 years ago
|
Component: DOM: Other → DOM
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•