213946 - [META] Optimize script security manager

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Running on the testcase in bug 213943, of the 1602 total profiler hits, 443 are
in nsScriptSecurityManager::CanAccess, and of those 430 are in
CheckPropertyAccessImpl.  The breakdown for those 430 (only listing the top
callees) is:

275 nsScriptSecurityManager::doGetObjectPrincipal
 42 nsScriptSecurityManager::GetSubjectPrincipal
 30 nsDOMClassInfo::GetClassDescription
 25 nsMemory::Free

The breakdown for those 275 hits in doGetObjectPrincipal (again, only top
callees) is:

178 nsCOMPtr_base::assign_from_helper
 22 nsCOMPtr_base
 18 XPCWrappedNative::GetNative
  9 JS_GetParent
  7 JS_GetPrivate
  6 nsDocument::GetPrincipal

and the breakdown for the assign_from_helper kids looks like:

84 nsHTMLDivElement::QueryInterface
26 XPCWrappedNative::QueryInterface
22 nsHTMLDocument::QueryInterface
18 nsHTMLBodyElement::QueryInterface
16 nsHTMLHtmlElement::QueryInterface

ANALYSIS:  

The one clear bit here are the nsDOMClassInfo::GetClassDescription and
nsMemory::Free bits.  Those come from the construction, use, and destruction of
ClassInfoData objects.  The right way to address this, imo, is to create
nsIClassInfo2 (inheriting from nsIClassInfo), change all the function signatures
to pass in nsIClassInfo2 objects (to avoid the QIs) and add a

  [noscript] void getConstClassDescription([shared, retval] out string aResult);

method on it (see the getUTF8String method on nsIAtom).  That would avoid having
to do all that allocation and deallocation and immediately cut 12% off the time
spent in the security manager.

Next up is do_GetObjectPrincipal.  This seems to be slow because it walks up the
object chain till it gets to something implementing nsIScriptObjectPrincipal
(the document, basically), QIing everything in its path.  There are two
possibilities here.  Either we introduce a way to get the relevant
nsIScriptObjectPrincipal directly off some commonly used objects (DOM nodes,
style declarations, etc), or we could try caching the principal for a given
JSObject in a hashtable in the script security manager.  We would need to test
the speed of the second approach if we decide to try it, and we would need to
figure out a way to remove things from the hash when they are garbage collected
(perhaps any JS GC should clear out the hashtable?)

Finally, GetSubjectPrincipal.  Again, the only thing I can think of offhand is
caching, but I suspect we create/destroy JSContexts far too often for that to be
useful.  Someone with a better idea of how this stuff works may think of
something here?

Comment 1

[Boris Zbarsky [:bzbarsky]]

One more note.  Apart from the nsIClassInfo2 suggestion, all this would only
help sites that access a very few objects many many times in a row.  Luckily,
this describes most "dhtml".

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

bz asked me to mark this as security sensitive for now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

dbradley says that if we decide to cache the principal for a JSObject we could
use JS_SetGCCallbackRT to get notified when GC happens...

Brendan, any thoughts?  Also, any thoughts on caching the principal for a JSContext?

Comment 4

[Brendan Eich [:brendan]]

You can certainly cache parent chain search results for object principals, and
invalidate selectively when objects are finalized.  See JS_SetGCCallback{,RT}
and JS_IsAboutToBeFinalized in the JS API.

JSContexts do not come and go frequently, AFAIK.  They are 1:1 with
GlobalWindowImpls for the most part (there are a few bootstrapping or otherwise
miscellaneous JSContexts around too).  But subject principals come from running
the compiler, and they're held by the resulting JSScript.  Is it really that
slow to get the top stack frame, then get its script, then get the script's
principals struct pointer?

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well... that operation takes about 10% of the time spent in the security check;
about 2.5% of the total time spent in setting the attribute in general.

Probably not something we should worry about as much, especially if it would be
hard to fix.  Caching the object principal would be a much bigger win, though,
and we should give that a shot if possible.

Comment 6

[Brendan Eich [:brendan]]

We can flatten layers with a special jsdbgapi.h extension, that does

  return cx->fp->script->principals;

But I'm still wondering why those three loads, even with three or more layers of
functional abstraction, add up to so much cost.  I don't see that in the
profile, though.  Instead, I see GetSubjectPrincipal calling
doGetObjectPrincipal, to handle the cloned function object case.

So I think the cost is all in the object principal getting, and caching will help.

/be

Comment 7

[Brendan Eich [:brendan]]

The cloned function object case:

In JS, when you precompile a function for later execution against different
scope objects, the compiler memorizes the scope object passed to it, in the
parent slot of the function object, in case you happen to execute the script
(call the function) using that scope chain final object.  In that case, the
interpreter can use the precompiled function's script as is.

But if the compile-time scope object (static link) stored in the function
object's parent slot doesn't match the run-time scope in the active frame on the
current context, the interpreter (in special prolog bytecode, so once per
function declaration in a script, before the body of the script runs -- even if
the function is nested in a loop body) will clone the function object created by
the compiler.  Cloning makes another function object that shares (via refcounted
strong link) the underlying JSFunction private data, including the script, that
the compiler created.

When looking for subject principals, therefore, the cloned function object case
is special: instead of returning the function's script's principals, which came
from a pre-compilation epoch and are meaningless (null or chrome-ish), the caps
code must find the clone's object principals, via the parent-linked scope chain
that reachs a window object of some sort (the parent slot cannot be set by script).

Hope this helps.

/be

Comment 8

[Brendan Eich [:brendan]]

Forgot the punchline: the cloned function object's parent is the run-time scope
(the object that differed from the compile-time scope, triggering the cloning).
 Typically the run-time scope is a window object, although you could have nested
functions and event handlers where the parent-linked scope chain is longer than
one link.

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

To be precise, nsScriptSecurityManager::GetSubjectPrincipal just calls
GetPrincipalAndFrame, which does the real work.  It never actually calls
doGetObjectPrincipal.

The GetPrincipalAndFrame code does indeed walk up the frame stack.  If I
understand correctly it only needs to do this sometimes?  If so, is there a way
to tell when it needs to do it?

Of course I could just be totally misunderstanding what's going on; bear with
me, since my understanding of how JS works is rather incomplete... 

Comment 10

[Brendan Eich [:brendan]]

The walk up the stack is just skipping native (getter, setter, resolver,
function) JS stack frames.  It doesn't skip many.  I didn't study the profiling
data, but I would be surprised if much time was spent there.

The indirect call from GetSubjectPrincipals to doGetObjectPrincipal should be on
most of the hits that go through GetSubjectPrincipal (my mental powers say). 
How did I do?

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

The breakdown for GetSubjectPrincipal is: of 43 hits, 4 are under
JS_FrameIterator and 38 under GetPrincipalAndFrame.

Of those 38, 31 are under GetFramePrincipal, 2 under JS_GetFrameFunctionObject,
and 3 under JS_FrameIterator.

Of the 31 under GetFramePrincipal, 24 are under GetFunctionObjectPrincipal, 3
under JS_GetFrameFunctionObject.

Of the 24 under GetFunctionObjectPrincipal, 7 are under GetScriptPrincipal, 5
under JS_GetPrivate, 2 under nsPrincipal::Addref, 2 under
JS_GetScriptPrincipals, 2 under ~nsCOMPtr_base, only 1 under doGetObjectPrincipal.

Of the 7 hits under GetFunctionObjectPrincipal, 4 are under nsPrincipal::AddRef,
1 under PR_AtomicIncrement, 1 under JS_GetScriptForPrincipals.

Conclusion: we need to addref less and the mental powers were off.  ;)

Comment 12

[Brendan Eich [:brendan]]

Maybe we just need a JS_GetSubjectPrincipals(cx) API to cut down on all the
unoptimizable function boundaries and previously-only-for-the-debugger little
abstractions.  Patch in a little bit.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: JS_GetSubjectPrincipals API

The caps code can be simplified quite a bit using this new API.  See the
comments in jsapi.h, and anyone who knows caps, read through the implementation
in jsapi.c and double-check my remapping of the caps logic.

/be

Comment 14

[Brendan Eich [:brendan]]

Note that if JS_GetSubjectPrincipals returns non-null, it has held the
JSPrincipals, so no need to AddRef in C++-land (which for some bogus reason is
the thread-safe PR_AtomicIncrement flavor -- why oh why?  JS supports principals
only on one thread [old API bug], and we only ever use 'em on one thread).

Note also the important difference between a null return from
JS_GetSubjectPrincipals(cx, caps_GetObjectPrincipals, &script) with the script
out param non-null (meaning someone compiled a script with null principals, a
no-no in Mozilla) and null return with *script null (meaning nothing scripted
was active on cx's stack).  The former case should result in NS_ERROR_FAILURE,
as it does today; the latter case should lead without failure to the

    //-- If there's no principal on the stack, look at the global object
    //   and return the innermost frame for annotations.
    if (cx)
    {
        . . .

code in nsScriptSecurityManager::GetPrincipalAndFrame.

/be

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Brendan, the threadsafe addref/release issue is covered under the mantle of bug
143559.  I considered incorporating that patch into my recent large checkin to
caps, but decided to do it separately.  I should revive that bug now that my
other patches have landed...

Comment 16

[Brendan Eich [:brendan]]

Caillon -- thanks for the pointer.  Any chance you can apply the js/src patch
here and simplify caps via the JS_GetSubjectPrincipals API?

/be

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

I don't really know much about how this portion of caps works; my involvement so
far has been with different aspects of the principals code.  I will need to read
this stuff in more detail to be able to fully understand what is going on before
I mess around with the code.  I'll be spending tonight and tomorrow doing just
that, though.

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Wow, someone can go cross-eyed with all the various methods to grab principals
that exist in nsScriptSecurityManager.  Anyway, I think I have a better
understanding of what's going on here now than I did earlier and I think your
patch is good, Brendan.  Seems you mapped things right, and this definitely will
help things.

I do wonder whether it would be a good idea to change the out parameter of
JS_GetSubjectPrincipals() to a JSStackFrame rather than a JSScript.  Several
things need to get at the stack frame's annotations to check capabilities, and
doing this would probably alleviate the need for the loop in
nsScriptSecurityManager::GetPrincipalAndFrame() methinks.

I'll look more tomorrow and try and produce a patch.

Comment 19

[Brendan Eich [:brendan]]

Attachment: revised JS_GetSubjectPrincipals to return topmost active scripted frame via out param

Caillon: good suggestion, give this a whirl.

/be

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Brendan, thanks.  I hacked up a quick patch and ran it on bz's testcase in bug
213943 and see a win of ~300ms out of ~12500ms (~2.5%) per run.  I'm going to
fine tune it and run jprof on it, since I kind of expected a larger win than
that based on bz's findings, but maybe not.

Of note, I split up the API provided into 2 functions: JS_GetSubjectPrincipals()
and JS_GetFramePrincipals(), with the former calling the latter in its loop. 
There was a caller in IsCapabilityEnabled() which walked up the stack looking
for a frame with a certain annotation.

More later.

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

I misread comment 5.  The 2.5% total win I see is dead on.  Patch to come later.

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Initial patch

Mainly attaching this so I don't lose it.  This is the patch I have that shows
the ~2.5% win on the testcase bz supplied.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 129696 [details] [diff] [review]
Initial patch

JS patch rework looks great!

>+JS_STATIC_DLL_CALLBACK(JSPrincipals*)
>+caps_GetObjectPrincipals(JSContext *cx, JSObject *obj)
>+{
>+    nsCOMPtr<nsIPrincipal> principal;
>+    nsScriptSecurityManager::doGetObjectPrincipal(cx, obj,
>+                                                  getter_AddRefs(principal));
>+    JSPrincipals *jsprin;
>+    if (principal)
>+        principal->GetJsPrincipals(&jsprin);
>+    else
>+        jsprin = nsnull;

Better to initialize jsprin to nsnull and eliminate the else clause, since you
don't check the GetJsPrincipal r.v.

>+
>+    return jsprin;
>+}
>+
>+static inline nsIPrincipal*
>+JSPrincipalsToIPrincipal(JSPrincipals *jsprin)
>+{
>+    nsJSPrincipals *nsjsprin = NS_STATIC_CAST(nsJSPrincipals*, jsprin);
>+    return nsjsprin->nsIPrincipalPtr;
>+}

Note reference below: isn't it the case that (jsprin != nsnull) => (this
function's return value will be non-null) ?

>-nsresult
> nsScriptSecurityManager::GetFunctionObjectPrincipal(JSContext *cx,
>                                                     JSObject *obj,
>                                                     nsIPrincipal **result)
> {
>     JSFunction *fun = (JSFunction *) JS_GetPrivate(cx, obj);
>     JSScript *script = JS_GetFunctionScript(cx, fun);
> 
>-    nsCOMPtr<nsIPrincipal> scriptPrincipal;
>+    *result = nsnull;
>+
>     if (script)
>     {
>         if (JS_GetFunctionObject(fun) != obj)
>         {
>             // Function is a clone, its prototype was precompiled from
>             // brutally shared chrome. For this case only, get the
>             // principals from the clone's scope since there's no
>             // reliable principals compiled into the function.
>             return doGetObjectPrincipal(cx, obj, result);
>         }

It's a shame we need this in caps, and the very similar JS_GetFramePrincipals
code in js/src.  I'm in a hurry, but I suspect there's a way to unify further
and avoid the magic clone special case in caps as well as in js/src.

> 
>-        if (NS_FAILED(GetScriptPrincipal(cx, script,
>-                                         getter_AddRefs(scriptPrincipal))))
>-            return NS_ERROR_FAILURE;
>-
>+        JSPrincipals *jsprin = JS_GetScriptPrincipals(cx, script);
>+        if (jsprin) {
>+            *result = JSPrincipalsToIPrincipal(jsprin);
>+            if (*result) {

Doesn't jsprin => *result here, per above?

> nsresult
> nsScriptSecurityManager::GetFramePrincipal(JSContext *cx,
>                                            JSStackFrame *fp,
>                                            nsIPrincipal **result)
> {
>-    JSObject *obj = JS_GetFrameFunctionObject(cx, fp);
>-    if (!obj)
>-    {
>-        // Must be in a top-level script. Get principal from the script.
>-        JSScript *script = JS_GetFrameScript(cx, fp);
>-        return GetScriptPrincipal(cx, script, result);
>+    JSPrincipals *jsprin = JS_GetFramePrincipals(cx, fp,
>+                                                 caps_GetObjectPrincipals);
>+    nsresult rv = NS_OK;
>+    if (jsprin) {
>+        NS_IF_ADDREF(*result = JSPrincipalsToIPrincipal(jsprin));
>+        if (!*result) {
>+            NS_ERROR("Null principal for non-native function!");
>+            rv = NS_ERROR_FAILURE;
>+        }
>     }

More of the same: if (jsprin) then (*result != nsnull).

>+nsScriptSecurityManager::GetPrincipalAndJSStackFrame(JSContext *cx,
>+                                                     nsIPrincipal **result,
>+                                                     JSStackFrame **frameResult)
> {
>-    // Get principals from innermost frame of JavaScript or Java.
>-    JSStackFrame *fp = nsnull; // tell JS_FrameIterator to start at innermost
>-    for (fp = JS_FrameIterator(cx, &fp); fp; fp = JS_FrameIterator(cx, &fp))
>+    if (cx)
>     {
>-        if (NS_FAILED(GetFramePrincipal(cx, fp, result)))
>-            return NS_ERROR_FAILURE;
>-        if (*result)
>-        {
>-            *frameResult = fp;
>+        JSStackFrame *fp = nsnull;
>+        JSPrincipals *jsprin =
>+            JS_GetSubjectPrincipals(cx, caps_GetObjectPrincipals, &fp);
>+
>+        if (jsprin) {
>+            if ((*result = JSPrincipalsToIPrincipal(jsprin))) {
>+                NS_ADDREF(*result);
>+                *frameResult = fp;
>+            } else if (fp) {
>+                *frameResult = nsnull;
>+                NS_ERROR("Null principal for non-native function!");
>+                return NS_ERROR_FAILURE;
>+            }

Even more of the same, but here it seems to me you miss the case where (!jsprin
&& fp), which should result in an NS_ERROR and failure return.	The old code
did that, IIRC.

> nsresult
> nsScriptSecurityManager::GetSubjectPrincipal(JSContext *cx,
>                                              nsIPrincipal **result)
> {
>     JSStackFrame *fp;
>-    return GetPrincipalAndFrame(cx, result, &fp);
>+    JSPrincipals *jsprin = JS_GetSubjectPrincipals(cx,
>+                                                   caps_GetObjectPrincipals,
>+                                                   &fp);
>+    nsresult rv = NS_OK;
>+    if (jsprin) {
>+        NS_IF_ADDREF(*result = JSPrincipalsToIPrincipal(jsprin));
>+        if (!*result && fp) {
>+            NS_ERROR("Null principal for non-native function!");
>+            rv = NS_ERROR_FAILURE;
>+        }
>+    } else {
>+      *result = nsnull;
>+    }

Ditto, and the indentation shifts to 2-space from 4-space c-basic-offset in the
else.

> NS_IMETHODIMP
> nsScriptSecurityManager::IsCapabilityEnabled(const char *capability,
>                                              PRBool *result)
> {
>-    nsresult rv;
>     JSStackFrame *fp = nsnull;
>     JSContext *cx = GetCurrentJSContext();
>     fp = cx ? JS_FrameIterator(cx, &fp) : nsnull;
>     if (!fp)
>     {
>         // No script code on stack. Allow execution.
>         *result = PR_TRUE;
>         return NS_OK;
>     }
>+
>     *result = PR_FALSE;
>-    nsCOMPtr<nsIPrincipal> previousPrincipal;
>+
>+    nsIPrincipal *previousPrincipal = nsnull;
>     do
>     {
>-        nsCOMPtr<nsIPrincipal> principal;
>-        if (NS_FAILED(GetFramePrincipal(cx, fp, getter_AddRefs(principal))))
>-            return NS_ERROR_FAILURE;
>+        nsIPrincipal *principal = nsnull;
>+        JSPrincipals *jsprin = JS_GetFramePrincipals(cx, fp,
>+                                                     caps_GetObjectPrincipals);
>+        if (jsprin) {
>+            principal = JSPrincipalsToIPrincipal(jsprin);
>+            if (!principal) {
>+                return NS_ERROR_FAILURE;
>+            }

Again, the caps code should know that (jsprin => principal) here.

I may have missed something elsewhere in the patch, but I think the above needs
to be addressed (or I've really missed the boat!).

/be

Comment 24

[Brendan Eich [:brendan]]

Why is this a security-sensitive bug?

/be

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, no we never set the principal pointer on nsJSPrincipals until someone
calls GetJsPrincipals() which seems broken.  I'll fix that.

Comment 26

[Brendan Eich [:brendan]]

Can we get this in soon for 1.6a testing?  Take some of the heat off caps and
let it fall more on xpconnect and js in bug 220603 ;-).

/be

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Sure, I want to re-land the principal re-factoring first though to avoid more
patch munging.  Waiting on dveditz there.  I'll ping him.

Comment 28

[Johnny Stenback (:jst)]

Re the cost if using the current nsIClassInfo methods in caps I think there
might be better ways to eliminate that than to invent an nsIClassInfo2
interface. Caillon and I threw some ideas around about eliminating a bunch of
the work that caps does today by doing some intelligent caching of the result of
a check and getting things down to the fast-path through caps being a simple
hashtable lookup, and the not so fast path would be what we have today. We think
it might be doable, and I'd much rather see that done than yet one more interface.

Comment 29

[Boris Zbarsky [:bzbarsky]]

So just hash the desired info on the nsIClassInfo pointer?  Since for DOM nodes
the nsIClassInfos are in fact singleton-ish?  That would work...

Comment 30

[Boris Zbarsky [:bzbarsky]]

For reference, in current builds the time spent in security manager on typical
DHTML testcases is roughly comparable to the time spent in reflow....  Any
chance of making progress on the patches in this bug?

Comment 31

[Benjamin Smedberg]

Or just redesign CAPS ;) http://wiki.mozilla.org/Mozilla2.0?CAPSSecurity

Comment 32

[Markus Hübner]

Is there any progress going on or is any further input/help needed?

Comment 33

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

Can someone attach a minimal testcase for this bug, or set testcase-needed in Whiteboard?

Furthermore, Boris Zbarsky seems to not respond to Comment 32 posted 2 years ago. Can someone reassign the bug to NOBODY?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Er... why should I specifically (or anyone else for that matter) respond to a comment on this bug, especially one that basically has nothing to do with getting this bug fixed?  Then again, why am I responding to yours?  :(

The dependencies of this bug have plenty of testcases.

Comment 35

[Brendan Eich [:brendan]]

Lucas: first, Boris was not addressed by name in comment 32. There are others who can answer, but it may be that Markus was posing a question to "everyone", which means "no one" in practice.

Second, this is a meta bug. As bz notes in comment 34, the bugs that block this bug are more concrete reports that contain test cases.

/be

Comment 36

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

(In reply to comment #35)
> Lucas: first, Boris was not addressed by name in comment 32.

Excuse me, I thought it was assigned to Zbarsky, and not to Christopher Aillon.

> Second, this is a meta bug.

Before you add the "meta" keyword it was not so clear :-) Can you add also [META] as title prefix?

Well, anyway I think a meta bug don't need to be Assigned. I think Aillon can reassign it to NOBODY, as I suggested before.

Comment 37

[Brendan Eich [:brendan]]

caillon, you are not actively owning this, right? It should go to nobody for now if that's the case.

/be

Comment 38

[Gregory Pappas [:gregp]]

Inactive metabug