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

RESOLVED FIXED in mozilla1.8beta2

Status

()

P1
normal
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla1.8beta2
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

This mostly helps with GetSubjectPrincipal, actually...  The idea is to not
addref/release principals all over in our private code.
Created attachment 180130 [details] [diff] [review]
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").
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
Attachment #180130 - Flags: superreview?(brendan)
Attachment #180130 - Flags: review?(caillon)
Keywords: perf
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Summary: deCOMify script security manager's private helpers a bit → [FIX]deCOMify script security manager's private helpers a bit
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
Attachment #180130 - Flags: superreview?(brendan) → superreview+
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.
Attachment #180130 - Flags: review?(caillon) → review+
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...
Attachment #180130 - Flags: approval1.8b2?
Summary: [FIX]deCOMify script security manager's private helpers a bit → [FIXr]deCOMify script security manager's private helpers a bit

Comment 5

14 years ago
Comment on attachment 180130 [details] [diff] [review]
Proposed patch

a=asa
Attachment #180130 - Flags: approval1.8b2? → approval1.8b2+
Created attachment 180330 [details] [diff] [review]
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.
Attachment #180130 - Attachment is obsolete: true
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Any chance this fix caused bug 289925 (crash @ JS_StackFramePrincipals) Boris ?
There's always a chance of everything.  See my comments in that bug, though.
Yeah, this caused bug 289925 (or more precisely, the better-described bug 289991).
Depends on: 289991

Updated

12 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.