289643 - [FIXr]deCOMify script security manager's private helpers a bit

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

This mostly helps with GetSubjectPrincipal, actually...  The idea is to not
addref/release principals all over in our private code.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Chris, if you don't think you'll get to this pretty soon, please recommend
someone else to look?  I think we're trying to get these perf improvements into
1.8b2....

The idea of this patch is to change no functionality other than the addrefing
(in particular, I was careful with the methods that use null principal and
NS_OK return to mean, effectively, "system principal").

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 180130 [details] [diff] [review]
Proposed patch

-		     rv = doGetObjectPrincipal(cx,
-					       NS_REINTERPRET_CAST(JSObject*,
-								   aJSObject),
-					      
getter_AddRefs(objectPrincipal));
-		     if (NS_FAILED(rv))
+		     objectPrincipal =
+			 doGetObjectPrincipal(cx,
+					      NS_REINTERPRET_CAST(JSObject*,
+								  aJSObject));
+		     if (!objectPrincipal)

The rest of the patch seems to use C-style casts to (JSObject *), so this could
too (it'd make for shorter lines; realize this is pre-existing but since you're
changing it....).

Use prevailing brace style in GetPrincipalAndFrame and sr=me.

/be

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 180130 [details] [diff] [review]
Proposed patch

> ///////////////// Principals ///////////////////////
> NS_IMETHODIMP
> nsScriptSecurityManager::GetSubjectPrincipal(nsIPrincipal **result)
> {
>+    nsresult rv;
>+    *result = doGetSubjectPrincipal(&rv);
>+    if (NS_SUCCEEDED(rv))
>+        NS_IF_ADDREF(*result);
>+    return rv;
>+}

I know the naming was pre-existing, but how about |aSubjectPrin| or something? 
I've never liked the mixing of |result| and |rv| where |result| is not an
|nsresult|.  There are a few other places in the patch you could fix up too
(and probably a few others that exist in the file, for bonus points).


>-nsresult
>+// static
>+nsIPrincipal*
> nsScriptSecurityManager::GetFunctionObjectPrincipal(JSContext *cx,
>                                                     JSObject *obj,
>-                                                    nsIPrincipal **result)
>+                                                    nsresult *rv)
> {
>+    NS_PRECONDITION(rv, "Null out param");
>     JSFunction *fun = (JSFunction *) JS_GetPrivate(cx, obj);
>     JSScript *script = JS_GetFunctionScript(cx, fun);
> 
>-    nsCOMPtr<nsIPrincipal> scriptPrincipal;
>+    *rv = NS_OK;
>     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.

Think I could get you to fix up this comment to say "there aren't reliable
principals..." or something of the sort?

r=caillon, regardless of whether the items above are addressed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180130 [details] [diff] [review]
Proposed patch

Requesting approval for 1.8b2.	This is a pretty safe patch that helps with
security manager performance a bit.  Given that we're adding more callers of
this code for the wrapper bug, it's good to have it be as fast as possible...

Comment 5

[Asa Dotzler [:asa]]

Comment on attachment 180130 [details] [diff] [review]
Proposed patch

a=asa

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

> The rest of the patch seems to use C-style casts to (JSObject *)

As it happens, aJSObject is already a JSObject* there.	So I removed the cast
completely.

> Use prevailing brace style in GetPrincipalAndFrame

Done.

> but how about |aSubjectPrin|

Done (aSubjectPrincipal, actually).

> Think I could get you to fix up this comment

Done.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 8

[Peter van der Woude [:Peter6]]

Any chance this fix caused bug 289925 (crash @ JS_StackFramePrincipals) Boris ?

Comment 9

[Boris Zbarsky [:bzbarsky]]

There's always a chance of everything.  See my comments in that bug, though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, this caused bug 289925 (or more precisely, the better-described bug 289991).