83536 - Merge 3 nsIPrincipal implementations into one

Status: RESOLVED FIXED

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

Description

[Mitchell Stoltz (not reading bugmail)]

Currently, we have several implementation types for nsIPrincipal,
nsCodebasePrincipal, nsCertificatePrincipal, nsAggreagtePrincipal (which groups
a codebase with a certificate), and nsSystemPrincipal, all of which inherit from
nsBasePrincipal. Merging the first three types into a single implementation
class has several advantages:

1) Clarity/ease of use: Currently, to see the URL associated with a principal,
callers have to get an nsIPrincipal and QI it to an nsICodebasePrincipal.
Merging these types saves a QI and makes call sites simpler.

2) Performance: All principals associated with documents are
AggregatePrincipals, which means that almost every function call on those
objects results in two virtual function calls: one to the AggregatePrincipal,
and one to the underlying CodebasePrincipal. Eliminating the indirection would
speed things up a bit.

The indirection provided by AggregatePrincipals is necessary for pages that set
document.domain, since this changes the document's principal, and the change
must be reflected both in the document's principal and the principal of any
event handlers compiled for that page. I think we can solve this by adding a
setDomain function to the principal itself.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Moving to post 0.9.2 as this is a pretty big change and needs lots of testing.

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Target is now 0.9.4, Priority P1.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

-> 0.9.5

Comment 4

[Mitchell Stoltz (not reading bugmail)]

time marches on. Retargeting to 0.9.6.

Comment 5

[Mitchell Stoltz (not reading bugmail)]

performance, footprint, feature work, and re-architecture bugs will be addressed
in 0.9.8

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Future

Comment 7

[Mitchell Stoltz (not reading bugmail)]

Chris is working on this.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

I should have something to look at by Wednesday.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

This should also fix bug 211174.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Work in progress

This patch pretty much works, as far as I can tell.  I recieved a regression
test suite, except it fails a bunch of things on both the trunk and a build
with my patch, both of which I used with a default profile (the test suite may
need some setup, which is not documented at all).  So, I don't have a good
baseline to test this against really.  From what I have tested though, things
look good.

So, everything seems to be in order I think.  I'm leaning towards getting
reviews here on this patch, landing early in beta, and plugging up any
regressions then.

Comment 11

[David Bradley]

Profiled this for bug 139986 and there's a marked improvement. The hash
functions were some of the top consumers of time, and this patch moves them way
down on the list.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Updated to trunk

Updated to trunk, a couple crashes fixed, and some random code cleanup.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

->beta

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 127790 [details] [diff] [review]
Updated to trunk

- In nsPrincipal::CanEnableCapability():

+  const char *start = capability;
+  *result = nsIPrincipal::ENABLE_GRANTED;
+  for(;;) {
+    const char *space = PL_strchr(start, ' ');
+    int len = space ? space - start : strlen(start);
+    nsCAutoString capString(start, len);
+    nsCStringKey key(capString);
+    PRInt16 value = (PRInt16)NS_PTR_TO_INT32(mCapabilities.Get(&key));
+    if (value == 0) {
+      value = nsIPrincipal::ENABLE_UNKNOWN;
+    }
+
+    if (value < *result) {
+      *result = value;
+    }
+
+    if (!space) {
+      break;
+    }
+
+    start = space + 1;
+  }

I know you didn't write this, but does this do the right thing if capabilities
are separated by more than one space? Same thing in all methods where we're
iterating over space-separated capabilities.

- In nsPrincipal::GetURI():

+  NS_ASSERTION(mCodebase, "All nsPrincipals should have a codebase");
+  NS_ADDREF(*aURI = mCodebase);

The old code protected against null URI's. I don't see anything that ensures
that the URI is non-null in the current code.

- In nsPrincipal::GetPreferences():

+  CapabilityList* capList = new CapabilityList();
+  capList->granted = &grantedListStr;
+  capList->denied = &deniedListStr;
+  mCapabilities.Enumerate(AppendCapability, (void*)capList);
...
+  return NS_OK;

Looks like this leaks capList. In fact, couldn't that be on the stack in stead
of coming from the heap? That would save you a malloc, fix the leak, and you
wouldn't need to add the missing OOM check after new.

- In FreeAnnotationEntry():

+  delete NS_STATIC_CAST(nsCStringKey*, aKey);

nsHashkey has a virtual destructor, no need for the cast.

- In nsPrincipal::Read():

+    nsHashtable *ht = new nsHashtable(aStream,
+		       ReadAnnotationEntry,
+		       FreeAnnotationEntry,
+		       &rv);
+    NS_ASSERTION(NS_SUCCEEDED(rv) || ht == nsnull,
+	    "failure but non-null return from nsHashtable ctor!");
+    if (NS_FAILED(rv)) {
+      return rv;
+    }

No OOM checking done here, except for in the assertion.

+    mCapabilities = nsHashtable(aStream,
+				 ReadAnnotationEntry,
+				 FreeAnnotationEntry,
+				 &rv);
+  }

Same here, need OOM checks.

- In nsScriptSecurityManager.cpp:

  *
  * ***** END LICENSE BLOCK ***** */
-#include "nsScriptSecurityManager.h"
...
+
+#include "nsScriptSecurityManager.h"

Why move this #include? Does it depend on other files include in this file? If
so, the header file itself should be fixed to include what it really depends on
in stead of spreading those dependencies out in the files that want to include
the header file.

- In nsScriptSecurityManager::SecurityCompareURIs():

+		     nsCOMPtr<nsIIOService> ioService(
+			 do_GetService(NS_IOSERVICE_CONTRACTID));
+		     if (!ioService)
+			 return NS_ERROR_FAILURE;

Wouldn't it make sense to cache this in a static member and release only on
shutdown?

- In nsScriptSecurityManager::GetCertificatePrincipal():

+    nsRefPtr<nsPrincipal> certificate = new nsPrincipal();
...
+    nsCOMPtr<nsIPrincipal> principal(do_QueryInterface(certificate));

No need to QI here.

...
+    if (fromTable)
+	 principal = fromTable;
+
+    NS_IF_ADDREF(*result = principal);

You know principal is non-null here, so use NS_ADDREF().

- In nsScriptSecurityManager::CreateCodebasePrincipal():

+    nsRefPtr<nsPrincipal> codebase = new nsPrincipal(aURI);
...
+    return CallQueryInterface(codebase.get(), result);

No need to QI here either, assign and addref.

- In nsScriptSecurityManager::JSEnabledPrefChanged():

+    mXPCDefaultGrantAll = NS_SUCCEEDED(rv) ? temp : PR_FALSE;

Maybe:

  mXPCDefaultGrantAll = NS_SUCCEEDED(rv) && temp;

to save us branching the code here.

- In nsScriptLoader::OnStreamComplete():

+/*    //-- Merge the principal of the script file with that of the document
     if (channel) {
       nsCOMPtr<nsISupports> owner;
       channel->GetOwner(getter_AddRefs(owner));
-      nsCOMPtr<nsIPrincipal> prin;
-      
-      if (owner) {
-	 prin = do_QueryInterface(owner, &rv);
-      }
+      nsCOMPtr<nsIPrincipal> prin = do_QueryInterface(owner);

       rv = mDocument->AddPrincipal(prin);
       if (NS_FAILED(rv)) {
	 mPendingRequests.RemoveObject(request);
	 FireScriptAvailable(rv, request, NS_LITERAL_STRING(""));
	 ProcessPendingReqests();
	 return NS_OK;
       }
     }
+*/

Uh? Don't we still want that code?

- In nsHTMLDocument::SetDomain():

+  nsresult rv = mPrincipal->SetDomain(newURI);

   // Bug 13871: Frameset spoofing - note that document.domain was set
   if (NS_SUCCEEDED(rv)) {
-    agg->SetDomainChanged(PR_TRUE);
     mDomainWasSet = PR_TRUE;
   }

-  return rv;
+  return NS_OK;

What's wrong with returning rv here? Seems like the right thing to do...

r=jst

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Fixes jst's comments

This fixes all of jst's comments with one exception, plus a few from dbradley
which he pointed out to me on irc, as well a few things I noticed myself.

The exception I mentioned is jst's first comment regarding
nsPrincipal::CanEnableCapability().  I am not sure I want to change that
behavior right now.  It may very well need to be changed, but I would feel more
comfortable doing that in a separate patch.  I will look into it some time in
the future.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 128060 [details] [diff] [review]
Fixes jst's comments

Boris, could you please sr this?

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

+nsPrincipal::GetURI(nsIURI** aURI)
+{
+  NS_ASSERTION(mCodebase, "All nsPrincipals should have a codebase");
+  NS_ADDREF(*aURI = mCodebase);

I swear I updated this per johnny's comment.  Somehow it did not appear in the
diff, though.  I'll verify that I fixed this in the correct tree when I am next
on my work machine.

Comment 18

[jag (Peter Annema)]

Comment on attachment 128060 [details] [diff] [review]
Fixes jst's comments

>Index: caps/src/nsPrincipal.cpp
>===================================================================
>+nsPrincipal::nsPrincipal(nsIURI *aURI, const char *aCertID)
>+  : mCachedSecurityPolicy(nsnull)
>+{
>+  if (aURI) {
>+    mCodebase = aURI;
>+  }
>+
>+  mCert = nsnull;

No need for this I think (nsAutoPtr is nsnull when default constructed).

>+  if (aCertID) {
>+    if (mCert = new Certificate(aCertID, nsnull)) {
>+      mCert->certificateID = aCertID;

Isn't certificateID already set in Certificate's constructor?

>+NS_IMETHODIMP
>+nsPrincipal::GetHashValue(PRUint32* aValue)
>+{
>+  nsCAutoString str;
>+
>+  // If there is a certificate, it takes precendence.
>+  if (mCert) {
>+    nsXPIDLCString certString;
>+    GetCertificateID(getter_Copies(certString));
>+    str = certString;

You could avoid an unnecessary string copy here by moving the HashCode call
into the |then| and |else| bodies. And another one if you just use
|nsCRT::HashCode(mCert->certificateID.get(), nsnull);|

Just a few things I noticed, no full review.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Updated patch

Fixes jag's comments, as well as the missing things I had changed in a
different tree, by mistake.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I'll try to get to this by tomorrow night.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128094 [details] [diff] [review]
Updated patch

>Index: caps/idl/nsIPrincipal.idl
>+    /**
>+     * Returns the security preferences associated with this principal.
>+     */
>+    void getPreferences(out string prefName, out string id,
>+                        out string grantedList, out string deniedList);

I assume "prefName" here would be a leafname of some sort?  Clarify, possibly?

>+     * Returns whether the principal is equivalent to this principal.

You mean whether "other" is equivalent to this principal?

>+    /**
>+     * Returns the domain security policy of the principal.
>+     */
>+    attribute voidPtr securityPolicy;

What happens if you set this?

>+    /**
>+     * Returns the URI to which this principal pertains.
>+     */
>+    attribute nsIURI URI;

"Returns the URI for which this principal provides security information",
maybe?	Or is that not what it does?

What happens if you set this?

>+    /**
>+     * Returns the Domain URI to which this principal pertains.
>+     * This is congruent with document.domain.
>+     */
>+    attribute nsIURI domain;

What happens if you set this?
If document.domain is not set, will this be null or equal to URI?

>+    /**
>+     * Returns the fingerprint ID of this principal's certificate.
>+     * This may not be unique.
>+     */
>+    attribute string certificateID;
>+
>+    /**
>+     * Returns the common name for the certificate.
>+     * This pertains to the certificate authority organization.
>+     */
>+    attribute string commonName;

What do these do in cases when hasCertificate is false?  Do they throw?  If so,
what?

Bonus points for some javadoc love, but feel free to spin that off into a
separate bug.

>Index: caps/include/nsPrincipal.h
>+public:
>+  NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr);
>+  NS_IMETHOD_(nsrefcnt) AddRef(void);
>+  NS_IMETHOD_(nsrefcnt) Release(void);

How about

NS_DECL_ISUPPORTS_INHERITED

with a comment that you are doing that because you will be doing black
refcounting magic?

>Index: caps/src/nsPrincipal.cpp

>+nsPrincipal::GetJsPrincipals(JSPrincipals **jsprin)
>+  JSPRINCIPALS_HOLD(cx, *jsprin);

Lucky us that JSPRINCIPALS_HOLD does not use the first arg... could we comment
to that effect here?

>+nsPrincipal::GetOrigin(char **origin)

Should this be taking the domain into account somehow?

>+nsPrincipal::InitFromPersistent(const char* aPrefName,
>+    rv = mJSPrincipals.Init(ToNewCString(token));

Ewww... this mismatches allocators with the PL_strfree in ~nsJSPrincipals

Maybe use PL_strdup(token.get()) here or something?

Should this function clear mAnnotations?

>+nsPrincipal::GetPreferences(char** aPrefName, char** aID,
>+                            char** aGrantedList, char** aDeniedList)

>+  *aPrefName = ToNewCString(mPrefName);

>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

Is caller expected to clean up *aPrefName on that error return?  That's not the
way things usually work... (as in, you should clean up that string allocation,
imo).  Similar for other early returns in this function.

>Index: caps/src/nsScriptSecurityManager.cpp
>@@ -820,59 +786,55 @@ nsScriptSecurityManager::CheckSameOrigin
>+    nsCOMPtr<nsIURI> subjectURI;
>+    nsCOMPtr<nsIURI> objectURI;
>+    aSubject->GetURI(getter_AddRefs(subjectURI));
>+    aObject->GetURI(getter_AddRefs(objectURI));
>+
>     PRBool isSameOrigin = PR_FALSE;
>-    nsresult rv = aSubject->Equals(aObject, &isSameOrigin);
>+    nsresult rv = SecurityCompareURIs(subjectURI, objectURI, &isSameOrigin);

Does this need to deal with document.domain in any way?  Or does the return
value of GetURI already account for the domain?

> nsScriptSecurityManager::InitDomainPolicy(JSContext* cx,
>     rv = mPrefBranch->GetChildList(policyPrefix.get(),
>-				   &prefCount, &prefNames);
>+				                   &prefCount, &prefNames);

What's with the indent here?

>Index: caps/src/nsSystemPrincipal.cpp
>+nsSystemPrincipal::GetURI(nsIURI** aURI)
>+{
>+  return NS_OK;
>+}

Set the out param to null?

>+nsSystemPrincipal::GetCertificateID(char** aID)
>+{
>+  return NS_OK;
>+}

Same.

>+nsSystemPrincipal::GetCommonName(char** aName)
>+{
>+  return NS_OK;
>+}

Same.

>+nsSystemPrincipal::GetDomain(nsIURI** aDomain)
>+{
>+  return NS_OK;
>+}

Same.

>+nsSystemPrincipal::GetSecurityPolicy(void** aCachedSecurityPolicy)
>+{
>+    return NS_OK;
>+}

Same.

The rest looks pretty reasonable.

I did not carefully check the callers of GetURI/GetDomain/GetOrigin to see what
they should be calling; in part this is because the IDL does not clearly
describe how those three items differ.	Some clearer comments would very much
help there.

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Addressing comments

- Does not include the removed files. Just assume they are still being removed
;-)
- Fixes bz's comments.
- Utilizes the cached sIOService in nsScriptSecurityManager to optimize
  NS_NewURI callers
- Fixes a few additional style nits I found.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128377 [details] [diff] [review]
Addressing comments

>Index: caps/idl/nsIPrincipal.idl
>+     * pertain.  id is a psuedo-unique identifier, pertaining to either 

"pseudo"

>+    attribute string certificateID;
>+    attribute string commonName;

Still need to document what attempts to get these do when hasCertificate is
false.

>Index: caps/src/nsPrincipal.cpp

>+nsPrincipal::GetPreferences(char** aPrefName, char** aID,
>+                            char** aGrantedList, char** aDeniedList)
>+  if (NS_FAILED(rv)) {
>+    nsMemory::Free(aPrefName);
>+    return rv;
>+  }

How about setting the out param to null too?  And that should be *aPrefName. 
So:

nsMemory::Free(*aPrefName);
*aPrefName = nsnull;

>+      nsMemory::Free(aPrefName);
>+      nsMemory::Free(aID);

Same as above.

>+      nsMemory::Free(aPrefName);
>+      nsMemory::Free(aID);
>+      if (*aGrantedList) {
>+        nsMemory::Free(aGrantedList);

And here.

sr=bzbarsky with those fixed.

Comment 24

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in 07/23/2003 22:15.

Codesize reductions from tinderbox:

Brad:  ~21k
Comet: ~24k
Luna:  ~23k
Beast: ~11k

Had no noticable effect on Tp/Ts/Txul.

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Backout patch

This is going to get backed out at my (and drivers') request due to a sticky
issue that should bake in an alpha.  This is the patch to do the backout.

Additionally, I ran |cvs checkout| on the previous versions of the files I |cvs
remove|'d with this patch, and I will |cvs add| them back before I land (no
need for them to be displayed in this patch, though, IMO.  I will not be |cvs
remove|ing the nsPrincipal.* files since they will be turned back on in alpha,
and they will just be turned off by the makefile changes.

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Re-opening.

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 130154 [details] [diff] [review]
Backout patch

chofmann said I get a=chofmann if I get r+sr=jst.  wanna take a look jst?

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 130154 [details] [diff] [review]
Backout patch

r+sr=jst for backing this out.

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 130154 [details] [diff] [review]
Backout patch

verbal a=chofmann given this morning.

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 130154 [details] [diff] [review]
Backout patch

Backed out 08/21/2003 20:04 thru 20:06

Comment 31

[jag (Peter Annema)]

So it looks like this (in an improved form I hope) was checked in again. Could
you attach the patch?

Comment 32

[Christopher Aillon (sabbatical, not receiving bugmail)]

Jag, the patch on this bug effectively stayed the same, although, it got patched
in a separate bug which I landed simultaneously.  I just had some very minor
conflicts here which I resolved (the content sinks got split up, so I had to
move code from one file to another).  The patch to this patch was taken care of
in bug 216041.

Resolving fixed.