If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Sort out principals vs scopes situation

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: bz, Assigned: jst)

Tracking

({perf})

Trunk
mozilla1.9alpha8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [should block bug on cross-domain XMLHttpRequest])

Attachments

(2 attachments, 2 obsolete attachments)

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...
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.
Flags: blocking1.9a1?
(Assignee)

Comment 2

12 years ago
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...
Doing that would let us restore the status quo ante bug 289655, right?  But is that the behavior we actually want?
(Assignee)

Comment 4

12 years ago
IMO that is definately closer to what we want than what we have now.
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...
Blocks: 317497
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?
Depends on: 316794
(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
*** Bug 321287 has been marked as a duplicate of this bug. ***
Blocks: 322565
Shaver had another suggestion, which is to force the caller principal, if any, onto documents gotten via XMLHttpRequest.  Thoughts on that?
We should really reach a decision here one way or another; what do we want the behavior to be security-wise?
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?
Assignee: dveditz → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
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.
Assignee: bzbarsky → general
Component: Security → DOM
Priority: P1 → --
QA Contact: toolkit → ian
Target Milestone: mozilla1.9alpha → ---
Created attachment 220413 [details] [diff] [review]
Minimal change to disable
Attachment #220413 - Flags: superreview?(jst)
Attachment #220413 - Flags: review?(jst)
(Assignee)

Comment 14

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

r+sr=jst
Attachment #220413 - Flags: superreview?(jst)
Attachment #220413 - Flags: superreview+
Attachment #220413 - Flags: review?(jst)
Attachment #220413 - Flags: review+
Flags: blocking1.9a1? → blocking1.9a2?
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.
Attachment #220413 - Attachment is obsolete: true
Note to self:  When we give each document its own scope object, we should revisit bug 347524 comment 4.
Flags: blocking1.9a2? → blocking1.9+
Whiteboard: [should block bug on cross-domain XMLHttpRequest]
Blocks: 333906
Boris, would you be able to do this for 1.9? If not jst offered to take a look.
Assignee: general → 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.  ;)
Assignee: bzbarsky → jst
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...

Updated

11 years ago
Target Milestone: --- → mozilla1.9alpha6
(Assignee)

Comment 20

11 years ago
(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?
Blocks: 326337
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....
> 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?
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...
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Not really going to block on this
Flags: blocking1.9+ → blocking1.9-
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

10 years ago
So this blocks bug 375470 which is a significant perf regression - shouldn't this bug also block?
Flags: blocking1.9- → blocking1.9?
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...
reminusing. If someone thinks this would be a worthwhile place to spend time in order to improve performance, feel free to renominate.
Flags: blocking1.9? → blocking1.9-

Comment 30

10 years ago
Jonas/Bz - k thnx - do we have a plan for bug 375470 then?
(Assignee)

Comment 31

10 years ago
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.
(Assignee)

Comment 32

10 years ago
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.
(Assignee)

Updated

10 years ago
Flags: blocking1.9- → blocking1.9+
Priority: -- → P1
(Assignee)

Comment 33

10 years ago
Created attachment 299326 [details]
Re-enable optimization.
Attachment #299326 - Flags: superreview?(bzbarsky)
Attachment #299326 - Flags: review?(bzbarsky)
(Assignee)

Comment 34

10 years ago
Created attachment 299328 [details] [diff] [review]
Re-enable optimization and remove code we don't really need any more.
Attachment #299326 - Attachment is obsolete: true
Attachment #299328 - Flags: superreview?(bzbarsky)
Attachment #299328 - Flags: review?(bzbarsky)
Attachment #299326 - Flags: superreview?(bzbarsky)
Attachment #299326 - Flags: review?(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!
Attachment #299328 - Flags: superreview?(bzbarsky)
Attachment #299328 - Flags: superreview+
Attachment #299328 - Flags: review?(bzbarsky)
Attachment #299328 - Flags: review+
(Assignee)

Comment 36

10 years ago
Created attachment 299780 [details] [diff] [review]
Fix that was checked in.

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

Comment 37

10 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Keywords: perf
Depends on: 415324
You need to log in before you can comment on or make changes to this bug.