Closed Bug 213946 Opened 20 years ago Closed 4 months ago

[META] Optimize script security manager

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Unassigned)

References

Details

(Keywords: meta, perf)

Attachments

(2 files, 1 obsolete file)

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?
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".
bz asked me to mark this as security sensitive for now.
Group: security
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?
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
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.
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
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
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
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... 
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
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.  ;)
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
Attached patch JS_GetSubjectPrincipals API (obsolete) — Splinter Review
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
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
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...
Caillon -- thanks for the pointer.  Any chance you can apply the js/src patch
here and simplify caps via the JS_GetSubjectPrincipals API?

/be
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.
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.
Caillon: good suggestion, give this a whirl.

/be
Attachment #128900 - Attachment is obsolete: true
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.
Status: NEW → ASSIGNED
I misread comment 5.  The 2.5% total win I see is dead on.  Patch to come later.
Attached patch Initial patchSplinter Review
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 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
Why is this a security-sensitive bug?

/be
Actually, no we never set the principal pointer on nsJSPrincipals until someone
calls GetJsPrincipals() which seems broken.  I'll fix that.
Blocks: 220603
Keywords: perf
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
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.
Depends on: 229391
Blocks: 203448
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.
So just hash the desired info on the nsIClassInfo pointer?  Since for DOM nodes
the nsIClassInfos are in fact singleton-ish?  That would work...
Summary: Optimize script security manager some more → Optimize script security manager (blocked on dveditz)
Summary: Optimize script security manager (blocked on dveditz) → Optimize script security manager
Priority: -- → P3
Blocks: 229391
No longer depends on: 229391
Blocks: 119646
Blocks: 243488
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?
Depends on: 289643
Depends on: 289655
Depends on: 311598
Depends on: 311600
Depends on: 313155
Depends on: 313157
Is there any progress going on or is any further input/help needed?
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?
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.
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
Keywords: meta
(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.
caillon, you are not actively owning this, right? It should go to nobody for now if that's the case.

/be
Summary: Optimize script security manager → [META] Optimize script security manager
QA Contact: carosendahl → caps
Assignee: caillon → nobody
Status: ASSIGNED → NEW
Severity: normal → S3

Inactive metabug

Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.