Closed
Bug 730221
Opened 12 years ago
Closed 12 years ago
delegate transcoding of JS principals to the browser
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: igor, Assigned: igor)
References
Details
Attachments
(2 files, 1 obsolete file)
62.83 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
16.01 KB,
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #728250 +++ I split the bug 728250 so the patch there dealing with elimination of JSSecurrityCallbacks::transcodePrincipals can be landed separately under this bug.
Assignee | ||
Comment 1•12 years ago
|
||
Comments from the patch: ---- Currently to serialize principals stored in JSScript we have a rather complex schema. First there is the transcode callback that the embedding must provide to transcode principals using XDR API. Second we use rather complex glue code to implement that callback in terms of writing/reading nsIObjectOutputStream/nsIObjectInputStream. This glue code is duplicated in 3 places. All this can be avoided if we simply delegate transcoding of principals to the caller. In addition, at least in the case of the cached startup scripts we do not even need to transcode the principals as the the cached scripts always have the system principal so we can skip all the transcode complexity there. The patch implemnts this idea. In particular, the code in JS engine responsible to transcoding of principals is replaced by the single API function JS_XDRSetPrincipals that the embedding can use to set principals for decoded scripts and functions. Then the startup cache uses this to set the principals for the decoded script to the system principals. The rest 2 places in nsJSContext::Serialize and XBL_SerializeFunction that needs to serialize principals together with a function or script now uses common utilities in nsXPConnect so the serialization complexity resides in the single place. ----
Attachment #604356 -
Flags: review?(luke)
Attachment #604356 -
Flags: review?(bzbarsky)
Comment 2•12 years ago
|
||
Comment on attachment 604356 [details] [diff] [review] v1 Review of attachment 604356 [details] [diff] [review]: ----------------------------------------------------------------- Very nice ::: js/xpconnect/src/nsXPConnect.cpp @@ +2843,5 @@ > + if (NS_FAILED(rv)) return rv; > + } > + > + JSXDRState *xdr = ::JS_XDRNewMem(cx, JSXDR_ENCODE); > + if (! xdr) I think bholley fixed up xpconnect so that you don't need goofy spaces anymore.
Attachment #604356 -
Flags: review?(luke) → review+
Comment 3•12 years ago
|
||
Also, don't need goofy :: before JS_XDRNewMem and friends
Comment 4•12 years ago
|
||
XPConnect uses JS style now, IIRC.
Assignee | ||
Comment 5•12 years ago
|
||
Boris: the patch needs rebasing. Should I do it and ask for a new review or do you prefer to review the older version first?
Comment 6•12 years ago
|
||
I'm about 3/4 through the old version, so I'd prefer to finish that....
Comment 7•12 years ago
|
||
Comment on attachment 604356 [details] [diff] [review] v1 >The patch implemnts this idea. In particular, the code in JS engine > responsible to transcoding "for transcoding" > The rest 2 places "The other two places" > that needs to serialize "that need" It would be good to wrap the checkin comments at a sane width, I think. >try: -b do -p all -u all -t none And remove this part. >+++ b/js/xpconnect/loader/mozJSLoaderUtils.cpp > WriteCachedScript(StartupCache* cache, nsACString &uri, JSContext *cx, JSScript *script) This should assert somewhere that the script's principal is the system principal, right? The reader certainly assumes that! >+++ b/js/xpconnect/src/nsXPConnect.cpp > // Global cache of the default script security manager (QI'd to >-// nsIScriptSecurityManager) >+// nsIScriptSecurityManager) and the system principal I don't see the system principal being stored in a global here... > nsXPConnect::SetDefaultSecurityManager(nsIXPCSecurityManager *aManager, Are you sure no one calls this with a null aManager? Seems to me like someone could, and your code would crash... >+ReadScriptOrFunction(nsIObjectInputStream *stream, JSContext *cx, >+ if (!xdr) { >+ return NS_ERROR_OUT_OF_MEMORY; This leaks |data|. Need to nsMemory::Free it first. r=me with those issues fixed..
Attachment #604356 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 8•12 years ago
|
||
To address the comment 7 I removed the optimization that avoided serializing the system principals in (Read|Write)ScriptOrFunction that I added to nsXPConnect.cpp. As the startup cache does not use those utilities and just assigns the system principals to the read scripts, the extra performance win for doing that optimization is unclear and do not justifies the extra changes. Instead I suppose it would be nice to investigate what exactly the principals we can ever serialize. It may well be that we do not need the generic power of nsISerializable in nsIPrincipal as there is no need to serialize generic principals. Also the patch now passes the system principal to WriteCachedScript. This way the function can assert that the script indeed has the system principals and the optimization to skip writing the principals is valid.
Attachment #604356 -
Attachment is obsolete: true
Attachment #606818 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•12 years ago
|
||
Comment 10•12 years ago
|
||
I'm not sure why you had to get rid of the optimization to address comment 7.... It should be quite relevant for XBL, I suspect.
Comment 11•12 years ago
|
||
Igor, I'm waiting for a response to comment 10 here.
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #10) > I'm not sure why you had to get rid of the optimization to address comment > 7.... It should be quite relevant for XBL, I suspect. I put that optimization in the first place to support the start up cache before realizing that it does not need to serialize principals at all. For XBL I want to do that in another bug - it may be that it would be possible to avoid the principal serialization there in a similar way.
Comment 13•12 years ago
|
||
Comment on attachment 606818 [details] [diff] [review] v2 OK, r=me
Attachment #606818 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c5015550af
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/46c5015550af
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment 16•12 years ago
|
||
I just realized this patch changes the serialization format for all sorts of stuff. Do we not need to bump cache version numbers?
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #16) > I just realized this patch changes the serialization format for all sorts of > stuff. Do we not need to bump cache version numbers? Thanks for spotting this! https://hg.mozilla.org/integration/mozilla-inbound/rev/fd4ee3ba9f3e is the landed version bump.
Comment 19•12 years ago
|
||
In case you hadn't seen... :-) Improvement! Trace Malloc Allocs decrease 2.22% on CentOS release 5 (Final) Mozilla-Inbound ------------------------------------------------------------------------------------------- Previous: avg 465058.900 stddev 1322.557 of 30 runs up to revision 22af7bc45a18 New : avg 454718.800 stddev 719.015 of 5 runs since revision 46c5015550af Change : -10340.100 (2.22% / z=7.818) Graph : http://mzl.la/GAtbBh Improvement! Trace Malloc MaxHeap decrease 0.8% on CentOS release 5 (Final) Mozilla-Inbound ------------------------------------------------------------------------------------------- Previous: avg 26983256.667 stddev 16887.777 of 30 runs up to revision 22af7bc45a18 New : avg 26767300.000 stddev 24095.331 of 5 runs since revision 46c5015550af Change : -215956.667 (0.8% / z=12.788) Graph : http://mzl.la/GAtbRP Improvement! Trace Malloc Allocs decrease 2.91% on WINNT 5.2 Mozilla-Inbound ---------------------------------------------------------------------------- Previous: avg 360445.500 stddev 1454.163 of 30 runs up to revision 22af7bc45a18 New : avg 349941.400 stddev 1422.330 of 5 runs since revision 46c5015550af Change : -10504.100 (2.91% / z=7.223) Graph : http://mzl.la/GBSxA9 Improvement! Trace Malloc Allocs decrease 2.45% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound ---------------------------------------------------------------------------------------------------- Previous: avg 456415.800 stddev 1372.416 of 30 runs up to revision 22af7bc45a18 New : avg 445243.400 stddev 905.416 of 5 runs since revision 46c5015550af Change : -11172.400 (2.45% / z=8.141) Graph : http://mzl.la/GB9nwI \o/
You need to log in
before you can comment on or make changes to this bug.
Description
•