Closed Bug 730221 Opened 12 years ago Closed 12 years ago

delegate transcoding of JS principals to the browser

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(2 files, 1 obsolete file)

+++ 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.
Attached patch v1 (obsolete) — Splinter Review
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 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+
Also, don't need goofy :: before JS_XDRNewMem and friends
XPConnect uses JS style now, IIRC.
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?
I'm about 3/4 through the old version, so I'd prefer to finish that....
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+
Attached patch v2Splinter Review
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)
Attached patch v1-v2 interdiffSplinter Review
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.
Igor, I'm waiting for a response to comment 10 here.
(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 on attachment 606818 [details] [diff] [review]
v2

OK, r=me
Attachment #606818 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/46c5015550af
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Blocks: 737607
Blocks: 737624
I just realized this patch changes the serialization format for all sorts of stuff.  Do we not need to bump cache version numbers?
(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.
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/
Blocks: 725092
You need to log in before you can comment on or make changes to this bug.