Last Comment Bug 730221 - delegate transcoding of JS principals to the browser
: delegate transcoding of JS principals to the browser
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 728250
Blocks: 737607 725092 737624
  Show dependency treegraph
 
Reported: 2012-02-24 00:45 PST by Igor Bukanov
Modified: 2012-04-04 12:38 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (65.51 KB, patch)
2012-03-09 03:12 PST, Igor Bukanov
bzbarsky: review+
luke: review+
Details | Diff | Splinter Review
v2 (62.83 KB, patch)
2012-03-16 18:47 PDT, Igor Bukanov
bzbarsky: review+
Details | Diff | Splinter Review
v1-v2 interdiff (16.01 KB, patch)
2012-03-16 18:48 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review

Description Igor Bukanov 2012-02-24 00:45:56 PST
+++ 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.
Comment 1 Igor Bukanov 2012-03-09 03:12:29 PST
Created attachment 604356 [details] [diff] [review]
v1

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.

----
Comment 2 Luke Wagner [:luke] 2012-03-09 09:26:27 PST
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.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-03-09 09:28:45 PST
Also, don't need goofy :: before JS_XDRNewMem and friends
Comment 4 Andrew McCreight [:mccr8] 2012-03-09 09:33:51 PST
XPConnect uses JS style now, IIRC.
Comment 5 Igor Bukanov 2012-03-16 11:41:21 PDT
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 Boris Zbarsky [:bz] 2012-03-16 12:22:14 PDT
I'm about 3/4 through the old version, so I'd prefer to finish that....
Comment 7 Boris Zbarsky [:bz] 2012-03-16 14:14:20 PDT
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..
Comment 8 Igor Bukanov 2012-03-16 18:47:30 PDT
Created attachment 606818 [details] [diff] [review]
v2

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.
Comment 9 Igor Bukanov 2012-03-16 18:48:08 PDT
Created attachment 606819 [details] [diff] [review]
v1-v2 interdiff
Comment 10 Boris Zbarsky [:bz] 2012-03-16 19:03:30 PDT
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 Boris Zbarsky [:bz] 2012-03-19 15:20:34 PDT
Igor, I'm waiting for a response to comment 10 here.
Comment 12 Igor Bukanov 2012-03-19 16:24:49 PDT
(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 Boris Zbarsky [:bz] 2012-03-19 19:32:57 PDT
Comment on attachment 606818 [details] [diff] [review]
v2

OK, r=me
Comment 15 Matt Brubeck (:mbrubeck) 2012-03-20 10:28:30 PDT
https://hg.mozilla.org/mozilla-central/rev/46c5015550af
Comment 16 Boris Zbarsky [:bz] 2012-03-21 08:58:49 PDT
I just realized this patch changes the serialization format for all sorts of stuff.  Do we not need to bump cache version numbers?
Comment 17 Igor Bukanov 2012-03-21 11:17:38 PDT
(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 18 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-22 06:29:46 PDT
https://hg.mozilla.org/mozilla-central/rev/fd4ee3ba9f3e
Comment 19 Ed Morley [:emorley] 2012-03-25 04:37:46 PDT
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/

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