728250 - slimmer and faster JSPrincipals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

With a fresh profile during startup even with optimization from the bug 725576 we still read from the XDR cache about 900 principals. In all cases the principal is the system one, yet just to support that we do at least 5 indirect calls and one malloc/free pair per top-level scripts with a storage requirement of at least 20 bytes using a complex interaction between XDR and object input stream.

All this can be avoided if JS_XDRScript avoid any principal transcoding. Rather during decoding the caller should supply the necessary principals to XDR.

While looking at this I noticed that this pattern of too much indirection is rather widespread while working with JSPrincipals.

For example, the code assumes that any JSPrincipal that is stored in the script is an instance of nsJSPrincipals that contains a pointer to nsIPrincipal and uses simple static_cast<nsJSprincipal *>(jsPrincipal)->nsIPrincipalPtr. Yet to do the reverse action of translating the nsIPrincipal pointer into JSPrincipals* we use the virtual call that returns a strong reference to JSPrincipals* that must be dropped. This indirection and JSPRICIPALS_DROP can be avoided if nsJSprincipal simply subclasses both nsIPrincipal and JSPrincipals and any principal implementation then should subclass nsJSprincipal rather than storing its instance in one of its fields. Then one can use static_cast for any nsIPrincipal <-> JSPrincipal conversion.

Another unnecessary indirection is the implementation of JSSecurityCallbacks::findObjectPrincipals. It is defined in nsJSEnvironment which then calls the security manager via a virtual function. This can be avoided if the security manager implements JSSecurityCallbacks::findObjectPrincipals directly.

Yet another inefficiency is the JSPrincipals::codebase string. It is not used in the JS engine. Moreover, its uses outside the engine are not on performance-critical paths so the string can be removed completely and reconstructed as necessary from the fields stored in the principals.

It would be nice to address these inefficiencies.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Yet another inefficiency is the JSPrincipals::codebase string. It is not used in the JS
> engine.

Really?  Then I agree; we should just nuke it.

Comment 2

[Igor Bukanov]

(In reply to Boris Zbarsky (:bz) from comment #1)
> > Yet another inefficiency is the JSPrincipals::codebase string. It is not used in the JS
> > engine.
> 
> Really?  Then I agree; we should just nuke it.

Yes - if one simply moves the codebase field from JSPrincipals to nsJSPrincipals, then everything compiles and runs.

Comment 3

[Luke Wagner [:luke]]

(1) findObjectPrincipals can go away with c-p-g (because we can assert findObjectPrincipals(obj) == obj->compartment()->principals).

(2) If you remove codebase, can you provide a simple helper so that it is easy to get a human-readable string out of a principals?  This is most useful when debugging compartment assertions.

Comment 4

[Igor Bukanov]

(In reply to Luke Wagner [:luke] from comment #3)
> (2) If you remove codebase, can you provide a simple helper so that it is
> easy to get a human-readable string out of a principals?  This is most
> useful when debugging compartment assertions.

Is it OK to have a debug-only method like JS_PrintPrincipal(JSRuntime *rt, JSPrincpals *) or do you prefer to avoid any function calls during debugging?

Comment 5

[Luke Wagner [:luke]]

Compartment assertions are only in debug builds, so that sounds fine to me.  One request, could you instead make an #ifdef __cplusplus dump() member function of JSPrincipals (analogous to the newly added JSAtom::dump/JSObject::dump functions)?

Comment 6

[Igor Bukanov]

Attachment: part 1

This patch removes the codebase field from JSPrincipals and makes nsJSPrincipals to subclass both nsIPrincipals and JSPrincipals so a conversion between these types can be done via a simple static_cast without the need for any virtual calls. To trimmer JSPrincipals farther I moved the Destroy and Subsume function pointers to the security callbacks structure.

The patch also removed the JSContext:: version of the security callbacks as they are not used in FF since the web workers gained own runtime.

Comment 7

[Igor Bukanov]

Attachment: part 2

The patch here removes the transcoding support for principals delegating that to the callers. That allowed not only to remove the complex XDR - ObjectStream play but also the need to create ObjectStream in the first place when reading/writing scripts to the startup cache.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 598811 [details] [diff] [review]
part 1

Looks great from a JS perspective.  Great to see the runtime/context-security-callbacks split removed.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 598811 [details] [diff] [review]
part 1

>+JS_EXPORT_API(void)
>+JSPrincipals::dump()
> {
>+    } else if (debugToken == 0x7e2df9d2) {

This should really be a define/const/enum/

>--- a/caps/src/nsPrincipal.cpp
>+++ b/caps/src/nsPrincipal.cpp
>@@ -141,53 +141,39 @@ nsPrincipal::Init(const nsACString& aCer
>-  nsresult rv;
>+  nsresult rv = NS_OK;
>   if (!aCertFingerprint.IsEmpty()) {
>     rv = SetCertificate(aCertFingerprint, aSubjectName, aPrettyName, aCert);
>   }
>   return rv;

This could be

if (aCertFingerprint.IsEmpty()) {
  return NS_OK;
}

return SetCertificate(aCertFingerprint, aSubjectName, aPrettyName, aCert);

>+nsSystemPrincipal::GetSpec(nsACString &aStr)
>+{
>+    aStr.Assign(SYSTEM_PRINCIPAL_SPEC);
>+    if (!aStr.EqualsLiteral(SYSTEM_PRINCIPAL_SPEC)) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    return NS_OK;

Can't you do the Length() trick here? (Maybe == ArrayLength(SYSTEM_PRINCIPAL_SPEC) - 1.)

>--- a/content/base/src/nsFrameMessageManager.cpp
>+++ b/content/base/src/nsFrameMessageManager.cpp
>@@ -804,27 +805,25 @@ nsFrameScriptExecutor::LoadFrameScriptIn
>         JSScript* script =
>-          JS_CompileUCScriptForPrincipals(mCx, nsnull, jsprin,
>-                                         (jschar*)dataString.get(),
>+          JS_CompileUCScriptForPrincipals(mCx, nsnull,
>+                                          nsJSPrincipals::get(mPrincipal),
>+                                          (jschar*)dataString.get(),

While you're here, could you please make this static_cast<const jschar*>(dataString.get())?

>--- a/dom/base/nsJSEnvironment.cpp
>+++ b/dom/base/nsJSEnvironment.cpp
>   // Safety first: get an object representing the script's principals, i.e.,
>   // the entities who signed this script, or the fully-qualified-domain-name
>   // or "codebase" from which it was loaded.

Duplicated code... I'm afraid I don't really see how to avoid it :/

>@@ -1575,58 +1536,51 @@ nsJSContext::CompileScript(const PRUnich
>   // SecurityManager said "ok", but don't compile if aVersion is unknown.
>   // Since the caller is responsible for parsing the version strings, we just
>   // check it isn't JSVERSION_UNKNOWN.
>   if (ok && ((JSVersion)aVersion) != JSVERSION_UNKNOWN) {
>     JSAutoRequest ar(mContext);
> 
>     JSScript* script =
>         ::JS_CompileUCScriptForPrincipalsVersion(mContext,
>                                                  scopeObject,
>-                                                 jsprin,
>+                                                 nsJSPrincipals::get(aPrincipal),
>                                                  static_cast<const jschar*>(aText),
>                                                  aTextLength,
>                                                  aURL,
>                                                  aLineNo,
>                                                  JSVersion(aVersion));
>     if (script) {
>       NS_ASSERTION(aScriptObject.getScriptTypeID()==JAVASCRIPT,
>                    "Expecting JS script object holder");
>       rv = aScriptObject.set(script);
>     } else {
>       rv = NS_ERROR_OUT_OF_MEMORY;
>     }
>   }
> 
>-  // Whew!  Finally done.
>-  JSPRINCIPALS_DROP(mContext, jsprin);
>   return rv;

This can now use early returns... Feel like doing that?

Comment 10

[Igor Bukanov]

(In reply to Ms2ger from comment #9)
> >+JS_EXPORT_API(void)
> >+JSPrincipals::dump()
> > {
> >+    } else if (debugToken == 0x7e2df9d2) {
> 
> This should really be a define/const/enum/

Do you have any suggestion in which header it should be defined so it can be used both in caps/src and dom/workers ? Is it OK to to put it into caps/include/nsJSPrincipals.h and in include that under dom/workers?

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Igor Bukanov from comment #10)
> (In reply to Ms2ger from comment #9)
> > >+JS_EXPORT_API(void)
> > >+JSPrincipals::dump()
> > > {
> > >+    } else if (debugToken == 0x7e2df9d2) {
> > 
> > This should really be a define/const/enum/
> 
> Do you have any suggestion in which header it should be defined so it can be
> used both in caps/src and dom/workers ? Is it OK to to put it into
> caps/include/nsJSPrincipals.h and in include that under dom/workers?

bent might be able to answer that, I've CC'd him.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Since it's worker-specific we could put it in dom/workers/Workers.h

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 598811 [details] [diff] [review]
part 1

It'll take me a day or two to get to this.

Comment 14

[Igor Bukanov]

Attachment: part 1

The new patch incorporates nice suggestions from the comment 9.

Comment 15

[Igor Bukanov]

Attachment: part 2

The new patch reflects changes from the bug 725576.

Comment 16

[Igor Bukanov]

Attachment: v3

here is a rebased patch - the previous no longer applies to MC tip.

Comment 17

[Igor Bukanov]

Attachment: v4

The patch required a rebase.

Comment 18

[Igor Bukanov]

Attachment: v5

here is a rebased patch.

Boris, do you have time to review this?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yes, I've been working on it as we go.  It keep changing with no indication of what changed, which is complicating things a bit.... :(

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 603075 [details] [diff] [review]
v5

>bug 728250 - remove JSPrincipals::codebase
>
>try: -b do  -p all -u all -t none

This needs a real checkin comment, please.....  Probably one that describes the actual changes being made at a bit more length.

>+++ b/caps/idl/nsIPrincipal.idl
>+    [noscript] readonly attribute AUTF8String spec;

Perhaps scriptLocation?

The are two callers, one of which can handle failure in this method fine and the other of which ignores the return value....  I think just always returning NS_OK from this function, even on OOM, would be fine.

>+++ b/caps/include/nsJSPrincipals.h
>+++ b/caps/include/nsNullPrincipal.h
>+  // Our refcount is managed by nsJSPrincipal.  Use this macro to avoid an

nsJSPrincipals

>   // FIXME: bug 327245 -- I sorta wish there were a clean way to share the
>-  // mJSPrincipals munging code between the various principal classes without
>+  // nsJSPrincipal munging code between the various principal classes without

nsJSPrincipals, but once this patch lands this looks easy: just move the refcounting to nsJSPrincipals, right?

>+++ b/caps/include/nsPrincipal.h
>+  // Our refcount is managed by nsJSPrincipal.  Use this macro to avoid

nsJSPrincipals

>+++ b/caps/include/nsSystemPrincipal.h
>+    // Our refcount is managed by nsJSPrincipal.  Use this macro to avoid

nsJSPrincipals

>+++ b/caps/src/nsJSPrincipals.cpp
>+nsJSPrincipals::Destroy(JSPrincipals *jsprin)
>     // Note that we don't want to use NS_IF_RELEASE because it will try
>     // to set nsjsprin->nsIPrincipalPtr to nsnull *after* nsjsprin has
>     // already been destroyed.

This comment no longer seems relevant.  Take it out?

>+++ b/caps/src/nsPrincipal.cpp
>@@ -141,53 +141,37 @@ nsPrincipal::Init(const nsACString& aCer
>+  return aCertFingerprint.IsEmpty()
>+         ? NS_OK
>+         : SetCertificate(aCertFingerprint, aSubjectName, aPrettyName, aCert);

I would prefer using an actual if statement here.

>+++ b/caps/src/nsScriptSecurityManager.cpp
> nsScriptSecurityManager::Shutdown()
>-        JS_SetRuntimeSecurityCallbacks(sRuntime, NULL);

Why is this no longer needed?

>+++ b/dom/base/nsJSEnvironment.cpp
>@@ -1205,54 +1204,49 @@ nsJSContext::EvaluateStringWithValue(con
>   nsIPrincipal *principal = aPrincipal;
>+  nsCOMPtr<nsIPrincipal> globalPrincipalStrongRef;

Why not just make |principal| an nsCOMPtr?  That's assuming we really need the strong ref....

>@@ -1414,65 +1404,45 @@ nsJSContext::EvaluateString(const nsAStr
>   nsIPrincipal *principal = aPrincipal;
...
>+  nsCOMPtr<nsIPrincipal> globalPrincipalStrongRef;

Likewise.

You may want to have bent look over the worker changes; they seem fine to me.

I assume someone else is covering review for js/src.

>+++ b/js/xpconnect/src/XPCComponents.cpp
>@@ -1755,30 +1732,27 @@ class JSCompartmentsMultiReporter : publ
>+        path.Insert(js::IsSystemCompartment(c)
>+                    ? NS_LITERAL_CSTRING("compartments/system/")
>+                    : NS_LITERAL_CSTRING("compartments/user/"),

I seem to recall at least some compilers miscompiling this.... Maybe it's ok now, though.

I really like these changes in general!

I'd also really appreciate an interdiff for whatever changes you make to the patch if you want a follow-up reviews.

r=me with the nits fixed.

Comment 21

[Igor Bukanov]

(In reply to Boris Zbarsky (:bz) from comment #20)

> The are two callers, one of which can handle failure in this method fine and
> the other of which ignores the return value....  I think just always
> returning NS_OK from this function, even on OOM, would be fine.

Those two callers expects an instance of nsJSPrincipals, so in the updated patch I moved the function out from IDL to nsJSPrincipals.

> >+++ b/caps/src/nsScriptSecurityManager.cpp
> > nsScriptSecurityManager::Shutdown()
> >-        JS_SetRuntimeSecurityCallbacks(sRuntime, NULL);
> 
> Why is this no longer needed?

The patch moves the destroy callback from the JSPrincipals to the security manager structure. But at the point when nsScriptSecurityManager::Shutdown() is called the JS may still have not run the last GC and some principals may yet to be destroyed. To allow proper finalization of them the patch keeps the security callbacks intact. This should be fine as the callbacks are a static structure that outlives the JSRuntime.

Should I comment about that in the code?

EvaluateStringWithValue(con
> >   nsIPrincipal *principal = aPrincipal;
> >+  nsCOMPtr<nsIPrincipal> globalPrincipalStrongRef;
> 
> Why not just make |principal| an nsCOMPtr?  That's assuming we really need
> the strong ref....

I kept the strong ref at the code previously did. But I wanted to avoid useless AddRef/Release on aPrincipal for the common case when aPrincipal is not null. This is the reason for the extra variable.

> I assume someone else is covering review for js/src.

Luke already did that.

Comment 22

[Igor Bukanov]

Attachment: v6

Bent, can you have a look at web-workers related small changes that the patch makes?

Comment 23

[Igor Bukanov]

Attachment: v5 - v6 interdiff

Here are the changes since v5 to fix the comments. Most of the changes are related to moving GetScriptLocation from the idl to nsJSprincipals.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> This should be fine as the callbacks are a static structure that outlives the JSRuntime.

It's only fine if the methods in that structure don't use gScriptSecMan directly or indirectly....  There should probably be an audit to that effect and comments added to all relevant methods.

Alternately, maybe swap out the callbacks on security manager shutdown for a structure that only defines destroy and document that _that_ must not depend on the security manager?

> I kept the strong ref at the code previously did.

The code previously held a strong ref even if aPrincipal is not null.  If you're actually trying to preserve behavior, you should do that too; nothing guarantees that the caller has any more of a ref to aPrincipal than it would have via the nsIScriptObjectPrincipal in the !aPrincipal codepath...

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 603684 [details] [diff] [review]
v5 - v6 interdiff

r=me modulo the comments

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 603682 [details] [diff] [review]
v6

Review of attachment 603682 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

::: dom/workers/WorkerPrivate.cpp
@@ +156,5 @@
>  }
>  
>  NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(JsWorkerMallocSizeOf, "js-worker")
>  
> +struct WorkerJSRuntimeStats : public JS::RuntimeStats {

Nit: { on next line

@@ +161,5 @@
> +  WorkerJSRuntimeStats()
> +   : JS::RuntimeStats(JsWorkerMallocSizeOf) { }
> +
> +    virtual void initExtraCompartmentStats(JSCompartment *c,
> +                                           JS::CompartmentStats *cstats) MOZ_OVERRIDE {

Nit: { on next line

::: dom/workers/Workers.h
@@ +123,5 @@
>  WorkerCrossThreadDispatcher*
>  GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker);
>  
> +// Random unique constant to facilitate JSPrincipal debugging
> +const uint32_t JSPRINCIPALS_DEBUG_TOKEN = 0x7e2df9d2;

Nit: Since this is not a #define I think I'd prefer 'kJSPrincipalsDebugToken'.

Comment 27

[Igor Bukanov]

Attachment: v7

Comment 28

[Igor Bukanov]

Attachment: v6 - v7 interdiff

The new patch addresses the comments for the v6. In particular, it always keeps a strong reference to the passed JSPrincipal pointer in EvaluateStringWithValue/EvaluateString and moves the destroy principals callback outside of JSSecurityCallbacks into own JSRuntime field. This way the security manager can continue to clear the security callbacks on shutdown while the principals can be safely destroyed after the security manager is destroyed.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 604062 [details] [diff] [review]
v6 - v7 interdiff

> +    // The JS runtime can calls this method during the last GC when

s/calls/call/ and r=me

Comment 30

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f51a5ba84b56

Comment 31

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f51a5ba84b56