324600 - Push GetPrincipal() up to nsINode

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The plan is to use the following setup:

1) nsNodeInfoManager has an mPrincipal
2) Whenever the document's principal changes it sets the right principal on
   the nsNodeInfoManager
3) We get rid of the lazy principal creation (since in practice it's
   useless -- we always create one)
4) We inline GetPrincipal() on nsINode (via mNodeInfo->NodeInfoManager())
5) We eliminate the GetDocumentAndPrincipal() stuff in nsContentUtils.

Anyone have any objections?

Notes to self:

A) When doing Reset(), need to set mPrincipal from the channel owner only
   conditionally.
B) Check to see whether there are current GetOwnerDoc()->GetPrincipal() callers
   that could be eliminated.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So you plan on letting nsNodeInfoManager::GetPrincipal return null until the document gets a mDocumentURI? That should be ok I guess as long as we make sure all callsites can deal.

If so we should try to deny access to any nodes without a principal and see if that works (i.e. rather then allowing access to all such nodes).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, for now the plan is that null will be a possible return value (as now) and callers will need to deal.

Going forward that may change, but separate bug.  ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Code that I should simplify or fix as I do this:

CheckSameOrigin method in extensions/xmlextras/base/src/nsDOMSerializer.cpp
Maybe nsXFormsUtils::CheckSameOrigin and callers?  Not sure.
nsImageFrame::TriggerLink.. need to sort out what principal it wants
URIUtils::ResetWithSource
Possibly XULContentSinkImpl::OpenScript
nsNodeInfoManager

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Tada!

This is really not as long as it looks... a lot is s/GetPrincipal()/GetNodePrincipal()/

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 209798 [details] [diff] [review]
Tada!

>Index: content/base/src/nsContentUtils.cpp

>@@ -571,143 +509,75 @@ nsContentUtils::CheckSameOrigin(nsIDOMNo
...
>+  // Make sure we have both principals
>+  NS_ENSURE_TRUE(trustedPrincipal && unTrustedPrincipal, NS_ERROR_UNEXPECTED);
> 
>   return sSecurityManager->CheckSameOriginPrincipal(trustedPrincipal,
>                                                     unTrustedPrincipal);

It would probably be worth to early-return here if the two principals are the same (should be the common case).

>Index: content/html/document/src/nsHTMLDocument.cpp
>@@ -2032,29 +2042,25 @@ nsHTMLDocument::OpenCommon(const nsACStr
...
>-  // Restore the principal to that of the caller.
>-  mPrincipal = callerPrincipal;
>-
>-  // Recover if we had a problem obtaining the caller principal. In
>-  // such a case we set the documents URI to be about:blank (uri is
>-  // set to that above already) and the appropriate principal will be
>-  // created as needed.
>-  if (!mPrincipal) {
>-    mDocumentURI = uri;
>+  // Restore the principal to that of the caller, if any.  Otherwise, just use
>+  // the principal we already have, which is the URI principal for |uri| that
>+  // we picked up in Reset().
>+  if (callerPrincipal) {
>+    SetPrincipal(callerPrincipal);
>   }

This seems like a change of behaviour. If we can't get the source principal setting to about:blank seems like a good idea.

Looks good otherwise. I want to have a second look through to make sure that you set the principal everywhere where needed. Especially the XULDocument changes got me a bit confused.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> It would probably be worth to early-return here if the two principals are the
> same (should be the common case).

Early return is the first thing GetSameOriginPrincipal does.  I could do a check here too, I suppose, but is the extra code worth it?  Let me know.

> This seems like a change of behaviour. 

Er... I was trying to make sure that the behavior did not change...

Let me see again.  Old behavior:

1) Get caller principal into callerPrincipal
2) If there is a caller principal, set |uri| to its URI.  Else set to
   about:blank.
3) Create a channel for |uri|.
4) Call Reset.  This nulls out mPrincipal and sets mDocumentURI to the channel
   URI.
5) Set mPrincipal to callerPrincipal.
6) If mPrincipal is null, set mDocumentURI to |uri|.

Note that step 6 is a no-op, since that already happened in step 4.  Also note that if callerPrincipal is null we will end up creating a principal based on |uri|, which will always be about:blank in that case, the next time GetPrincipal() is called.

The new behavior is:

1) Get caller principal into callerPrincipal
2) If there is a caller principal, set |uri| to its URI.  Else set to
   about:blank.
3) Create a channel for |uri|.
4) Call Reset.  This nulls out mPrincipal and sets mDocumentURI to the channel
   URI.  It then sets mPrincipal to the codebase principal created from that
   URI.
5) If callerPrincipal is not null, reset principal to callerPrincipal.

This looks like it should give the same result as before, no?

That said, I was thinking that it would make more sense to set callerPrincipal as the owner on the channel and then just have Reset() do the right thing and remove step 5.  Thoughts?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated with better principal handling in nsHTMLDocument like that

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 209866 [details] [diff] [review]
Updated with better principal handling in nsHTMLDocument like that

- In nsDOMImplementation::CreateDocumentType():

+  // XXXbz shouldn't this use the original document principal instead?
   nsCOMPtr<nsIPrincipal> principal;
   rv = nsContentUtils::GetSecurityManager()->
     GetCodebasePrincipal(mBaseURI, getter_AddRefs(principal));
   NS_ENSURE_SUCCESS(rv, rv);

Seems like it should indeed, wanna fix that while you're here, or in a separate bug?

- In nsGlobalWindow::WouldReuseInnerWindow():

   if (mOpenerScriptURL) {
     if (sSecMan) {
       PRBool isSameOrigin = PR_FALSE;
+      // XXXbz shouldn't we store the opener _principal_ and use
+      // CheckSameOriginPrincipal here instead?  That would make a lot more
+      // sense to me...

Yup, that would make more sense to me too.

This looks all good to me. sr=jst

Comment 9

[Boris Zbarsky [:bzbarsky]]

> wanna fix that while you're here, or in a separate bug?

That's bug 324601.

> Yup, that would make more sense to me too.

I'll do that in a followup too.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ugh, sorry, i forgot i never finished this review.

> > It would probably be worth to early-return here if the two principals are 
> > the same (should be the common case).
> 
> Early return is the first thing GetSameOriginPrincipal does.  I could do a
> check here too, I suppose, but is the extra code worth it?  Let me know.

Yeah, i think it would be nice to avoid the extra virtual call. IIRC this function was farily performance critical.

I'm still confused about the changes at
@@ -2097,24 +2076,32 @@ nsXULDocument::PrepareToLoadPrototype(ns

Not that they seem wrong, but why weren't they needed before?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Yeah, i think it would be nice to avoid the extra virtual call.

Will do.

> Not that they seem wrong, but why weren't they needed before?

Before we implemented nsXULDocument::GetPrincipal so we always just asked the proto.  Now that GetNodePrincipal is nonvirtual, we have to store the principal on the nodeinfo manager like all other document classes.  So at some point we have to get it from the proto and store it on the nodeinfo manager.  This is the right point for it.  ;)

Does that help?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 209866 [details] [diff] [review]
Updated with better principal handling in nsHTMLDocument like that

Yeah, looks good, sorry this fell off my radar for a while.

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments and trunk

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional build bustage fix

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: More build bustage fixing

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Third and final bustage fix

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in.  Looks like about 2k codesize savings if nothing else.  More work to be done in bug 324601.

Comment 18

[Allan Beaufour]

This is killing something in XForms:
We've got an XTF factory.
WARNING: NS_ENSURE_TRUE(principal) failed, file nsDocument.cpp, line 1271
WARNING: NS_ENSURE_TRUE(principal) failed, file nsDocument.cpp, line 1271
nsXFormsModelElement::Rebuild()
nsXFormsModelElement::Recalculate()
nsXFormsModelElement::Revalidate()
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file nsEditor.cpp, line 1278
nsXFormsModelElement::InitializeControls()
WARNING: NS_ENSURE_TRUE(trustedPrincipal && unTrustedPrincipal) failed, file nsContentUtils.cpp, line 531
###!!! ASSERTION: failed to set up original instance document: 'NS_SUCCEEDED(rv)', file nsXFormsInstanceElement.cpp, line 338
Break: at file nsXFormsInstanceElement.cpp, line 338

(http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsInstanceElement.cpp#312)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Allan, I bet the problem is that DOMImplementation is creating documents with no principal now.  I'll fix that in bug 324601 in the next few days.