317240 - Sort out principals vs scopes situation

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

When I wrote the patch for bug 289655, I was assuming that there is only one relevant principal per XPCWrappedNativeScope (the latter correspond to script global objects).  Unfortunately, the asserts I added in that patch fire, indicating that this is not the case.

One obvious offender, from looking at stacks, is XMLHttpRequest.  What happens there is that XMLHttpRequest returns a new Document object, one that has its own principal (based on the URI loaded via XMLHttpRequest).  But there's only one scope around -- that of the caller, since the XMLHttpRequest document has no script global of its own.  Hence the JSObject for that document has the caller as __parent__.  And that caller's principal may not be the same as the document's principal, which triggers an assert.

Now before the patch for bug 289655, what would happen is that for the XMLHttpRequest document itself and for all nodes in it we'd end up with the XMLHttpRequest document's principal (due to the preCreate hook on the nodes).  But for various other objects (return values of .childNodes, getElementsByTagName(), .style, etc) we would end up with the calling scope as the principal for the newly created object.

I _think_ this is only an issue for cases where a document has no scope of its own, given that XPConnect uses the object we're working with as initial scope object for wrapping things.  But I'm not sure.

In any case, given the fact that the "one scope one principal" invariant doesn't hold, what do we want to do?  We could back out bug 289655 -- that means we'd still have issues for everything without a preCreate hook, but that might just be what we have to live with.  We could try to make this invariant hold, which means that all documents would at least have their own XPConnect scope (if not their own inner window).  That might be a pretty tough change, though...

Thoughts?  Please feel free to cc others on this bug if I've forgotten someone who might have an idea on this...

Comment 1

[Boris Zbarsky [:bzbarsky]]

For 1.9 we absolutely need to do something here, btw -- as things stand we're getting a different principal than we used to for basic cases like that XMLHttpRequest document, and that's almost certainly wrong.

Comment 2

[Johnny Stenback (:jst)]

The only thing that comes to mind here is to make XPCWrappedNative's know whether their native's principal match their scope's principal and based on that either pick a fast or slow path to the actual principal object. This would require an API to clear this state from all wrappers when an object's principal changes, and the wrappers would need to have the smarts to inherit this state from their parents etc. The state needed on the wrappers here would probably be two bits for representing "don't know", "match", and "mismatch", meaning whether the wrappers principals match that of it's scope or not, or if we don't know yet.

Not pretty, but that's the only way out here that I've thought of so far...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Doing that would let us restore the status quo ante bug 289655, right?  But is that the behavior we actually want?

Comment 4

[Johnny Stenback (:jst)]

IMO that is definately closer to what we want than what we have now.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I agree on that, but if the status quo ante is all we want we should probably just back out bug 289655.  The real question I had is what behavior things _should_ have...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Actually, once bug 316794 is fixed it might not be so bad to give every single document a script global -- some documents would have an inner window, but no outer... or something.  Does that sound like something worth trying?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Actually, once bug 316794 is fixed it might not be so bad to give every single
> document a script global -- some documents would have an inner window, but no
> outer... or something.  Does that sound like something worth trying?

Yes.  We should make all documents have lexical scope that ends in a window object (an inner window of course).  If there is no GUI window, there still ought to be something very much like an inner window.  I'm not convinced we need to abstract much here.

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

*** Bug 321287 has been marked as a duplicate of this bug. ***

Comment 9

[Boris Zbarsky [:bzbarsky]]

Shaver had another suggestion, which is to force the caller principal, if any, onto documents gotten via XMLHttpRequest.  Thoughts on that?

Comment 10

[Boris Zbarsky [:bzbarsky]]

We should really reach a decision here one way or another; what do we want the behavior to be security-wise?

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK.  So I don't see how I can give each document an inner window without leaking via the "document -> window -> JSContext -> root JSObject -> js stuff to document wrapper" path.

What I am now contemplating is creating an nsISupports object that will have weak refs to and from the document and only stay alive while JS keeps it alive (so while there are any wrappers for any of the document's objects around).

Does that seem like it should fly?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Actually, I take that back.  I started poking at it, and realized that I don't know enough about what I'd need to set up (eg, whether I need to call InitClasses, and if so whether it should just be the XPConnect version or the nsJSContext version, etc).  And I don't really have the time to learn before 1.9a1.  :(

So I'm just going to disable the optimization for now and leave this bug for someone who knows this stuff.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal change to disable

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 220413 [details] [diff] [review]
Minimal change to disable

r+sr=jst

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 220413 [details] [diff] [review]
Minimal change to disable

Checked this in on trunk.  I'd still like to fix this so we can reenable it...

Note that just munging the principal in XMLHttpRequest isn't going to work because you can get a similar effect with createDocument() and load(), and in that case there's no reason the load() and createDocument() callers will have the same principal.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Note to self:  When we give each document its own scope object, we should revisit bug 347524 comment 4.

Comment 17

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

Boris, would you be able to do this for 1.9? If not jst offered to take a look.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I somewhat doubt I will.  I've got multiple months of work already scheduled in.... :(

Though with the cycle collector this might be easier.... Just have everything own  everything else and give all documents inner windows.  ;)

Comment 19

[Boris Zbarsky [:bzbarsky]]

To clarify, I think we should make the inner window owned by the document and have it own the document.  Both implement cycle collection, so that should be OK.  We'll need to do something wacky when we reuse an existing inner window -- probably swap the inner windows between the two documents or something.  The inner window should be created in nsDocument::Init.

We'll also need to fix whatever code is current checking for a script global as a security or "script enabled for this document" check (and yes, it's sad, we have such code).

But once that's done, I think we should basically be ok more or less by default -- each document will have its own script global object, with its own XPConnect scope, and its own global JSObject.  Then we set the principal on the XMLHttpRequest documents to be whatever we think it should be, and refix the security manager fast path in bug 317497.

All this does involve having enough faith in the cycle collector and our use of it to be willing to introduce the strong refs in question.  It's probably a good idea to not do that till after the cycle collector collects on shutdown, so we'll see leaks if the happen...

Comment 20

[Johnny Stenback (:jst)]

(In reply to comment #15)
> Note that just munging the principal in XMLHttpRequest isn't going to work
> because you can get a similar effect with createDocument() and load(), and in
> that case there's no reason the load() and createDocument() callers will have
> the same principal.

I'm wondering if this wouldn't be worth thinking harder about now, especially with the cross origin XHR support that we're thinking about adding. In a case where we have cross origin XHR support we'll need to figure out how to get the caller of XHR access to the loaded document in the case where it came from another domain, as in if we do what we do now and give the resulting document a principal based on the loaded URI, we'll block script from accessing the result document that it should have access to.

IOW, what if we made XHR and DOMParser return a document that had the principal of the caller, and document.load() would change the documents principal to that of the caller (much like document.open() does). Would that not make it such that all objects within an XPConnect scope would share the same principal? What would this break?

Comment 21

[Boris Zbarsky [:bzbarsky]]

So load() would use the caller principal instead of the principal of the thing being loaded as it does now?  And same for XHR?

That should work, yes...  As long as the page calling load() is exactly the same as the page that first created the document.  And I mean same, not just same-origin, since the principals will be different otherwise.  I'm not sure we can enforce that...

I guess it might be enough to end up with principals that are same-origin no matter which way we go through GetObjectPrincipal, instead of insisting on object identity.  But it bothers me, to be honest.  :(

By the way, does document.open() reparent the document's wrapper to the new inner window?  I really hope it does....

Comment 22

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

> That should work, yes...  As long as the page calling load() is exactly the
> same as the page that first created the document.  And I mean same, not just
> same-origin, since the principals will be different otherwise.  I'm not sure
> we can enforce that...

Why isn't same-origin only ok? If the creating page and the opening page are able to access each other, does it really matter which of the two principals the opened document gets?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Same-origin is not a permanent condition.  For example, setting document.domain changes whether two pages are same-origin (in particular it can make pages that used to be same-origin not be same-origin anymore).  Now maybe that's ok in this particular case, but we'd need to actually make sure that it is...

Comment 24

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 25

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

Not really going to block on this

Comment 26

[Boris Zbarsky [:bzbarsky]]

Note that this is basically a performance bug... I agree that we shouldn't block on this, but it _is_ a nice DHTML perf win.  :(

Comment 27

[Mike Schroepfer]

So this blocks bug 375470 which is a significant perf regression - shouldn't this bug also block?

Comment 28

[Boris Zbarsky [:bzbarsky]]

This blocks that bug because it was one of the things that came up in the profile.  This bug isn't really the cause of the regression as far as I can tell, since it's about enabling an optimization we've never had enabled before...

Comment 29

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

reminusing. If someone thinks this would be a worthwhile place to spend time in order to improve performance, feel free to renominate.

Comment 30

[Mike Schroepfer]

Jonas/Bz - k thnx - do we have a plan for bug 375470 then?

Comment 31

[Johnny Stenback (:jst)]

There's still issues with re-enabling this patch as is, document.load in particular makes this break :(

But I think we still can re-enable this if we add code in places where we change principals to null out the XPCWrappedNativeScope's mScriptObjectPrincipal when we enter a situation where there can be more than one principal in a given scope. We could even only do that when there will be a principal that's not same origin in a given scope. That should give us the benefits of this optimization in the majority of the pages on the web by far.

Comment 32

[Johnny Stenback (:jst)]

The fix for bug 397791 made it so that a documents principal never changes, which means we can re-enable this optimization now. Re-blocking on this to keep this on the radar. Patch coming up.

Comment 33

[Johnny Stenback (:jst)]

Attachment: Re-enable optimization.

Comment 34

[Johnny Stenback (:jst)]

Attachment: Re-enable optimization and remove code we don't really need any more.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 299328 [details] [diff] [review]
Re-enable optimization and remove code we don't really need any more.

>+++ b/caps/src/nsScriptSecurityManager.cpp
>+                char ch = jsClass->name[0];

We don't need that anymore, do we?

With that, r+sr=bzbarsky.  Good to see this back on again!

Comment 36

[Johnny Stenback (:jst)]

Attachment: Fix that was checked in.

Correct, we don't need that, nor do we need the native pointer (use do_QueryWrappedNative() instead).

Comment 37

[Johnny Stenback (:jst)]

Fix checked in.