289655 - Consider a faster way to get principal from XPCWrappedNative

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This came up brendan and jst were working on the wrapper-sharing thing.

Basically, we currently walk all the way up the JSObject parent chain to the
window or document when we need to get an object principal, then return the
resulting document (or window) principal.

Brendan proposed short-circuiting this by grabbing the mJSGlobalObject of the
XPCWrappedNativeScope and getting the principal off of that.  I tried
implementing that (actually caching the pointer to the window on the scope), and
it speeds up the security check a lot (by about 60% when combined with the patch
in bug 289643).

I do have two concerns however:

1)  The principal of a window changes when a new document is loaded.  This means
that the principal of a node may effectively change at some point in this new
setup, to whatever is currently loaded in the window the node "belongs to". 
While I haven't found a way to exploit this yet, this worries me.  Jesse, if you
have testcases (from old bugs?) that try to do this sort of thing, especially
ones using unload handlers, timers, etc, could you point me to them so I can test?

2)  Holding a ref to the window in the XPCWrappedNativeScope looks like it makes
the window not actually go away till the scope is reused or something?  At least
I had to open a new window to get the --DOMWINDOW thing in my console, but that
may have been just a GC issue.  In any case, this also worries me.

For item #1, if it really is an issue the simplest resolution would be not to
reuse XPCWrappedNativeScopes across document loads, but it looks like we go to
some trouble to do that, so I assume there was a reason for it.

Thoughts?  Comments?  Suggestion?  Brendan says he wants this, or something like
it, for 1.8b, but I'm not quite happy enough with the state of this approach as
things stand (I just don't know the code involved well enough).

Comment 1

[Brendan Eich [:brendan]]

bz: there's already an mGlobalJSObject raw pointer that appears to be weak in
XPCWrappedNativeScope.  Did you make it strong, or add another one, or what?

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Did you make it strong, or add another one, or what?

Getting the nsIPrincipal from mGlobalJSObject was still pretty slow.  It
required the following steps:

1)  Get the private nsISupports.
2)  QI to nsIXPConnectWrappedNative
3)  Get the "native" pointer of this
4)  QI to nsIScriptObjectPrincipal
5)  Get the nsIPrincipal from this.

I added a cached pointer to the result of step 4 to the scope.  Being a QI
result on the window, it does rather have to be a strong ref.

In any case, attaching the patch in a sec so you can look for yourself.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Current patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ok, I did some more thinking and I _really_ don't like the approach in that patch.

Then I did some more thinking and realized that even in our current system the
principal of a node can change (because the principal of its document can change
-- see, eg, nsXMLDocument::Load and such beasties).  That seems wrong to me...

At this point I have some questions about the behavior we want here:

1)  Do we want to ensure that the principal of a wrapped native never changes
    once it's been fetched once?  If so, we need to store the principal on some
    object associated to the wrapped native which isn't going to change (most
    simply, on the XPCWrappedNative object).

The other two questions are based on the assumption that question #1 is answered
in the affirmative.  If question #1 is answered in the negative, then the patch
I already attached is probably "good enough".  I still don't like that idea, though.

2)  Do two XPCWrappedNatives that have the same XPCWrappedNativeProto
    necessarily have the same principal?  I did some testing, and we don't seem
    to have too many XPCWrappedNativeProtos around -- loading a dozen and a half
    pages with some heavy DHTML usage in them showed about 300 total protos
    alive at any one time, with about 400 distinct protos existing during the
    app lifetime.  So if memory and is somewhat a concern, storing the principal
    on the XPCWrappedNativeProto may be something to consider.

3)  If that's not desirable because it would use too much memory, perhaps we
    could insert a shim between the XPCWrappedNative(Proto) and the
    XPCWrappedNativeScope that exposes the principal.  This could be inserted
    any time we have to ask the scope for a principal, basically (and would
    store the then-active principal for the script global).  This may take some
    work to do because of lifetime management issues -- we want the shim to be
    created only if there isn't one already for that principal, which means we
    want the scope to hold on to it till it gets reinitialized.  We want the
    shim to hang around while there are XPCWrappedNative(Proto)s or scopes
    referencing it... or something.

If there's anyone else who knows something about our security model or xpconnect
who isn't cced here but should be, please let me know...

Comment 5

[Boris Zbarsky [:bzbarsky]]

So at the moment, I am considering the following model for this stuff, still
proceeding on my assumptions about when principals should or should not change):

1)  When an XPCWrappedNative that has a proto is initialized, it checks whether
    its mIdentity implements nsIScriptObjectPrincipal and stores this
    information on its mMaybeScope/mMaybeProto pointer (there's a free bit
    there).
2)  XPCWrappedNativeProto has an nsCOMPtr<nsIPrincipal> pointing to a principal.
3)  When we need to get the principal for an XPCWrappedNative, if the bit from
    #1 is set we just QI mIdentity to nsIScriptObjectPrincipal and return the
    principal.  This allows the principal of a document or window to change as
    needed.  Otherwise, we ask the proto (if any) for the principal.  If we
    still get null, we (somehow; not sure about how best to do this yet) get our
    principal and cache it on the proto.

If we don't want to change the behavior we have now for cases when the document
principal changes, I suspect it would be enough to cache pointers to the
nsIScriptObjectPrincipal on the proto instead of pointers to nsIPrincipal.

From my testing, it looks like very few wrapped natives _don't_ have a proto, so
this should handle the common case with minimal memory overhead.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Did some more thinking about how things now work....  At the moment, an
XPCWrappedNative can change its scope (and the JSObject's parent, for that
matter).  So caching principals in the security manager itself is more or less a
non-starter, though we could cache on the wrapper itself if we clear when we
reparent.

That said, another thought I had was to just make nsIContent inherit from
nsIScriptObjectPrincipal and return the principal of its OwnerDoc.  The problem
there is that we'd need to change some subclass QI impls and it'd _still_ be a
bit slow.  And only work for DOM nodes, not for, say the .style of an element.
We could make that impl the same interface and return the principal of its node,
but that's too whack-a-molish for my taste.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Take two

Remaining issues:

1)  XPCONNECT_STANDALONE vs moving some IDLs into XPConnect
2)  Whether it's better to have mDoc on the window or to add a method on
    nsIScriptGlobalObject to allow the document to sync the window's principal
    with it.

With this patch, doGetObjectPrincipal takes less time than getting and deleting
the classinfo strings...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Just the xpconnect and security manager changes

I've filed bug 313155 on making nsGlobalWindow::GetPrincipal faster; that's
going to be a slightly big patch...

Comment 9

[David Bradley]

Comment on attachment 200217 [details] [diff] [review]
Just the xpconnect and security manager changes

r=dbradley

One thing that concerns me is that if someone addes an IDL method after your newly added virtual function, I bet it won't be pointing to the right vtable entry. I suggest placing a comment warning of that.

That said, I think a better alternative is just to expose an API function to accomplish this. That would introduce a link time dependency from caps to XPConnect, and I don't know what the ramification of that would be. So given that, and the fact that hopefully this code will go away when more changes happen down the road for the principal/caps stuff, probably best to just take this.

This brings up a broader issue, though. If there are clients that need an optimized access to XPConnect we probably should look at exposing a C API that bypasses the XPCOM interfaces. But maybe most of this need is driven by other design issues that need to be addressed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I suggest placing a comment warning of that.

Will do.

Thanks for the review, David!

Comment 11

[David Bradley]

Oh, one quick thought. Couldn't this be marked as [notxpcom]. I'm going on memory here, as lxr appears to be sick at the moment. I think that might get around the vtable issue I mentioned, or at least it should.

Comment 12

[Boris Zbarsky [:bzbarsky]]

You mean put it in the IDL instead of in the {%C++ block and mark it "noscript,notxpcom"?  That would indeed work.  I'll do that.  Do you want to see the updated patch?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Except if I do that, can I still use #ifndef XPCONNECT_STANDALONE ?  I'd think we want that here....

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 200217 [details] [diff] [review]
Just the xpconnect and security manager changes

- In nsScriptSecurityManager::doGetObjectPrincipal():

+#ifdef DEBUG
+                if (aAllowShortCircuit)
+                {
+#endif
+                    result = xpcWrapper->GetObjectPrincipal();
+                    if (!result) fprintf(stderr, "nnnnn\n");

Remove the if check and the fprintf...

sr=jst

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  Looks like a 2-3% win on Tdhtml.