Documents should always know their global

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: krizsa)

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 12 obsolete attachments)

4.40 KB, patch
smaug
: review+
Details | Diff | Splinter Review
14.98 KB, patch
bholley
: review+
Details | Diff | Splinter Review
10.01 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.09 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.44 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.27 KB, patch
bholley
: review+
Details | Diff | Splinter Review
14.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
17.06 KB, patch
bholley
: review+
Details | Diff | Splinter Review
12.79 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.06 KB, patch
bholley
: review+
Details | Diff | Splinter Review
21.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.12 KB, patch
bholley
: review+
Details | Diff | Splinter Review
9.02 KB, patch
bholley
: review+
Details | Diff | Splinter Review
See http://pastebin.mozilla.org/1986339 for the full IRC discussion here

Right now, Document's don't always know their global - they hold onto an nsIScriptGlobalObject, but there are number of globals (particularly sandboxes, where we can have XHR documents) that don't implement SGO.

This means that nsNodeSH::PreCreate will end up creating a reflector for the Document (and consequently all descendant Nodes) in whatever scope uses it first. This is wacky, and can lead to spurious security exceptions.

I propose creating a new simplified interface with only one method, GetGlobalJSObject, that nsIScriptGlobalObject would inherit from (we could even [notxpcom] it if we wanted to). Then, we should make sure that Documents always know their global, and turn the bailout case in nsNodeSH::PreCreate into an assertion.
Sounds reasonable.

Comment 2

5 years ago
nsDOMEventTargetHelper should then probably bind to this simpler object, not necessarily to
window. That way the right global would be passed to document when XHR creates one.
(Assignee)

Updated

5 years ago
Blocks: 820094
(Assignee)

Updated

5 years ago
Blocks: 786681
(Assignee)

Comment 3

5 years ago
If no one is actively working on this already I'll take this one, since for me this is P1.
All yours.
Assignee: nobody → gkrizsanits
(Assignee)

Comment 5

5 years ago
I have a bunch of questions after reading some code and doing some initial work on this. So first in nsNodeSH::PreCreate I don't understand why do we need that voodoo with WrappNative instead of just QI and call GetGlobalJSObject once we got the scope object with doc->GetScopeObject?

I think I'm also missing something in general from the approach of introducing this new simplified interface (let's call it now nsIGlobalJSObjectHolder). So for setters I can change function signatures probably and accept a pointer to this new interface as input, for storing it in the member variables (with or without mergeing them) I don't see a big problem since they are weak pointers, so they don't have to be changed. But for getters like GetScopeObject I cannot really change the return type that simply. A lot of consumer of these getters expect and needs a nsIScriptGlobalObject. (I can ofc QI to it at any point of code they are used, but I don't think that is the right thing to do)

I could create a special getter like GetGlobalOfDocument or something on the document for this, but that would still leave us getter and setter for the same member (mScopeObject) with different types (nsIGlobalJSObjectHolder vs nsIScriptGlobalObject)... I feel like I'm missing something obvious here from that irc discussion. Could someone quickly help me out here?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> I have a bunch of questions after reading some code and doing some initial
> work on this. So first in nsNodeSH::PreCreate I don't understand why do we
> need that voodoo with WrappNative instead of just QI and call
> GetGlobalJSObject once we got the scope object with doc->GetScopeObject?

It looks like this was done in bug 564585 comment 0, so that nsNodeSH::PreCreate could unconditionally call WrapNativeParent, which assumes that the parent has a wrapper cache. Peter says:

> because if the node that's being wrapped is a document node we use the
> document's scope object which doesn't have a wrapper cache

AFAICT, nsGlobalWindow at least is wrapper-cached. However, that doesn't apply to sandboxes and such.

Regardless though, I think the simplification you're proposing is probably fine.

> I think I'm also missing something in general from the approach of
> introducing this new simplified interface (let's call it now
> nsIGlobalJSObjectHolder).

How about just nsIGlobalObject?

> So for setters I can change function signatures
> probably and accept a pointer to this new interface as input, for storing it
> in the member variables (with or without mergeing them) I don't see a big
> problem since they are weak pointers, so they don't have to be changed. But
> for getters like GetScopeObject I cannot really change the return type that
> simply. A lot of consumer of these getters expect and needs a
> nsIScriptGlobalObject. (I can ofc QI to it at any point of code they are
> used, but I don't think that is the right thing to do)

The idea is that nsIScriptGlobalObject inherits nsIGlobalObject, so you don't have to change any function signatures except in the places where non-SGO stuff doesn't go but currently needs to go. In those places, you can just widen the type to nsIGlobalObject, and it shouldn't cause problems for calling code that expects an nsIScriptGlobalObject.

Does that help? If not, I might need more of a concrete example of the problem you're running into.
(Assignee)

Comment 7

5 years ago
In reply to Bobby Holley (:bholley) from comment #6)
> Does that help? If not, I might need more of a concrete example of the
> problem you're running into.

The first part very much, the second part not that much. I think my question wasn't clear.

So we have this object with nsIGlobalObject iface, and we pass it into XHR that wants to remember it to pass into the NS_NewDOMDocument later. For that I did what smaug suggested in Comment 2 pretty much. 

nsDocument
We have mScriptObject, mScopeObject and mScriptGlobalObject (strong ref on nsIDocument). At this point I kind of want to scream. Other than that I don't know what the 3rd guy does. I can kind of guess the difference between the other two by now. Let's store nsIGlobalObject in mScopeObject and hopefully we can get rid of mScriptObject later.

I probably have to change the return type of GetScopeObject to nsIGlobalObject* and check all its consumer if there is needed to be done some QI to upcast it back to nsIScriptGlobalObject*. And SetScriptHandlingObject since we use that to set mScriptObject.

But then there are things like the GetScriptHandlingObject call in nsDocument::CloneDocHelper. Seems like I have to change GetScriptHandlingObject's return type too... 

Now the question is what to do with GetScriptHandlingObject, GetScriptHandlingObjectInternal, GetScriptGlobalObject, GetScopeObject. At this point it seems like I will have to change all of their return type in the end (except the one that could go anyway), and that would mean a bunch of QI at random places all over the code where they are used and a nsIScriptGlobalObject is expected. Sounds bad.

I'm looking for a sane version but could not find it yet. Could someone explain me what is the role of mScriptGlobalObject?

One more thing, are there any other scope than XPCWrappedNativeScope in xpconnect that should implement this new interface?
(Assignee)

Comment 8

5 years ago
I got pretty far with this I have two problems left. One that I can fix but ugly and one that I don't know how to fix.

Bobby:
So since documents have to know their globals, nsXHR should know it as well in cases where it is able to create a document (non-worker). There is a catch: the nsXMLHttpRequest::Init() function. The principal is something we can easily get from sm. But what about the global? I was thinking that maybe we can just grab the global of the current context (I'm not sure it this right), but even we have the global JSObject how do we fetch the nsIGlobalObject from it? If it is a sandbox then it is relatively simple (except it adds more xpconnect magic to this file...), but for windows? Or are there any other case? How do I fetch some nsISupport in the chrome window case? GetNativeOfWrapper? Do you have any sane idea to solve this problem elegantly maybe?

Smaug, Kyle:
The other issue is that I did what you suggested in Comment 2, so that member is a nsIGlobalObject* and for GetOwner I simply do a QI. But here is what I did not expect, indexeddb calls GetOwner off the main thread! Which worked fine since it's a raw pointer, but if I do a QI now in the getter then it throws (from addRef)since the global window is not thread safe...
I think it's a horrible idea to use the pointer of the window off main thread, from some worker thread. But it can be done safely and I guess they know what they're doing, even though it is like a time bomb imo. Since anyone calls a non-constant function on that pointer later, will cause all kind of hard to find issues. Not to mention that I don't know what keeps the window alive while that point is being used...
Anyway, the only thing I can do for now, is keeping the original nsPIDOMWindow* and add another member the nsIGlobalObject* which is really whacky (so the original member becomes a cached QI result of the new member). So I would like to hear your opinion and I also CC Kyle. So Kyle, why do you need the window in OpenDatabaseHelper::DoDatabaseWork() exactly? Is that pointer only used for something like a key in a map?

Comment 9

5 years ago
Hmm, nsDOMEventTargetHelper needs also a pointer to nsPIDOMWindow so that it can do innerwindowcorrectness checks (without QIing). So just adding a new pointer to nsDOMEventTargetHelper
sounds ok. But need to be careful...nsDOMEventTargetHelper must not own its global, at least 
in case the global is nsPIDOMWindow. mOwner used to be nsCOMPtr but it was changed to be
raw pointer (which is explicitly cleared) to ensure that window objects are deleted earlier.
(Assignee)

Comment 10

5 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> Hmm, nsDOMEventTargetHelper needs also a pointer to nsPIDOMWindow so that it

Alright that is exactly what I'm doing right now. I will add some comments to explain it, what is going on. In that case: nevermind Kyle...
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> Bobby:
> So since documents have to know their globals, nsXHR should know it as well
> in cases where it is able to create a document (non-worker). There is a
> catch: the nsXMLHttpRequest::Init() function. The principal is something we
> can easily get from sm. But what about the global? I was thinking that maybe
> we can just grab the global of the current context (I'm not sure it this
> right), but even we have the global JSObject how do we fetch the
> nsIGlobalObject from it? If it is a sandbox then it is relatively simple
> (except it adds more xpconnect magic to this file...), but for windows? Or
> are there any other case? How do I fetch some nsISupport in the chrome
> window case? GetNativeOfWrapper? Do you have any sane idea to solve this
> problem elegantly maybe?

I thought the idea was that we'd guarantee that all XPConnect globals (objects where compartmentPrivate->scope is non-null) have an nsIGlobalObject in their private slot. We talked on IRC about solving this problem for sandboxes, and it should already be correct for Windows. We might also need to add it for the backstage pass, but that should be trivial. In that case, we can just assert in XPCWrappedNativeScope::SetGlobal that JS_GetPrivate(aGlobal) QIs to nsIGlobalObject.

Is that helpful?
(Assignee)

Comment 12

5 years ago
(In reply to Bobby Holley (:bholley) from comment #11)
> problem for sandboxes, and it should already be correct for Windows.

I hoped this is true but was not sure in it.

> Is that helpful?

Absolutely. Thanks!
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> (In reply to Bobby Holley (:bholley) from comment #11)
> > problem for sandboxes, and it should already be correct for Windows.
> 
> I hoped this is true but was not sure in it.

Well, Window currently implements nsIScriptGlobalObject. It's up to you to make that inherit your new nsIGlobalObject. :-)
(Assignee)

Comment 14

5 years ago
(In reply to Bobby Holley (:bholley) from comment #13)
> Well, Window currently implements nsIScriptGlobalObject. It's up to you to
> make that inherit your new nsIGlobalObject. :-)

Sure, that part is done, I just was not sure if it's set as its private like in the sandbox case, or there is some more subtle way to get to that pointer.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> (In reply to Bobby Holley (:bholley) from comment #13)
> > Well, Window currently implements nsIScriptGlobalObject. It's up to you to
> > make that inherit your new nsIGlobalObject. :-)
> 
> Sure, that part is done, I just was not sure if it's set as its private like
> in the sandbox case, or there is some more subtle way to get to that pointer.

Oh wait, you're right. The private in that case is an XPCWN with the nsIGlobalObject as its Native(). You'll have to handle that, but it should be very straightforward and can be factored into an API method for XPConnect.
(Assignee)

Comment 16

5 years ago
(In reply to Bobby Holley (:bholley) from comment #11)
> We
> might also need to add it for the backstage pass, but that should be
> trivial. 

So I don't know much about this backstage pass, and since it uses system principal I have no idea what should be the js global in this case, and where to get it from?
The backstage pass is basically akin to a PrincipalHolder, except it gets wrapped as an XPCWN. We use it for JSMs and XPCWrappedJS stuff. Just do the same thing you're doing for sandboxes: Have it implement nsIGlobalObject, and have it grab the global JSObject off of mScope. See the declaration in xpcprivate.h.
(Assignee)

Comment 18

5 years ago
(In reply to Bobby Holley (:bholley) from comment #17)
> and have it grab the global JSObject off of mScope. See the declaration in
> xpcprivate.h.

This is the part I'm having problem with. By mScope are you referring to the mScopes SET on the nsXPConnect that is not being used for anything in the code according to mxr, or the mScopes LIST on XPCContext? I can find no mScope in xpcprivate.h

So what I see here that this back stage pass is a singleton per xpconnect which is a singleton itself. But this backstage pass can be used from various system compartments I assume... So I can just ask the xpcruntime when the back stage pass is created to give me the current xpccallcontext and grab its mScopeForNewJSObjects and use that. But that mean every document created through backstage pass will end up in the first compartment that wanted to use back stage pass, which I'm not sure is the right thing to do. Although it will probably just work, and won't be worse that it is now. Am I overlooking something here?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18)
> (In reply to Bobby Holley (:bholley) from comment #17)
> > and have it grab the global JSObject off of mScope. See the declaration in
> > xpcprivate.h.
> 
> This is the part I'm having problem with. By mScope are you referring to the
> mScopes SET on the nsXPConnect that is not being used for anything in the
> code according to mxr, or the mScopes LIST on XPCContext? I can find no
> mScope in xpcprivate.h

Oh, whoops. Heh, my eyes glazed over from the definition of the backstage pass to the definition of nsXPCComponents (which has an mScope).

Yeah, so this is actually a problem. The C++ object for the backstage pass has a many-to-one relationship with JS objects, which breaks under this current model. So there's no way for the document to hold a C++ reference to the native global, since the native global is insufficient to determine the JS global object.

I think we probably need to make the backstage pass a non-singleton object. This should be pretty straightforward, and I don't see any reason why we can't. It just involves getting rid of the cache and having the backstage pass store a ref to its global JSObject (around line 747 of mozJSComponentLoader.cpp), similar to what happens with nsGlobalWindow.

Let's just check with mrbkap to make sure this is ok. Blake, is there anything that depends on the BackstagePass being a singleton?
Flags: needinfo?(mrbkap)
(Assignee)

Comment 20

5 years ago
I came to the same conclusion, one thing I worry about is the life time of these back stage passes object. Since so fat it has been kept alive by XPConnect (it has a strong ref to it) and that will change. Anyway, I give it a try and see what happens.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20)
> I came to the same conclusion, one thing I worry about is the life time of
> these back stage passes object. Since so fat it has been kept alive by
> XPConnect (it has a strong ref to it) and that will change. Anyway, I give
> it a try and see what happens.

I don't think it's a problem. The XPCWN will hold a strong ref to it, and it'll go away naturally: the global dies, it releases the native, and the native dies. I think we should add a finalizer hook to null out mJSObject just in case someone else holds a strong ref to a BackstagePass, though.
(Assignee)

Comment 22

5 years ago
I'm not sure how can I add that finalizer. I guess I have to add a nsIXPCScriptable::WANT_FINALIZE flag to the BackstagePass and then implement some method? Or can I just somehow set a finalizer on the JSObject after it has been created?

The more important problem I'm struggling with is getting the right global... So for example: http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug431701.html?force=1#54

here we have this SpecialPower thing, and in this case if I try to get the global from the current context (in nsXMLHttpRequest::Init()), I will get to an nsInProcessTabChildGlobal... I'm confused... what is that exactly? Or is it the right global in this case, just yet another type of global I have to deal with? Not sure how SpecialPower really works.
(Assignee)

Comment 23

5 years ago
Based on what smaug told me on IRC nsInProcessTabChildGlobal seems like just another type of global that has to implement nsIGlobalObject.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22)
> I'm not sure how can I add that finalizer. I guess I have to add a
> nsIXPCScriptable::WANT_FINALIZE flag to the BackstagePass and then implement
> some method?

Yes, this. Should be very straightforward.
(Assignee)

Comment 25

5 years ago
Smaug: do you know what should we do with TestNativeXMLHttpRequest.cpp ? It creates an XHR without any JS object or even context around and creates a Document... I assume the test should be changed in this case, right? (creating some JS compartment and passing in its global for XHR) Is this one an important test?
Ben added the test.
(In reply to Bobby Holley (:bholley) from comment #19)
> Let's just check with mrbkap to make sure this is ok. Blake, is there
> anything that depends on the BackstagePass being a singleton?

I don't know of anything offhand, no.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22)
> I'm not sure how can I add that finalizer. I guess I have to add a
> nsIXPCScriptable::WANT_FINALIZE flag to the BackstagePass and then implement
> some method? Or can I just somehow set a finalizer on the JSObject after it
> has been created?

You actually want to define XPC_MAP_WANT_FINALIZE near the WANT_NEWRESOLVE in XPCRuntimeService.cpp and then implement BackstagePass::Finalize. You can't set finalizers on an object once the class has been set (changing JSClasses after creation/use is a very dangerous proposition).
Flags: needinfo?(mrbkap)
(Assignee)

Comment 28

5 years ago
I still cannot say I'm done here, but I think it's better if we get started with some reviewing... The patches passed the tests except that TestNativeXMLHttpRequest I mentioned, but they are need to be updated, polished and documents still don't know their globals in every case. But I guess an f? at this point makes sense.
(Assignee)

Comment 29

5 years ago
Created attachment 708029 [details] [diff] [review]
part1: Merging members on nsDocument

Needed an extra boolean flag to merge these field. I need a better name, but I could not find any yet...
Attachment #708029 - Flags: feedback?(bugs)
(Assignee)

Comment 30

5 years ago
Created attachment 708030 [details] [diff] [review]
part2: nsIGlobalObject

I really need to fix these idls I know. I want to remove those C++{} parts, just had various compiler problems with calling conventions and JSObject* return type. I think I need some help here how can I make this compile without using any C++{}?

other than that this patch is about having all the globals implement nsIGlobalObject
Attachment #708030 - Flags: feedback?(bobbyholley+bmo)
(Assignee)

Comment 31

5 years ago
Created attachment 708032 [details] [diff] [review]
part3: preparing nsDocument for dealing with nsIGlobalObject

this is a quite straightforward patch, it's about Document using this new nsIGlobalInterface in some of its functions
Attachment #708032 - Flags: feedback?(bugs)
Comment on attachment 708030 [details] [diff] [review]
part2: nsIGlobalObject

Review of attachment 708030 [details] [diff] [review]:
-----------------------------------------------------------------

Why would you use XPIDL interfaces for any of these? In any case, virtual functions in {C++ blocks are fundamentally broken.
(Assignee)

Comment 33

5 years ago
Created attachment 708036 [details] [diff] [review]
part4: documents should know their global

this is the real patch... there are some dodgy things here. I think we discussed all the edgy cases on irc, or here, but let me know if something is not clear. 

Any suggestion that helps imporving this patch is highly welcome.

As I mentioned earlier, document does not know yet their global in every case, so I could not turn that branch in nsIDOMClassInfo into an assert yet. Tests are green and the problem with XHR is solved, but the patch is not yet finished. The problem is that in some cases it's hard to find a global...

One case is that cpp test I mentioned where there is no global or compartment or even JS context at all... But there are some others too. nsContentUtils::ConvertToPlainText

XBL Documents in nsXMLDocument

nsParserUtils::Sanitize

Any ideas what shall we do in these cases?

Finally: I know, I know I should find a way to break these patches into smaller ones, especially this last one...
Attachment #708036 - Flags: feedback?(bugs)
Attachment #708036 - Flags: feedback?(bobbyholley+bmo)
(Assignee)

Comment 34

5 years ago
(In reply to :Ms2ger from comment #32)
> Comment on attachment 708030 [details] [diff] [review]
> part2: nsIGlobalObject
> 
> Review of attachment 708030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why would you use XPIDL interfaces for any of these? In any case, virtual
> functions in {C++ blocks are fundamentally broken.

Last time I added an equally trivial interface, Bobby asked me to do so. So I kind of assumed that we always prefer IDL... on my part, I'm happy not to use IDL ever.
Comment on attachment 708030 [details] [diff] [review]
part2: nsIGlobalObject

Review of attachment 708030 [details] [diff] [review]:
-----------------------------------------------------------------

Please add an assertion in XPCWrappedNativeScope::SetGlobal that |native| QIs to nsIGlobalObject.

How feasible would it be to make nsIGlobalObject inherit nsISOP? It's not totally clear to me whether we have the invariant that all globals implement nsISOP, but if they do, that would make a lot of sense.

::: content/xbl/src/nsXBLDocumentInfo.cpp
@@ +49,5 @@
>  
>    // nsISupports interface
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +
> +  // nsIGlobalJSObjectHolder methods

Update the name here.

::: dom/base/nsIScriptGlobalObject.h
@@ +64,2 @@
>  { 0x92569431, 0x6e6e, 0x408a, \
>    { 0xa8, 0x8c, 0x45, 0x28, 0x5c, 0x1c, 0x85, 0x73 } }

Needs IID rev

::: js/xpconnect/idl/nsIGlobalObject.idl
@@ +14,5 @@
> +interface nsIGlobalObject : nsISupports
> +{
> +%{ C++
> +  virtual JSObject* GetGlobalJSObject() = 0;
> +%}

You can use [notxpcom] to get this behavior.

::: js/xpconnect/idl/nsIJSRuntimeService.idl
@@ +15,4 @@
>  interface nsIJSRuntimeService : nsISupports
>  {
>      readonly attribute JSRuntime        runtime;
> +    readonly attribute nsIBackstagePass backstagePass;

This no longer makes sense as an attribute, given that we return a new value every time. In fact, I think we should just kill the XPCOM goop and expose the BSP more directly, either letter mozJSComponent loader do "new BackstagePass()" or "NS_NewBackstagePass()", depending on how the includes work out.

This would also let us get rid of nsIBackstagePass, since we could just use the concrete class directly everywhere.

::: js/xpconnect/idl/nsISandboxPrivate.idl
@@ +6,5 @@
> +
> +#include "nsIGlobalObject.idl"
> +
> +[scriptable, uuid(b644b241-7f36-4227-92ed-ae1489ceefbc)]
> +interface nsISandboxPrivate: nsIGlobalObject

Why do we need this? It seems like we only need to call this in the sandbox finalizer hook, at which point we should know exactly what kind of private we have and be able to just static_cast, yeah?

::: js/xpconnect/src/XPCComponents.cpp
@@ +2844,5 @@
>  {
>      return mHoldee;
>  }
>  
> +NS_IMPL_ISUPPORTS_INHERITED2(SandboxPrivate, PrincipalHolder, nsISandboxPrivate, nsIGlobalObject)

I think this coupling here is more trouble than it's worth. Let's move PrincipalHolder into XPCJSContextStack, and make a separate class SandboxPrivate that implements both nsIGO and nsISOP.

@@ +3278,5 @@
> +    nsCOMPtr<nsIPrincipal> principal;
> +    if (sop) {
> +        principal = sop->GetPrincipal();
> +    } else {
> +        principal = do_QueryInterface(prinOrSop);

Let's reframe this so that sop is scoped very tightly.

nsCOMPtr<nsIPrincipal> principal = do_QueryInterface(principalOrSop);
if (!principal) {
  nsCOMPtr<nsISOP> sop = do_QueryInterface(principalOrSop);
  if (sop)
    principal = sop->GetPrincipal();
  else
    principal = do_CreateInstance("nullprincipal");
  MOZ_ASSERT(principal);
}

::: js/xpconnect/src/nsXPConnect.cpp
@@ +1994,5 @@
> +        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
> +    if (!secman)
> +        return NS_ERROR_NOT_AVAILABLE;
> +    if (NS_FAILED(secman->GetSystemPrincipal(getter_AddRefs(sysprin))))
> +        return NS_ERROR_NOT_AVAILABLE;

Can we hoist this into the BackstagePass constructor? We should never be creating a BackstagePass with anything but system principal.

::: js/xpconnect/src/xpcpublic.h
@@ +285,5 @@
>  bool
>  Throw(JSContext *cx, nsresult rv);
>  
> +nsIGlobalObject *
> +GetGlobalFromContext(JSContext *cx);

What's this all about?
Attachment #708030 - Flags: feedback?(bobbyholley+bmo) → feedback+
(Assignee)

Comment 36

5 years ago
(In reply to Bobby Holley (:bholley) from comment #35)
> Comment on attachment 708030 [details] [diff] [review]
> part2: nsIGlobalObject

I will think about all the interface related changes, and will get back to you about them after I'm done with the updating. Thanks a lot for the quick feedback!

> > +nsIGlobalObject *
> > +GetGlobalFromContext(JSContext *cx);
> 
> What's this all about?

This is for the case when XHR is created via the factory constructor. It's an attempt to figure out the right global from the current context. One of the shaky part of the patch...
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #36)

> > > +nsIGlobalObject *
> > > +GetGlobalFromContext(JSContext *cx);
> > 
> > What's this all about?
> 
> This is for the case when XHR is created via the factory constructor. It's
> an attempt to figure out the right global from the current context. One of
> the shaky part of the patch...

Yes, but I don't see the implementation in this patch.
(Assignee)

Comment 38

5 years ago
(In reply to Bobby Holley (:bholley) from comment #37)
> Yes, but I don't see the implementation in this patch.

Right, sorry about that. I'll just move it to the right patch where it belongs.
Comment on attachment 708036 [details] [diff] [review]
part4: documents should know their global

Review of attachment 708036 [details] [diff] [review]:
-----------------------------------------------------------------

Most of this stuff is not my area of expertise, and I'll let smaug review it. However I do have some comments about the GetGlobalForContext stuff. Let's move that into the other patch and let smaug be the sole reviewer for this one?

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +292,5 @@
>             : nullptr;
>  }
>  
> +nsIGlobalObject *
> +GetGlobalFromContext(JSContext *cx)

I'd prefer that this API take a JSObject* and have the caller do JS_GetGlobalForScopeChain() if necessary.

I think the naming could also be improved. What about GetNativeForGlobal?

@@ +310,5 @@
> +        nsCOMPtr<nsISupports> native;
> +        wn->GetNative(getter_AddRefs(native));
> +        MOZ_ASSERT(native);
> +        global = do_QueryInterface(native);
> +    }

This needs to be improved and made fully general.

First, we need to check if the global has the flags JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_ISSUPORTS. Actually on second thought, let's just assert that in XPCWrappedNativeScope::SetGlobal and fix any callers (in a separate patch).

After doing that, this function can do the following:

(1)
if (EnsureCompartmentPrivate(obj)->scope)
  return nullptr; // not an XPConnect global

(2) Assert the flags from above, then grab the private as |native|.

(3) QI the private to nsIXPConnectWrappedNative. If so, let |native = native->GetIdentity()|.

(4) QI native to nsIGlobalObject, and assert that it succeeds.
Attachment #708036 - Flags: feedback?(bobbyholley+bmo)
Comment on attachment 708029 [details] [diff] [review]
part1: Merging members on nsDocument


>     mAllowDNSPrefetch(true),
>     mIsBeingUsedAsImage(false),
>     mHasLinksToUpdate(false),
>     mDirectionality(eDir_LTR),
>-    mPartID(0)
>+    mPartID(0),
>+    mHasHadScriptHandlingObject(false)
this is actually not needed since nsDocument is initialized to 0, so everything is
0/false by default.
> nsDocument::GetScriptHandlingObjectInternal() const
> {
>   NS_ASSERTION(!mScriptGlobalObject,
>                "Do not call this when mScriptGlobalObject is set!");
>+  if (mHasHadDefaultView)
>+    return NULL;
if (mHasHadDefaultView) {
  return nullptr;
}
Attachment #708029 - Flags: feedback?(bugs) → feedback+
Comment on attachment 708032 [details] [diff] [review]
part3: preparing nsDocument for dealing with nsIGlobalObject

># HG changeset patch
># Parent 9f301f1f987f63de04e8a3cd4182a919bd9e8855
># User Gabor Krizsanits <gkrizsanits@mozilla.com>
>[mq]: using nsIGlobalObject in documents
>
>diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h
>--- a/content/base/public/nsIDocument.h
>+++ b/content/base/public/nsIDocument.h
>@@ -9,16 +9,17 @@
> #include "nsAutoPtr.h"                   // for member
> #include "nsCOMArray.h"                  // for member
> #include "nsCRT.h"                       // for NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW
> #include "nsCompatibility.h"             // for member
> #include "nsCOMPtr.h"                    // for member
> #include "nsGkAtoms.h"                   // for static class members
> #include "nsIDocumentEncoder.h"          // for member (in nsCOMPtr)
> #include "nsIDocumentObserver.h"         // for typedef (nsUpdateType)
>+#include "nsIGlobalObject.h"
I don't see any reason to include nsIGlobalObject.h
class nsIGlobalObject; should be enough.

You need to update IID of nsIDocument

> nsContentUtils::GetContextFromDocument(nsIDocument *aDocument)
> {
>-  nsIScriptGlobalObject *sgo = aDocument->GetScopeObject();
>+  nsCOMPtr<nsIScriptGlobalObject> sgo =  do_QueryInterface(aDocument->GetScopeObject());
Hmm, this makes the method quite a bit slower. Could you check whether the method is called in hot code paths.

> nsContentUtils::GetContextForEventHandlers(nsINode* aNode,
>                                            nsresult* aRv)
> {
>   *aRv = NS_OK;
>   bool hasHadScriptObject = true;
>-  nsIScriptGlobalObject* sgo =
>-    aNode->OwnerDoc()->GetScriptHandlingObject(hasHadScriptObject);
>+  nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(
>+    aNode->OwnerDoc()->GetScriptHandlingObject(hasHadScriptObject));
This is definitely no-no. addref/release/QI are just too slow in this super hot code path.
So, need to figure out how to not cause this slowdown. Add a flag to nsIDocument to tell
whether script handling object is script global object?
Attachment #708032 - Flags: feedback?(bugs) → feedback-
Comment on attachment 708036 [details] [diff] [review]
part4: documents should know their global


> nsXMLHttpRequest::Init()
> {
>   nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
>   nsCOMPtr<nsIPrincipal> subjectPrincipal;
>   if (secMan) {
>     secMan->GetSystemPrincipal(getter_AddRefs(subjectPrincipal));
>   }
>   NS_ENSURE_STATE(subjectPrincipal);
>-  Construct(subjectPrincipal, nullptr);
>+
>+  nsCOMPtr<nsIGlobalObject> global;
>+  JSContext* cx = nsContentUtils::GetCurrentJSContext();
>+  if (cx) {
>+    // if there is a JS context, let's use it's global as the owner
>+    global = xpc::GetGlobalFromContext(cx);
>+    nsCOMPtr<nsPIDOMWindow> ownerWindow(do_QueryInterface(global));
>+    if (ownerWindow) {
>+      // if the global is a window make sure the Construct gets the inner
>+      if (ownerWindow->IsOuterWindow()) {
>+        global = do_QueryInterface(
>+          ownerWindow->GetCurrentInnerWindow());
>+      }
>+    }
>+  }
>+  Construct(subjectPrincipal, global);
>   return NS_OK;
Not super happy bringing back XHR relying on cx stack. If binary addon creates XHR, the objects ends up being bound to somewhat random window.
(C++ should use the other Init but may not do that.)


>     bool hasHadScriptObject = true;
>-    nsCOMPtr<nsIScriptGlobalObject> scriptObject = do_QueryInterface(
>-      doc->GetScriptHandlingObject(hasHadScriptObject));
>+    nsCOMPtr<nsIGlobalObject> globalObject = 
>+      doc->GetScriptHandlingObject(hasHadScriptObject);
>+    NS_ENSURE_STATE(globalObject);
>+
>+    nsCOMPtr<nsIScriptGlobalObject> scriptObject = 
>+      do_QueryInterface(globalObject);
>     NS_ENSURE_STATE(scriptObject);
> 
>     nsIScriptContext *context = scriptObject->GetContext();
>     NS_ENSURE_TRUE(context, NS_OK);
> 
>-    nsCOMPtr<nsIXMLHttpRequest> req =
>-        do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIXMLHttpRequest> req = new nsXMLHttpRequest();
>+    NS_ENSURE_TRUE(req, NS_ERROR_OUT_OF_MEMORY);
> 
>     nsCOMPtr<nsPIDOMWindow> owner = do_QueryInterface(scriptObject);
>-    req->Init(docPrincipal, context, owner ? owner->GetOuterWindow() : nullptr,
>-              nullptr);
>+    if (owner) {
>+      // In case the global of the document is a window, we need to outer here
ScriptHandlingObject must never be outer window

>+      nsCOMPtr<nsPIDOMWindow> outer = owner->GetOuterWindow();
>+      NS_ENSURE_TRUE(outer, NS_OK);
>+      globalObject = do_QueryInterface(outer);
>+    }
>+    rv = req->Init(docPrincipal, context, globalObject, nullptr);
>+    NS_ENSURE_SUCCESS(rv, rv);
...but why these changes to XUL are needed at all? Shouldn't just simple change to nsIScriptGlobalObject -> nsIGlobalObject
be enough?

Could we create some safe chrome global, and use that as a parent of otherwise parentless XHRs and XBL documents?
Attachment #708036 - Flags: feedback?(bugs) → feedback+
(Assignee)

Comment 43

5 years ago
Created attachment 715043 [details] [diff] [review]
part0: merge scope members on nsDocument
Attachment #708029 - Attachment is obsolete: true
Attachment #708030 - Attachment is obsolete: true
Attachment #708032 - Attachment is obsolete: true
Attachment #708036 - Attachment is obsolete: true
Attachment #715043 - Flags: review?(bugs)
(Assignee)

Comment 44

5 years ago
Created attachment 715044 [details] [diff] [review]
part1: nsIGlobalObject

So I asked bz and he was on the opinion that c++ interfaces should be implemented in header, since it's now inherited from nsIScriptObjectPrincipal I didn't really have a choice anyway.
Attachment #715044 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 45

5 years ago
Created attachment 715045 [details] [diff] [review]
part2: SandboxPrivate

SandboxPrivate now is public, because I will use it from jsd too. We might want to rename it since it now replaces PrincipalHolder too.
Attachment #715045 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 46

5 years ago
Created attachment 715046 [details] [diff] [review]
part3: jsd

I couldn't find a cleaner way to do this in jsd. I'm not happy that I had to touch jsd at all.
Attachment #715046 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 47

5 years ago
Created attachment 715047 [details] [diff] [review]
part4: BackstagePass
Attachment #715047 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 48

5 years ago
Created attachment 715048 [details] [diff] [review]
part5: assert for xpc globals
Attachment #715048 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 49

5 years ago
Created attachment 715050 [details] [diff] [review]
part6: using nsIGlobalObject

I put Bobby for r? on thins patch because we talked more about the inheritance of nsIGlobalObject, but feel free to pass it to Smaug.
Attachment #715050 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 50

5 years ago
Created attachment 715051 [details] [diff] [review]
GetNativeForGlobal

I just put this function in a separated patch in the end.
Attachment #715051 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 51

5 years ago
Created attachment 715052 [details] [diff] [review]
part8: JunkDrawer

name as requested :)
Attachment #715052 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 52

5 years ago
Created attachment 715053 [details] [diff] [review]
part9: nsDOMEventTarget holds a global
Attachment #715053 - Flags: review?(bugs)
(Assignee)

Comment 53

5 years ago
Created attachment 715054 [details] [diff] [review]
part10: weakref support for globals

So it turned out that I need to inherit from a class to support weakref...
Attachment #715054 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 54

5 years ago
Created attachment 715056 [details] [diff] [review]
part11: documents should know their globals

So I owe you a follow up here that should merge the different scope setters on nsDocument.
Attachment #715056 - Flags: review?(bugs)
(Assignee)

Comment 55

5 years ago
And I'm still chasing some leaks I get at shut down:
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 544 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of XPCNativeScriptableInfo with size 8 bytes each (16 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of XPCNativeScriptableShared with size 232 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of XPCWrappedNative with size 52 bytes each (104 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of XPCWrappedNativeProto with size 32 bytes each (64 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsXPCComponents with size 64 bytes each (128 bytes total)
Comment on attachment 715044 [details] [diff] [review]
part1: nsIGlobalObject

diff --git a/dom/base/nsIGlobalObject.h b/dom/base/nsIGlobalObject.h

+class nsIGlobalObject : public nsIScriptObjectPrincipal

Ok. So we're saying that all native globals should be able to identify their
principals? Seems reasonable, especially in a CPG world, and since most of
the native globals were already doing this.

diff --git a/dom/base/nsIScriptGlobalObject.h b/dom/base/nsIScriptGlobalObject.h
--- a/dom/base/nsIScriptGlobalObject.h
+++ b/dom/base/nsIScriptGlobalObject.h

Please add a comment to this file explaining that nsISGO is a heavyweight
interface implemented only by DOM globals, and might go away at some point.

r=bholley
Attachment #715044 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 715045 [details] [diff] [review]
part2: SandboxPrivate

diff --git a/js/xpconnect/public/SandboxPrivate.h b/js/xpconnect/public/SandboxPrivate.h
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/public/SandboxPrivate.h

So, this is in public/ so that it can be shared with jsd in subsequent patches,
right? Please add a comment in the file indicating that the whole thing can be
moved to xpcprivate once jsd is gone (which should happen within the year, I'd
think).

+class SandboxPrivate : public nsIGlobalObject
+{
+public:
+    SandboxPrivate(nsIPrincipal *holdee, JSObject *global)

It's probably best to aPrefix these.

+private:
+    nsCOMPtr<nsIPrincipal> mHoldee;

Let's call this mPrincipal.

+    JSObject *mGlobalJSObject;

 
 static void
 sandbox_finalize(JSFreeOp *fop, JSObject *obj)
 {
     nsIScriptObjectPrincipal *sop =
-        (nsIScriptObjectPrincipal *)xpc_GetJSPrivate(obj);
+        static_cast<nsIScriptObjectPrincipal *>(xpc_GetJSPrivate(obj));
+    MOZ_ASSERT(sop);

Do we need the QI here? Can't we just static_cast from nsISupports?

+    static_cast<SandboxPrivate *>(sop)->ForgetGlobalObject();
     NS_IF_RELEASE(sop);
     DestroyProtoAndIfaceCache(obj);
 }
 
         // Pass on ownership of sop to |sandbox|.

The varianble is no longer called |sop|.

r=bholley
Attachment #715045 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 715046 [details] [diff] [review]
part3: jsd

I have two concerns with this patch:

1 - Why are we setting up the global in jsd_high but setting up the private in
jsd_xpc? It seems like there's a good chance we could create the global in jsd_high
but then not touch it in jsd_xpc, meaning that we never set the private, thus
violating the assertion in the finalize hook.

2 - Grabbing the principal off the calling context is extremely dubious. I think
we should grab the null principal in jsd_high, pass that to JS_NewGlobalObject,
and pass that to the SandboxPrivate.

r- until we get that sorted out.
Attachment #715046 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 715043 [details] [diff] [review]
part0: merge scope members on nsDocument

Review of attachment 715043 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsIDocument.h
@@ +2169,5 @@
>    // If true, we have an input encoding.  If this is false, then the
>    // document was created entirely in memory
>    bool mHaveInputEncoding;
>  
> +  bool mHasHadDefaultView;

Documentation...
Comment on attachment 715044 [details] [diff] [review]
part1: nsIGlobalObject

Review of attachment 715044 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsInProcessTabChildGlobal.h
@@ +116,5 @@
>    void DelayedDisconnect();
> +
> +  virtual JSObject* GetGlobalJSObject() {
> +    if (!mGlobal)
> +      return NULL;

{}, nullptr
Comment on attachment 715045 [details] [diff] [review]
part2: SandboxPrivate

Review of attachment 715045 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCComponents.cpp
@@ +3324,5 @@
>  
> +        nsCOMPtr<nsIScriptObjectPrincipal> sbp =
> +            new SandboxPrivate(principal, sandbox);
> +        if (!sbp)
> +            return NS_ERROR_OUT_OF_MEMORY;

'new' can't fail

::: js/xpconnect/src/XPCJSContextStack.cpp
@@ +124,5 @@
>  static void
>  SafeFinalize(JSFreeOp *fop, JSObject* obj)
>  {
> +    SandboxPrivate* sop =
> +        static_cast<SandboxPrivate*>(xpc_GetJSPrivate(obj));

I tend to be a fan of helpers like

SandboxPrivate*
GetSafeContextPrivate(JSObject *obj)
{
  MOZ_ASSERT(GetObjectJSClass(obj) == global_class);
  return static_cast<SandboxPrivate*>(js::GetObjectPrivate(obj));
}

to make sure your cast is correct. I'm probably the only one, though.
Comment on attachment 715047 [details] [diff] [review]
part4: BackstagePass

Review of attachment 715047 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/Makefile.in
@@ +20,5 @@
>  		xpcObjectHelper.h \
>  		xpcpublic.h \
>  		XPCJSMemoryReporter.h \
> +		GeneratedEvents.h \
> +		BackstagePass.h

Add

... \
$(NULL)

::: js/xpconnect/src/XPCRuntimeService.cpp
@@ +167,5 @@
> +    return NS_OK;
> +}
> +
> +nsresult
> +NS_NewBackstagePass(BackstagePass** ret) {

{ on the next line

@@ +176,5 @@
> +      return NS_ERROR_NOT_AVAILABLE;
> +  if (NS_FAILED(secman->GetSystemPrincipal(getter_AddRefs(sysprin))))
> +      return NS_ERROR_NOT_AVAILABLE;
> +  nsRefPtr<BackstagePass> bsp = new BackstagePass(sysprin);
> +  NS_ENSURE_TRUE(bsp, NS_ERROR_OUT_OF_MEMORY);

No need

@@ +177,5 @@
> +  if (NS_FAILED(secman->GetSystemPrincipal(getter_AddRefs(sysprin))))
> +      return NS_ERROR_NOT_AVAILABLE;
> +  nsRefPtr<BackstagePass> bsp = new BackstagePass(sysprin);
> +  NS_ENSURE_TRUE(bsp, NS_ERROR_OUT_OF_MEMORY);
> +  *ret = bsp.forget().get();

bsp.forget(ret);
(Assignee)

Comment 63

5 years ago
(In reply to Bobby Holley (:bholley) from comment #58)
> 1 - Why are we setting up the global in jsd_high but setting up the private
> in
> jsd_xpc? It seems like there's a good chance we could create the global in
> jsd_high
> but then not touch it in jsd_xpc, meaning that we never set the private, thus
> violating the assertion in the finalize hook.

So this is an ancient code. jsd_high is written in pure c just the file was renamed to cpp pretty much. It operates with malloc and all, so using |new| in it just feels wrong. The other thing is that all the xpconnect related magic should happen in jsd_xpc it seems, and I don't want to break that... so I try to leave jsd_high alone as much as possible. Unfortunately the global_class is defined there (could be moved maybe...), and even worse: the global is created there. So I get your concern but I'm still not sure that we could do any xpconnect magic in that file. But I could export another utility function for it call it in jsd_high and implement it in jsd_xpc, would that work for you?
Comment on attachment 715051 [details] [diff] [review]
GetNativeForGlobal

Review of attachment 715051 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +661,5 @@
> +GetNativeForGlobal(JSObject *obj)
> +{
> +    MOZ_ASSERT(obj);
> +    if (!EnsureCompartmentPrivate(obj)->scope)
> +        return nullptr;

Why not

CompartmentPrivate* priv = GetCompartmentPrivate(obj);
if (!priv || !priv->scope)
    return nullptr;

?
(Assignee)

Comment 65

5 years ago
(In reply to :Ms2ger from comment #61)
> 'new' can't fail

Should be the true... unfortunately it is not on every compiler :( can remove it though if you wish.
Comment on attachment 715052 [details] [diff] [review]
part8: JunkDrawer

Review of attachment 715052 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2874,5 @@
> +XPCJSRuntime::GetJunkDrawer()
> +{
> +    JSObject *sandboxObj;
> +    if (!mJunkDrawer)
> +    {

Leave the { on the previous line

::: js/xpconnect/src/xpcpublic.h
@@ +390,5 @@
>  bool
>  Throw(JSContext *cx, nsresult rv);
>  
>  nsIGlobalObject *
> +GetNativeForGlobal(JSObject *obj);

What happened here?
Comment on attachment 715053 [details] [diff] [review]
part9: nsDOMEventTarget holds a global

Review of attachment 715053 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.cpp
@@ +451,5 @@
> +
> +  nsCOMPtr<nsIGlobalObject> global;
> +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> +  if (cx) {
> +    // if there is a JS context, let's use it's global as the owner

its

@@ +452,5 @@
> +  nsCOMPtr<nsIGlobalObject> global;
> +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> +  if (cx) {
> +    // if there is a JS context, let's use it's global as the owner
> +    JSObject *obj = JS_GetGlobalForScopeChain(cx);

* to the left

@@ +473,2 @@
>    NS_ENSURE_ARG_POINTER(aPrincipal);
> +  

Trailing whitespace

::: content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp
@@ +174,5 @@
>      nsCOMPtr<nsIXMLHttpRequest> req =
>          do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = req->Init(docPrincipal, context, 

Trailing whitespace
Comment on attachment 715056 [details] [diff] [review]
part11: documents should know their globals

Review of attachment 715056 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +4549,5 @@
>      if (!scriptObject && hasHadScriptObject) {
>        rv.Throw(NS_ERROR_UNEXPECTED);
>        return nullptr;
>      }
> +    mDOMImplementation = new DOMImplementation(this, 

Trailing whitespace

::: content/xml/document/src/nsXMLDocument.cpp
@@ +124,5 @@
>    }
>  
> +  if (nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aEventObject)) {
> +    d->SetScriptHandlingObject(sgo);
> +  } else if (aEventObject){

Space before {

::: dom/base/nsDOMClassInfo.cpp
@@ +6813,5 @@
>      // We're called for a document object; set the parent to be the
>      // document's global object, if there is one
>  
>      // Get the scope object from the document.
> +    nsIGlobalObject *scope = doc->GetScopeObject();

* to the left

::: js/xpconnect/tests/unit/test_allowedDomainsXHR.js
@@ +77,5 @@
> +  function changeListener(event) {
> +    if (checkResults(async))
> +      finish();
> +  }
> +  

More trailing whitespace
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #65)
> > 'new' can't fail
> 
> Should be the true... unfortunately it is not on every compiler :( can
> remove it though if you wish.

I've been treating |new| as infallible since infallible malloc landed, and was under the impression that this was the encouraged practice. If you have reason to believe that it isn't, let me know and I'll change my habits. Otherwise, let's remove the null check. :-)
(Assignee)

Comment 70

5 years ago
(In reply to Bobby Holley (:bholley) from comment #69)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #65)
> > > 'new' can't fail
> > 
> > Should be the true... unfortunately it is not on every compiler :( can
> > remove it though if you wish.
> 
> I've been treating |new| as infallible since infallible malloc landed, and
> was under the impression that this was the encouraged practice. If you have
> reason to believe that it isn't, let me know and I'll change my habits.
> Otherwise, let's remove the null check. :-)

Naah, I jsut read too much old windows code, on msvc6 it can return null on some cases, but I doubt we should care.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #63)
> So this is an ancient code. jsd_high is written in pure c just the file was
> renamed to cpp pretty much. It operates with malloc and all, so using |new|
> in it just feels wrong. The other thing is that all the xpconnect related
> magic should happen in jsd_xpc it seems, and I don't want to break that...
> so I try to leave jsd_high alone as much as possible. Unfortunately the
> global_class is defined there (could be moved maybe...), and even worse: the
> global is created there. So I get your concern but I'm still not sure that
> we could do any xpconnect magic in that file. But I could export another
> utility function for it call it in jsd_high and implement it in jsd_xpc,
> would that work for you?

Jorendorff says: "in the service of adding invariants gabor can do *anything* to JSD as far as i'm concerned".

So just put it all in jsd_high. The file is going away soon anyway. :-)
Comment on attachment 715047 [details] [diff] [review]
part4: BackstagePass

 
     nsCOMPtr<nsIXPConnectJSObjectHolder> holder;
-    rv = xpc->InitClassesWithNewWrappedGlobal(cx, backstagePass,
+    rv = xpc->InitClassesWithNewWrappedGlobal(cx,
+                                              static_cast<nsIScriptObjectPrincipal*>(backstagePass),

Nit: I think it would be more obvious to cast to nsIGlobalObject. This occurs
in a few places.

     [noscript, notxpcom] void registerGCCallback(in JSGCCallback func);
     [noscript, notxpcom] void unregisterGCCallback(in JSGCCallback func);
+    [noscript] BackstagePassPtr createBackstagePass();

Why do we need this? Is the linkage of xpcshell such that we can't directly
invoke NS_NewBackstagePass?


 };
diff --git a/js/xpconnect/loader/mozJSComponentLoader.cpp b/js/xpconnect/loader/mozJSComponentLoader.cpp
--- a/js/xpconnect/loader/mozJSComponentLoader.cpp
+++ b/js/xpconnect/loader/mozJSComponentLoader.cpp
@@ -732,31 +732,35 @@ mozJSComponentLoader::PrepareObjectForLo
         holder = mLoaderGlobal;
     }
 
     nsresult rv = NS_OK;
     nsCOMPtr<nsIXPConnect> xpc =
         do_GetService(kXPConnectServiceContractID, &rv);
     NS_ENSURE_SUCCESS(rv, nullptr);
 
+    nsRefPtr<BackstagePass> backstagePass;
+
     if (!mLoaderGlobal) {
-        nsCOMPtr<nsIXPCScriptable> backstagePass;
-        rv = mRuntimeService->GetBackstagePass(getter_AddRefs(backstagePass));
+        rv = NS_NewBackstagePass(getter_AddRefs(backstagePass));
         NS_ENSURE_SUCCESS(rv, nullptr);
 
-        rv = xpc->InitClassesWithNewWrappedGlobal(aCx, backstagePass,
+        rv = xpc->InitClassesWithNewWrappedGlobal(aCx,
+                                                  static_cast<nsIScriptObjectPrincipal*>(backstagePass),
                                                   mSystemPrincipal,
                                                   0,
                                                   getter_AddRefs(holder));
         NS_ENSURE_SUCCESS(rv, nullptr);
 
         JSObject *global;
         rv = holder->GetJSObject(&global);
         NS_ENSURE_SUCCESS(rv, nullptr);
 
+        backstagePass->SetGlobalObject(global);
+
         JSAutoCompartment ac(aCx, global);
         if (!JS_DefineFunctions(aCx, global, gGlobalFun) ||
             !JS_DefineProfilingFunctions(aCx, global)) {
             return nullptr;
         }
 
         if (aReuseLoaderGlobal) {
             mLoaderGlobal = holder;
@@ -774,16 +778,19 @@ mozJSComponentLoader::PrepareObjectForLo
         // global, but rather we use a different object as the 'this' object.
         JSObject* newObj = JS_NewObject(aCx, &kFakeBackstagePassJSClass,
                                         nullptr, nullptr);
         NS_ENSURE_TRUE(newObj, nullptr);
 
         obj = newObj;
     }
 
+    if (backstagePass)
+        backstagePass->SetGlobalObject(obj);

Eek, no. On b2g, |obj| here is an entirely different object from the global.
We create one for each JSM, but only one global object (and thus one BSP)
for the entire system. Unless I'm missing something, this would cause us to
update the same BSP to point to a different scope object each time a JSM is
loaded, which definitely seems wrong. Remove this and re-scope the RefPtr?

diff --git a/js/xpconnect/src/XPCRuntimeService.cpp b/js/xpconnect/src/XPCRuntimeService.cpp
--- a/js/xpconnect/src/XPCRuntimeService.cpp
+++ b/js/xpconnect/src/XPCRuntimeService.cpp

+#define                             XPC_MAP_WANT_FINALIZE
 #define XPC_MAP_FLAGS       nsIXPCScriptable::USE_JSSTUB_FOR_ADDPROPERTY   |  \
                             nsIXPCScriptable::USE_JSSTUB_FOR_DELPROPERTY   |  \
                             nsIXPCScriptable::USE_JSSTUB_FOR_SETPROPERTY   |  \
                             nsIXPCScriptable::DONT_ENUM_STATIC_PROPS       |  \
                             nsIXPCScriptable::DONT_ENUM_QUERY_INTERFACE    |  \
                             nsIXPCScriptable::IS_GLOBAL_OBJECT             |  \
-                            nsIXPCScriptable::DONT_REFLECT_INTERFACE_NAMES
+                            nsIXPCScriptable::DONT_REFLECT_INTERFACE_NAMES |  \
+                            nsIXPCScriptable::WANT_FINALIZE

Why do you need this? The XPC_MAP_WANT_FINALIZE should add it to the flags, no?

+nsresult
+NS_NewBackstagePass(BackstagePass** ret) {
+  nsCOMPtr<nsIPrincipal> sysprin;
+  nsCOMPtr<nsIScriptSecurityManager> secman =
+      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
+  if (!secman)
+      return NS_ERROR_NOT_AVAILABLE;
+  if (NS_FAILED(secman->GetSystemPrincipal(getter_AddRefs(sysprin))))
+      return NS_ERROR_NOT_AVAILABLE;

It's kind of ridiculous that it's this much work to get the system principal.
Can you add some machinery so that we can get a non-addrefed reference to the
system principal by doing nsContentUtils::GetSystemPrincipal? Followup bug is fine.

+  nsRefPtr<BackstagePass> bsp = new BackstagePass(sysprin);
+  NS_ENSURE_TRUE(bsp, NS_ERROR_OUT_OF_MEMORY);
No need to check this - |new| uses infallible malloc.

r- on account of the issue in mozJSComponentLoader.
Attachment #715047 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 715048 [details] [diff] [review]
part5: assert for xpc globals

 XPCWrappedNativeScope::SetGlobal(JSContext *cx, JSObject* aGlobal)
 {
+    MOZ_ASSERT(aGlobal);
+    MOZ_ASSERT(js::GetObjectJSClass(aGlobal));
I don't think you need to assert that the object has a class.

+    MOZ_ASSERT(js::GetObjectJSClass(aGlobal)->flags & (JSCLASS_PRIVATE_IS_NSISUPPORTS |
+                                                       JSCLASS_HAS_PRIVATE));

Shouldn't regular old js::GetObjectClass work?

It seems to me that this is missing the final (and most important) assertion,
which is that the private QIs to nsIGlobalObject. Can we add that?
Attachment #715048 - Flags: review?(bobbyholley+bmo)
Comment on attachment 715050 [details] [diff] [review]
part6: using nsIGlobalObject

I think smaug should review this rather than me. However, I do have one comment.

+void
+nsDocument::SetScopeObject(nsIGlobalObject* aGlobal)
+{
+  mScopeObject = do_GetWeakReference(aGlobal);
+}

So, this patch isn't correct on its own, because it depends on part 10. These
should be re-ordered so that the weak reference support comes first - it's
important that each changeset leave the tree in a green state, because it allows
you to bisect for regressions later on down the line.

So please reorder the patches, and add an assertion that the object QIs to
nsISupportsWeakReference in the assertions patch.
Attachment #715050 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 75

5 years ago
(In reply to Bobby Holley (:bholley) from comment #73)
> It seems to me that this is missing the final (and most important) assertion,
> which is that the private QIs to nsIGlobalObject. Can we add that?

No. Unfortunately this code is called before we could have set the private on the global in most case :( I think we talked about this.
(Assignee)

Comment 76

5 years ago
(In reply to Bobby Holley (:bholley) from comment #74)
> So please reorder the patches, and add an assertion that the object QIs to
> nsISupportsWeakReference in the assertions patch.

Whoops, thanks for spotting it! I thought I already reordered it correctly but it seems like I have not...
Comment on attachment 715051 [details] [diff] [review]
GetNativeForGlobal

Review of attachment 715051 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with comments addressed.

::: js/xpconnect/src/xpcpublic.h
@@ +390,5 @@
>  bool
>  Throw(JSContext *cx, nsresult rv);
>  
> +nsIGlobalObject *
> +GetGlobalFromContext(JSContext *cx);

This is wrong.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +657,5 @@
>      return newSameCompartmentWrapper;
>  }
>  
> +nsIGlobalObject *
> +GetNativeForGlobal(JSObject *obj)

This function could use some commenting.

@@ +659,5 @@
>  
> +nsIGlobalObject *
> +GetNativeForGlobal(JSObject *obj)
> +{
> +    MOZ_ASSERT(obj);

Please just assert JS_IsGlobalObject here - the null check assertion will happen implicitly.

@@ +661,5 @@
> +GetNativeForGlobal(JSObject *obj)
> +{
> +    MOZ_ASSERT(obj);
> +    if (!EnsureCompartmentPrivate(obj)->scope)
> +        return nullptr;

I understand where Ms2ger is coming from in his suggestion, in that the current world guarantees that XPConnect scopes already have a compartment private. But I think it's easier for outsiders to comprehend if we use EnsureCompartmentPrivate here.

@@ +665,5 @@
> +        return nullptr;
> +
> +    MOZ_ASSERT(GetObjectJSClass(obj));
> +    MOZ_ASSERT(GetObjectJSClass(obj)->flags & (JSCLASS_PRIVATE_IS_NSISUPPORTS |
> +                                               JSCLASS_HAS_PRIVATE));

As in the other patch:
* no need to assert that the object has a JSClass
*use GetObjectClass.
Attachment #715051 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 715048 [details] [diff] [review]
part5: assert for xpc globals

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #75)
> (In reply to Bobby Holley (:bholley) from comment #73)
> > It seems to me that this is missing the final (and most important) assertion,
> > which is that the private QIs to nsIGlobalObject. Can we add that?
> 
> No. Unfortunately this code is called before we could have set the private
> on the global in most case :( I think we talked about this.

Ah, right. Sorry. r=bholley with the other comments addressed.
Attachment #715048 - Flags: review+
(Assignee)

Comment 79

5 years ago
(In reply to Bobby Holley (:bholley) from comment #72)
> Comment on attachment 715047 [details] [diff] [review]
> part4: BackstagePass

> Why do we need this? Is the linkage of xpcshell such that we can't directly
> invoke NS_NewBackstagePass?

Yeah exactly, I think it was for xpcshell...

> Eek, no. On b2g, |obj| here is an entirely different object from the global.
> We create one for each JSM, but only one global object (and thus one BSP)
> for the entire system. Unless I'm missing something, this would cause us to
> update the same BSP to point to a different scope object each time a JSM is
> loaded, which definitely seems wrong. Remove this and re-scope the RefPtr?

Hmmm... Ok I think I really misunderstood this code I think. This might be the root of a problem I'm chasing...

> 
> Why do you need this? The XPC_MAP_WANT_FINALIZE should add it to the flags,
> no?

Uhh... good question I will look into it, but probably you are right.

> It's kind of ridiculous that it's this much work to get the system principal.
> Can you add some machinery so that we can get a non-addrefed reference to the
> system principal by doing nsContentUtils::GetSystemPrincipal? Followup bug
> is fine.

+1, and same for null principal... it's used less often but still
Comment on attachment 715052 [details] [diff] [review]
part8: JunkDrawer

Review of attachment 715052 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC, Let's make GetJunkDrawer look like this:

if (mJunkDrawer)
  return mJunkDrawer;
JS::Value v;
JSContext *cx = mJSContextStack->GetSafeJSContext();
SandboxOptions options;
nsresult rv = xpc_CreateSandboxObject(cx, &v, nsContentUtils::GetSystemPrincipal(), options);
NS_ENSURE_SUCCESS(rv);
mJunkDrawer = &v.toObject();
JS_AddNamedObjectRoot(cx, &mJunkDrawer, "XPConnect Junk Drawer");


Also, this needs very thorough documentation to explain why the name makes perfect sense. :-)
Attachment #715052 - Flags: review?(bobbyholley+bmo) → review-
(In reply to Bobby Holley (:bholley) from comment #78)
> > No. Unfortunately this code is called before we could have set the private
> > on the global in most case :( I think we talked about this.
> 
> Ah, right. Sorry. r=bholley with the other comments addressed.

Oh, can we at least assert this in XPCWrappedNative::WrapNewGlobal?
Comment on attachment 715054 [details] [diff] [review]
part10: weakref support for globals

Review of attachment 715054 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley

::: js/xpconnect/public/SandboxPrivate.h
@@ +11,3 @@
>  
> +class SandboxPrivate : public nsIGlobalObject,
> +                       public nsSupportsWeakReference

I guess with this change the sandbox JSClass hooks will need to QI before casting to SandboxPrivate* :-(
Attachment #715054 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #79)
> (In reply to Bobby Holley (:bholley) from comment #72)
> > Comment on attachment 715047 [details] [diff] [review]
> > part4: BackstagePass
> 
> > Why do we need this? Is the linkage of xpcshell such that we can't directly
> > invoke NS_NewBackstagePass?
> 
> Yeah exactly, I think it was for xpcshell...

Hm, can you explain the problem in more detail here? If we're returning a BackstagePass* either way, why does it help to put it on nsIXPConnect?
Comment on attachment 715053 [details] [diff] [review]
part9: nsDOMEventTarget holds a global

># HG changeset patch
># Parent dee293b8fda3358d66721bd98ae48fcf10686ea7
># User Gabor Krizsanits <gkrizsanits@mozilla.com>
>nsDOMEventTarget holds a global
>
>diff --git a/content/base/public/nsIXMLHttpRequest.idl b/content/base/public/nsIXMLHttpRequest.idl
>--- a/content/base/public/nsIXMLHttpRequest.idl
>+++ b/content/base/public/nsIXMLHttpRequest.idl
>@@ -7,17 +7,17 @@
> 
> interface nsIChannel;
> interface nsIDOMDocument;
> interface nsIDOMEventListener;
> interface nsIPrincipal;
> interface nsIScriptContext;
> interface nsIURI;
> interface nsIVariant;
>-interface nsPIDOMWindow;
>+interface nsIGlobalObject;
> interface nsIInputStream;
> interface nsIDOMBlob;
> 
> %{C++
> // for jsval
> #include "jsapi.h"
> %}
> 
>@@ -95,17 +95,17 @@ interface nsIXMLHttpRequestUpload : nsIX
>  *   The 'onload', 'onerror', and 'onreadystatechange' attributes moved to
>  *   nsIJSXMLHttpRequest, but if you're coding in C++ you should avoid using
>  *   those.
>  *
>  * Conclusion: Do not use event listeners on XMLHttpRequest from C++, unless
>  * you're aware of all the security implications.  And then think twice about
>  * it.
>  */
>-[scriptable, uuid(8e9768b4-339c-413c-a210-0c74934eb9e1)]
>+[scriptable, uuid(977f1406-416a-40ac-ab89-ccd7ca0622ea)]
> interface nsIXMLHttpRequest : nsISupports
> {
>   /**
>    * The request uses a channel in order to perform the
>    * request.  This attribute represents the channel used
>    * for the request.  NULL if the channel has not yet been
>    * created.
>    *
>@@ -341,23 +341,26 @@ interface nsIXMLHttpRequest : nsISupport
>   /**
>    * Initialize the object for use from C++ code with the principal, script
>    * context, and owner window that should be used.
>    *
>    * @param principal The principal to use for the request. This must not be
>    *                  null.
>    * @param scriptContext The script context to use for the request. May be
>    *                      null.
>-   * @param ownerWindow The associated window for the request. May be null.
>+   * @param globalObject The associated global for the request. Can be the
>+   *                     outer window, a sandbox, or a backstage pass.
>+   *                     May be null, but then the request cannot create a
>+   *                     document.
>    * @param baseURI The base URI to use when resolving relative URIs. May be
>    *                null.
>    */
>   [noscript] void init(in nsIPrincipal principal,
>                        in nsIScriptContext scriptContext,
>-                       in nsPIDOMWindow ownerWindow,
>+                       in nsIGlobalObject globalObject,
>                        in nsIURI baseURI);
> 
>   /**
>    * Upload process can be tracked by adding event listener to |upload|.
>    */
>   readonly attribute nsIXMLHttpRequestUpload upload;
> 
>   /**
>diff --git a/content/base/src/nsXMLHttpRequest.cpp b/content/base/src/nsXMLHttpRequest.cpp
>--- a/content/base/src/nsXMLHttpRequest.cpp
>+++ b/content/base/src/nsXMLHttpRequest.cpp
>@@ -443,34 +443,51 @@ nsresult
> nsXMLHttpRequest::Init()
> {
>   nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
>   nsCOMPtr<nsIPrincipal> subjectPrincipal;
>   if (secMan) {
>     secMan->GetSystemPrincipal(getter_AddRefs(subjectPrincipal));
>   }
>   NS_ENSURE_STATE(subjectPrincipal);
>-  Construct(subjectPrincipal, nullptr);
>+
>+  nsCOMPtr<nsIGlobalObject> global;
>+  JSContext* cx = nsContentUtils::GetCurrentJSContext();
>+  if (cx) {
>+    // if there is a JS context, let's use it's global as the owner
>+    JSObject *obj = JS_GetGlobalForScopeChain(cx);
>+    MOZ_ASSERT(obj);
>+    global = xpc::GetNativeForGlobal(obj);
>+  }
I don't think we want random global from cx stack.
Please use the junk global here.


> nsDOMEventTargetHelper::BindToOwner(nsPIDOMWindow* aOwner)
> {
>-  if (mOwner) {
>-    static_cast<nsGlobalWindow*>(mOwner)->RemoveEventTargetObject(this);
>-    mOwner = nullptr;
>+  //TODO: remove this
Why?




>+nsDOMEventTargetHelper::BindToOwner(nsIGlobalObject* aOwner)
>+{
>+  if (mParentObject) {
>+    static_cast<nsGlobalWindow*>(mParentObject)->RemoveEventTargetObject(this);
Why is mParentObject nsGlobalWindow here?


>+    JSObject *global = JS_GetGlobalForScopeChain(cx);
>+    MOZ_ASSERT(global);
>+
>+    nsIScriptObjectPrincipal *sop =
>+        static_cast<nsIScriptObjectPrincipal *>(xpc_GetJSPrivate(global));
type* name
Also elsewhere


>+    nsCOMPtr<nsIGlobalObject> iglobal = do_QueryInterface(sop);
>+
>     nsCOMPtr<nsIXMLHttpRequest> xhr = new nsXMLHttpRequest();
>-    nsresult rv = xhr->Init(subjectPrincipal, NULL, NULL, NULL);
>+    nsresult rv = xhr->Init(subjectPrincipal, NULL, iglobal, NULL);
nullptr
Attachment #715053 - Flags: review?(bugs) → review-
Comment on attachment 715056 [details] [diff] [review]
part11: documents should know their globals

bholley or peterv should look at nsNodeSH::PreCreate
Attachment #715056 - Flags: review?(bugs) → review+
Comment on attachment 715043 [details] [diff] [review]
part0: merge scope members on nsDocument


>   if (aScriptObject) {
>     mHasHadScriptHandlingObject = true;
>+    mHasHadDefaultView = false;
Why do we need this if mScriptGlobalObject == aScriptObject?
Attachment #715043 - Flags: review?(bugs) → review+
(Assignee)

Comment 87

5 years ago
(In reply to Bobby Holley (:bholley) from comment #83)
> > Yeah exactly, I think it was for xpcshell...
> 
> Hm, can you explain the problem in more detail here? If we're returning a
> BackstagePass* either way, why does it help to put it on nsIXPConnect?

I would get a linker error (undefines symbol). Xpcshell only uses the header file of BackstagePass this way. And I cannot just put NS_NewBackstagePass into the header file since that would give me linker errors because of missing member functions definitions at the 'new BackstagePass' line.
(Assignee)

Comment 88

5 years ago
(In reply to Bobby Holley (:bholley) from comment #82)
> Comment on attachment 715054 [details] [diff] [review]
> part10: weakref support for globals
> > +class SandboxPrivate : public nsIGlobalObject,
> > +                       public nsSupportsWeakReference
> 
> I guess with this change the sandbox JSClass hooks will need to QI before
> casting to SandboxPrivate* :-(

I think we can still static cast if the pointer we start from is from the nsIGlobalObject part. If we had a pointer to the nsSupportsWeakReference* only then would static cast us in trouble. That being said I prefer QI... It's a bit slower but for example someone swapped the two super-classes and the code would break.
(Assignee)

Comment 89

5 years ago
(In reply to Olli Pettay [:smaug] from comment #84)
> Comment on attachment 715053 [details] [diff] [review]
> part9: nsDOMEventTarget holds a global
> >+  if (cx) {
> >+    // if there is a JS context, let's use it's global as the owner
> >+    JSObject *obj = JS_GetGlobalForScopeChain(cx);
> >+    MOZ_ASSERT(obj);
> >+    global = xpc::GetNativeForGlobal(obj);
> >+  }
> I don't think we want random global from cx stack.
> Please use the junk global here.

We don't want it but in case we keep the factory bases xhr creation API I don't think we can avoid. Well... we could use the JunkDrawer if you prefer... but I'm not sure it's a good idea to stress that compartment that much. But I can be convinced, it's really not clear to me what would be the best long term solution here.

> 
> 
> > nsDOMEventTargetHelper::BindToOwner(nsPIDOMWindow* aOwner)
> > {
> >-  if (mOwner) {
> >-    static_cast<nsGlobalWindow*>(mOwner)->RemoveEventTargetObject(this);
> >-    mOwner = nullptr;
> >+  //TODO: remove this
> Why?

I should have removed this comment long time ago...

> 
> 
> >+nsDOMEventTargetHelper::BindToOwner(nsIGlobalObject* aOwner)
> >+{
> >+  if (mParentObject) {
> >+    static_cast<nsGlobalWindow*>(mParentObject)->RemoveEventTargetObject(this);
> Why is mParentObject nsGlobalWindow here?

This is a bug! I should check for mOwnerWindow here within that if branch.

> 
> 
> >+    JSObject *global = JS_GetGlobalForScopeChain(cx);
> >+    MOZ_ASSERT(global);
> >+
> >+    nsIScriptObjectPrincipal *sop =
> >+        static_cast<nsIScriptObjectPrincipal *>(xpc_GetJSPrivate(global));
> type* name
> Also elsewhere

Ah well... switching back and forth between all these different coding styles... does someone has a patch validator script for this?

> 
> >+    nsCOMPtr<nsIGlobalObject> iglobal = do_QueryInterface(sop);
> >+
> >     nsCOMPtr<nsIXMLHttpRequest> xhr = new nsXMLHttpRequest();
> >-    nsresult rv = xhr->Init(subjectPrincipal, NULL, NULL, NULL);
> >+    nsresult rv = xhr->Init(subjectPrincipal, NULL, iglobal, NULL);
> nullptr
(Assignee)

Comment 90

5 years ago
(In reply to Olli Pettay [:smaug] from comment #86)
> Comment on attachment 715043 [details] [diff] [review]
> part0: merge scope members on nsDocument
> 
> 
> >   if (aScriptObject) {
> >     mHasHadScriptHandlingObject = true;
> >+    mHasHadDefaultView = false;
> Why do we need this if mScriptGlobalObject == aScriptObject?

If mScriptGlobalObject is not null right? But I'm not sure I understand your question.
Yes, if mScriptGlobalObject is non-null and aScriptObject is the same.
(Assignee)

Comment 92

5 years ago
So the mHasHadDefaultView is probably not the right name. I just need a flag that indicates the case when the old mScripObject is out of sync with mScopeObject, so it is null while mScopeObject is not. This can only happen when SetScriptGlobalObject was called (where I set it to true). But if SetScriptHandlingObject is called after this (this is the mScriptGlobalObject == aScriptObject case) then I have to reset that flag back to false, because mScopeObject and mScriptObject is in sync again (because of the line I changed).

I could actually move that line out of the 'if' branch but functionally there were no difference. It is not entirely clear to me the concept behind this logic I preserve here thought, so it's not easy to come up with a descriptive name, or to write a documentation about it...
mHasHadDefaultView sounds ok name to me, though it is not quite clear to me why
you think it is not ok.
(Assignee)

Comment 94

5 years ago
GetDefaultView uses GetWindow which calls GetScriptGlobalObject which does not really depend on this flag... So when do we call SetScriptHandlingObject with null? that is the question, because that changes the flag (or nulls out the old mScriptObject) while does not reset mScriptGlobalObject (so GetDefaultView would work still fine while this flag would indicate that we never had a defaultView...)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #87)
> (In reply to Bobby Holley (:bholley) from comment #83)
> > > Yeah exactly, I think it was for xpcshell...
> > 
> > Hm, can you explain the problem in more detail here? If we're returning a
> > BackstagePass* either way, why does it help to put it on nsIXPConnect?
> 
> I would get a linker error (undefines symbol). Xpcshell only uses the header
> file of BackstagePass this way. And I cannot just put NS_NewBackstagePass
> into the header file since that would give me linker errors because of
> missing member functions definitions at the 'new BackstagePass' line.

Can't you just annotate NS_NewBackstagePass with MOZ_EXPORT or something? It doesn't seem like there should be any fundamental reason why you can call XPIDL methods but not functions.
(Assignee)

Comment 96

5 years ago
(In reply to Bobby Holley (:bholley) from comment #95)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #87)
> > (In reply to Bobby Holley (:bholley) from comment #83)
> Can't you just annotate NS_NewBackstagePass with MOZ_EXPORT or something? It
> doesn't seem like there should be any fundamental reason why you can call
> XPIDL methods but not functions.

Hmmm... Indeed there should be a way. Export should just work. Ok, I'll try it again.
(Assignee)

Comment 97

5 years ago
Created attachment 720588 [details] [diff] [review]
part3: SandboxPrivate for jsd's global

I would still like to preserve the separation of jsd and xpconnect code as much as possible. Like this, using extern utility functions instead of including all the things needed and using mixed c/c++ within a single function. If you hate it, I just give it up and will cut paste the logic over to jsd_high and include all the stuff that is needed. Just the thought of using calloc and new in the same function makes me feel sick, so at least let me keep the SetSandboxPrivate utility function then..
Attachment #715046 - Attachment is obsolete: true
Attachment #720588 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 98

5 years ago
Created attachment 720589 [details] [diff] [review]
part4: BackstagePass
Attachment #715047 - Attachment is obsolete: true
Attachment #720589 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 99

5 years ago
Created attachment 720594 [details] [diff] [review]
part6: using nsIGlobalObject

Whoops, I think in the end no one really reviewed this one, smaug, could you take a look at this please?
Attachment #715050 - Attachment is obsolete: true
Attachment #720594 - Flags: review?(bugs)
(Assignee)

Comment 100

5 years ago
Created attachment 720595 [details] [diff] [review]
part8: JunkDrawer

I tried my best to marketing the splendid name for this concept...
Attachment #715052 - Attachment is obsolete: true
Attachment #720595 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 101

5 years ago
Created attachment 720596 [details] [diff] [review]
part9: nsDOMEventTarget holds a global

Not grabbing global from the context stack any longer.
Attachment #715053 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #720596 - Flags: review?(bugs)
(Assignee)

Comment 102

5 years ago
Sorry for the delay on this, I was on holiday and internet was not useable in the hotel. I addressed the other issues as well locally ofc. I'm still not sure about part0, smaug: do you want me to change something on it?
Comment on attachment 720594 [details] [diff] [review]
part6: using nsIGlobalObject


>+nsDocument::SetScopeObject(nsIGlobalObject* aGlobal)
>+{
>+  mScopeObject = do_GetWeakReference(aGlobal);
We should set the flag here that document has or has had script handling object, right?
Attachment #720594 - Flags: review?(bugs) → review+
Comment on attachment 720596 [details] [diff] [review]
part9: nsDOMEventTarget holds a global

I still don't like the name junk drawer. Would JunkGlobal work?

>+nsDOMEventTargetHelper::BindToOwner(nsIGlobalObject* aOwner)
>+{
>+  if (mParentObject) {
>+    if (mOwnerWindow) {
>+      static_cast<nsGlobalWindow*>(mOwnerWindow)->RemoveEventTargetObject(this);
>+      mOwnerWindow = nullptr;
>+    }
>+    mParentObject = nullptr;
>     mHasOrHasHadOwner = false;
>   }
>   if (aOwner) {
>-    mOwner = aOwner;
>-    mHasOrHasHadOwner = true;
>-    static_cast<nsGlobalWindow*>(mOwner)->AddEventTargetObject(this);
>+    mParentObject = aOwner;
>+    // Let's cache the result of this QI for fast access and off main thread usage
>+    if (mOwnerWindow = nsCOMPtr<nsPIDOMWindow>(do_QueryInterface(aOwner)).get()) {
>+      mHasOrHasHadOwner = true;
>+      static_cast<nsGlobalWindow*>(mOwnerWindow)->AddEventTargetObject(this);
>+    }
>   }
Could you rename mHasOrHasHadOwner to mHasOrHasHadOwnerWindow


>-  return mOwner ? static_cast<nsGlobalWindow*>(mOwner)->GetContextInternal()
>+  nsPIDOMWindow* owner = GetOwner();
>+  return owner ? static_cast<nsGlobalWindow*>(owner)->GetContextInternal()
>                 : nullptr;
Perhaps align : under ?

>   void GetParentObject(nsIScriptGlobalObject **aParentObject)
>   {
>-    if (mOwner) {
>-      CallQueryInterface(mOwner, aParentObject);
>+    if (mParentObject) {
>+      CallQueryInterface(mParentObject, aParentObject);
>     }
>     else {
while you're here, could you change this to
} else {


>+  nsIGlobalObject*           mParentObject;
>+  // mParentObject pre QI-ed and cached
>+  // (it is needed for off main thread access from indexeddb)
No need to mention IDB. We use mOwnerWindow also for other things.

>-    nsCOMPtr<nsPIDOMWindow> owner = do_QueryInterface(scriptObject);
>-    req->Init(docPrincipal, context, owner ? owner->GetOuterWindow() : nullptr,
>-              nullptr);
>+    rv = req->Init(docPrincipal, context, 
>+                   scriptObject ? scriptObject : doc->GetScopeObject(),
>+                   nullptr);
Hmm, could you explain this change. I thought this init wants outer, and scope object is inner.
Attachment #720596 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #104)
> Comment on attachment 720596 [details] [diff] [review]
> part9: nsDOMEventTarget holds a global
> 
> I still don't like the name junk drawer. Would JunkGlobal work?

Well, I think the importance of this feature has less to do with the global and more to do with the compartment (despite the 1-to-1 mapping).

I can buy that "Drawer" might be confusing for non-native english speakers though (it could be read as "something that Draws"). If people are opposed to the name, I'm ok with doing something more descriptive/unwieldy.

DefaultSystemCompartment?
DefaultSystemCompartment sounds too good. I like having something like Dummy or Junk or Trash or similar in the name.
(And yes, this can be about not-being native English speaker.)
(In reply to Olli Pettay [:smaug] from comment #106)
> DefaultSystemCompartment sounds too good. I like having something like Dummy
> or Junk or Trash or similar in the name.
> (And yes, this can be about not-being native English speaker.)

One other place I anticipate using this compartment is when we get rid of the CX-stack. We'll need some equivalent of "pushing null" (that is to say, restoring system privileges), which in a single-cx world would probably involve entering some system compartment.

So you like the word "Junk", just not "Drawer"? JunkScope? JunkCompartment?
Comment on attachment 720588 [details] [diff] [review]
part3: SandboxPrivate for jsd's global

Review of attachment 720588 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/jsd/jsd_xpc.cpp
@@ +3445,5 @@
> +    NS_IF_RELEASE(sop);
> +}
> +
> +bool
> +SetSandboxPrivate(JSObject *glob)

Let's call this AttachPrivateForJSDGlobal. It's already kind of hacky that we're using SandboxPrivate here, so I don't want to make it more confusing than necessary.

@@ +3448,5 @@
> +bool
> +SetSandboxPrivate(JSObject *glob)
> +{
> +    nsresult rv;
> +    nsCOMPtr<nsIPrincipal> nullPrin =  do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);

We don't want to have a mismatch between the principal stored on the compartment and the principal backing the global. So let's find a way to create this before the call to JS_NewGlobalObject and then pass it to this function. Adding another utility function is fine if you want to go that route.

@@ +3454,5 @@
> +
> +    nsCOMPtr<nsIScriptObjectPrincipal> sbp =
> +        new SandboxPrivate(nullPrin, glob);
> +
> +    // Pass on ownership of sop to |sandbox|.

This comment is out of date.
Attachment #720588 - Flags: review?(bobbyholley+bmo) → review-
(In reply to Bobby Holley (:bholley) from comment #107)
> So you like the word "Junk", just not "Drawer"? JunkScope? JunkCompartment?

jst and I spoke about this today. He suggests "JunkBox", which I like.

Of JunkBox, JunkScope, and JunkCompartment, what do people prefer?
(Assignee)

Comment 110

5 years ago
(In reply to Bobby Holley (:bholley) from comment #107)
> One other place I anticipate using this compartment is when we get rid of
> the CX-stack. We'll need some equivalent of "pushing null" (that is to say,
> restoring system privileges), which in a single-cx world would probably
> involve entering some system compartment.
> 
> So you like the word "Junk", just not "Drawer"? JunkScope? JunkCompartment?

Let's make it clear what is the purpose of this thing on the long term first. If it's just for having a compartment around to be used when we have no better idea what to use (and currently using a random compartment). Then JunkBox is fine, but since it's a JSObject* currently I would prefer JunkScope even better. And then we should make an attempt in the future to clean up things, and step-by-step getting rid of the "junk" we have, then remove this from the code.

But if as this comment suggests we plan to use it in the future as the default compartment in some cases, then maybe the junk word is not the right term. And even if I don't want to encourage people to over-use this compartment by labeling it as 'default', that's exactly what it is really.
(Assignee)

Comment 111

5 years ago
(In reply to Olli Pettay [:smaug] from comment #104)
> >+    rv = req->Init(docPrincipal, context, 
> >+                   scriptObject ? scriptObject : doc->GetScopeObject(),
> >+                   nullptr);
> Hmm, could you explain this change. I thought this init wants outer, and
> scope object is inner.

Yes. I think we talked about this, but it was quite some time ago... So xhr should bind to an inner window really but the init expected an outer window, which can lead to some bugs actually. Since as here we have an inner window we want to bind the XHR to but passing its outer window instead, and then the init will take its current inner which might or might not be the same inner... So we considered this a bug. What I did is that I changed the init function to do an innerization if needed, but if it gets an inner, then it just tries to use that. So it does no longer expects strictly an outer.
(Assignee)

Comment 112

5 years ago
(In reply to Olli Pettay [:smaug] from comment #103)
> Comment on attachment 720594 [details] [diff] [review]
> part6: using nsIGlobalObject
> We should set the flag here that document has or has had script handling
> object, right?

Yes! Thanks for noticing...
*Box doesn't really sounds anything JS related. So JunkScope for the scope and JunkCompartment for
the relevant compartment?

But yeah, if we want to start to use the thing more often, it should be named differently.
DefaultSystemScope/DefaultSystemCompartment.
(In reply to Olli Pettay [:smaug] from comment #113)
> *Box doesn't really sounds anything JS related. So JunkScope for the scope
> and JunkCompartment for
> the relevant compartment?
> 
> But yeah, if we want to start to use the thing more often, it should be
> named differently.
> DefaultSystemScope/DefaultSystemCompartment.

Well, _I_ plan on using it for things, internally, but I don't want other people to use it. If I use it for things like the a null-push replacement, I'll abstract it in a way such that the use of that compartment is an XPConnect detail.

So let's call it the JunkScope/JunkCompartment.
Comment on attachment 720589 [details] [diff] [review]
part4: BackstagePass

Review of attachment 720589 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with comments addressed

::: content/base/src/nsContentUtils.cpp
@@ +4647,5 @@
> +{
> +  nsCOMPtr<nsIPrincipal> sysPrin;
> +  // This method can fail according to its signature, but cannot
> +  // based on its implementation... Let's not carry over this
> +  // pointless extra complexity...

Nit: This pattern is common enough that I think the assert is clear enough without the comment. Please remove it.

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +1054,5 @@
>      if (NS_FAILED(rv)) {
>          NS_ERROR("InitClassesWithNewWrappedGlobal failed!");
>          return false;
>      }
>  

I believe you need to call backstagePass->SetGlobalObject(globalObj) somewhere down here.
Attachment #720589 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 720595 [details] [diff] [review]
part8: JunkDrawer

Review of attachment 720595 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with comments addressed.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2875,5 @@
> +    if (!mJunkDrawer) {
> +        JS::Value v;
> +        JSContext *cx = mJSContextStack->GetSafeJSContext();
> +        SandboxOptions options;
> +        options.sandboxName.AssignASCII("XPConnect Junk Drawer");

"XPConnect Junk Compartment"

@@ +2878,5 @@
> +        SandboxOptions options;
> +        options.sandboxName.AssignASCII("XPConnect Junk Drawer");
> +        JSAutoRequest ac(cx);
> +        nsresult rv = xpc_CreateSandboxObject(cx, &v,
> +            nsContentUtils::GetSystemPrincipal(), options);

Please always align the arguments with the opening paren of the function.

@@ +2881,5 @@
> +        nsresult rv = xpc_CreateSandboxObject(cx, &v,
> +            nsContentUtils::GetSystemPrincipal(), options);
> +        NS_ENSURE_SUCCESS(rv, nullptr);
> +        mJunkDrawer = &v.toObject();
> +        mJunkDrawer = js::UnwrapObject(mJunkDrawer);

Let's combine these into one line

@@ +2882,5 @@
> +            nsContentUtils::GetSystemPrincipal(), options);
> +        NS_ENSURE_SUCCESS(rv, nullptr);
> +        mJunkDrawer = &v.toObject();
> +        mJunkDrawer = js::UnwrapObject(mJunkDrawer);
> +        JS_AddNamedObjectRoot(cx, &mJunkDrawer, "XPConnect Junk Drawer");

Update the string here as well.

::: js/xpconnect/src/xpcprivate.h
@@ +921,5 @@
>  
>      AutoMarkingPtr**  GetAutoRootsAdr() {return &mAutoRoots;}
>  
> +    JSObject* GetJunkDrawer();
> +    void DeleteJunkDrawer();

JunkScope.

@@ +974,5 @@
>      bool mWatchdogHibernating;
>      PRTime mLastActiveTime; // -1 if active NOW
>      nsRefPtr<XPCIncrementalReleaseRunnable> mReleaseRunnable;
>      js::GCSliceCallback mPrevGCSliceCallback;
> +    JSObject* mJunkDrawer;

mJunkScope.

::: js/xpconnect/src/xpcpublic.h
@@ +396,5 @@
>  nsIGlobalObject *
>  GetNativeForGlobal(JSObject *global);
> +
> +/**
> + * In some cases native object does not really belong to any compartment.

"a native object". And give an example here?

@@ +400,5 @@
> + * In some cases native object does not really belong to any compartment.
> + * But when for some reason we have to wrap these natives (because of an event
> + * for example) instead of just wrapping them into some random compartment we
> + * find on the context stack (like we did previously) a default compartment is
> + * used. The global of this compartment can be received with this method. It is

"This function returns that compartment's global."

@@ +401,5 @@
> + * But when for some reason we have to wrap these natives (because of an event
> + * for example) instead of just wrapping them into some random compartment we
> + * find on the context stack (like we did previously) a default compartment is
> + * used. The global of this compartment can be received with this method. It is
> + * a singleton on runtime, and we should eventually get away from this. Since

on the runtime

@@ +404,5 @@
> + * used. The global of this compartment can be received with this method. It is
> + * a singleton on runtime, and we should eventually get away from this. Since
> + * conceptually is wrong that we have to wrap a native without knowing which
> + * compartment it belongs. And when no object is created any longer in this
> + * "junkdrawer", we can finally remove it.

Let's remove this part, since the future is more uncertain.

And add this paragraph below:

If you find yourself wanting to use this compartment, you're probably doing something wrong. Callers MUST consult with the XPConnect module owner before using this compartment. If you don't, bholley will hunt you down.

@@ +407,5 @@
> + * compartment it belongs. And when no object is created any longer in this
> + * "junkdrawer", we can finally remove it.
> + */
> +JSObject *
> +GetJunkDrawer();

Per other discussion on the bug, let's call this "GetJunkScope".
Attachment #720595 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 117

5 years ago
Created attachment 721629 [details] [diff] [review]
part12: wrapping nodes into documents compartment

I forgot to upload this patch (I put this change into a separate patch since the last time). There is a very important change I had to make here. I cannot just assert or throw here in case the document does not have a global, because it can mean simply that the window is already dead at the time of wrapping. And since it's a weakref on nsDocument, GetScopeObject will return null in that case and some (few) tests are failing because of this if I just assert... So I ended up using JunkScope to pass the tests since I don't think there is any better way. While I'm updating all the patches I will change this one as well from JunkDrawer to JunkScope ofc, I just would like you to take a look at it conceptually, since this is an important change.
Attachment #721629 - Flags: review?(bobbyholley+bmo)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #117)
> Created attachment 721629 [details] [diff] [review]
> part12: wrapping nodes into documents compartment
> 
> I forgot to upload this patch (I put this change into a separate patch since
> the last time). There is a very important change I had to make here. I
> cannot just assert or throw here in case the document does not have a
> global, because it can mean simply that the window is already dead at the
> time of wrapping. And since it's a weakref on nsDocument, GetScopeObject
> will return null in that case and some (few) tests are failing because of
> this if I just assert... So I ended up using JunkScope to pass the tests
> since I don't think there is any better way. While I'm updating all the
> patches I will change this one as well from JunkDrawer to JunkScope ofc, I
> just would like you to take a look at it conceptually, since this is an
> important change.

Hm, it seems kind of wrong for this to end up in the JunkScope. What happens if we just throw? This seems to be our behavior in SetParentToWindow, which means that in general we don't want to be creating reflectors after the scope has been torn down.
(Assignee)

Comment 119

5 years ago
(In reply to Bobby Holley (:bholley) from comment #118)
> Hm, it seems kind of wrong for this to end up in the JunkScope. What happens
> if we just throw? This seems to be our behavior in SetParentToWindow, which
> means that in general we don't want to be creating reflectors after the
> scope has been torn down.

I must have been tired or something when I pushed that patch that throws here, and accidentally I did a dereference before the throwing in it... anyway, let me re-try this throw when it's null approach and we'll see how does it work out on try. It only occurs in 2 tests btw, so if throwing does not work out we can take a closer look at those tests.
Comment on attachment 721629 [details] [diff] [review]
part12: wrapping nodes into documents compartment

Ok, cancelling review for now. :-)
Attachment #721629 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 121

5 years ago
There is really only one test is failing if I throw there. https://tbpl.mozilla.org/?tree=Try&rev=0be91b6aa927

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug490879.xul

I have no idea why, and it happens only on try, locally it works perfectly fine. If I assert there the test suit is crashing before it could reach this test so I don't have a stack, but currently I'm working on getting one (executing the test earlier in the oth suit)

Updated

5 years ago
Blocks: 851695
(Assignee)

Comment 122

5 years ago
So the bit misleading error message: uncaught exception - NS_ERROR_UNEXPECTED: Unexpected error arg 0 [nsIDOMXULDocument.popupNode] at chrome://mochitests/content/chrome/editor/libeditor/html/tests/test_bug490879.xul:48

Is thrown from CallMethodHelper::GatherAndConvertResults() (XPCWrappedNative.cpp) when the result of the documen.popupNode is wrapped.

By looking at the code and playing a bit with it nsXULDocument::GetPopupNode return sometimes a node via GetWindowRoot that it really shouldn't. Because the window of the document that the node belongs to is already dead. I don't know what service this line in the test serves to start with. And I don't know anything about this area in general but at first glance nsWindowRoot can keep alive a node while has only weak ref to the window, so it seems like it is capable to keep alive a node even after it's window is dead. Who knows more about this code?

I did a hack in nsXULDocument that makes the test pass. It simply returns null for popupNode when the wrapping would fail. (walking up the nodes parent chain, and checking if the scope object of the OwnerDoc is alive):

@@ -1480,19 +1480,25 @@ nsXULDocument::GetPopupNode(nsIDOMNode**
 
     if (!node) {
         nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
         if (pm) {
             node = pm->GetLastTriggerPopupNode(this);
         }
     }
 
-    if (node && nsContentUtils::CanCallerAccess(node))
-      node.swap(*aNode);
-
+    if (node && nsContentUtils::CanCallerAccess(node)) {
+        nsIDocument* doc = nullptr;
+        for (nsCOMPtr<nsINode> inode = do_QueryInterface(node); 
+             !doc && inode; inode = inode->GetParent()) {
+            doc = inode->OwnerDoc();
+        }
+        if (!doc || (doc && doc->GetScopeObject()))
+            node.swap(*aNode);
+    }
     return NS_OK;
 }

I don't feel like this were the proper fix for this issue but I do not know how to fix it properly since I don't know anything about this area, and I really would not like to block the jetpack feature (cross origin xhr) that is depending on this bug forever... So:

1., Anyone can confirm that this is a bug, to return a node like this?
2., Anyone has a good understanding of the owner architecture around WindowRoot to come up with a more sane fix for this problem?
3., Is it possible to land some hack in GetPopupNode or changing the failing test opening up a follow up back and addressing this issue there as a followup after I landed the patches on this bug? Or does that sounds like an absolutely bad idea?

CCing Ehsan since he added the test in question (see Comment 121 for link) so he might know more about this.
Flags: needinfo?(peterv)
(Assignee)

Updated

5 years ago
Flags: needinfo?(peterv) → needinfo?(enndeakin)
Some tests such as this one set document.popupNode because they don't want to open the context menu, or didn't have access to it when the test was written. document.popupNode is deprecated so if only tests are causing issues, they might be able to be converted to opening the context menu and performing their checks (going though the real context menu finds the popup node in a different way via GetLastTriggerPopupNode() instead.)
Flags: needinfo?(enndeakin)
(Assignee)

Comment 124

4 years ago
(In reply to Neil Deakin from comment #123)
> Some tests such as this one set document.popupNode because they don't want
> to open the context menu, or didn't have access to it when the test was
> written. document.popupNode is deprecated so if only tests are causing
> issues, they might be able to be converted to opening the context menu and
> performing their checks (going though the real context menu finds the popup
> node in a different way via GetLastTriggerPopupNode() instead.)

Thanks for the quick reply!

It's actually not the setting of the popupNode that throws but the getting part. So this line: var tmpNode = document.popupNode;
So if I just remove that line and change the document.popupNode = tmpNode; some line later to '= null' the test works just fine. So why do we need the tmpNode here? Can I just remove it from the test?

The other question is that if it's a deprecated API, is it Ok if it throws in (the very rare) case when it would return an orphan node that shouldn't be around on the first place?
Can't we just null out mPopupNode when the window dies?
> It's actually not the setting of the popupNode that throws but the getting part. So this > line: var tmpNode = document.popupNode;

Yes, but the getter only retrieves what someone has explicity set it to. Normal usage doesn't do that or set anything in the windowroot at all.
  
> So if I just remove that line and change the document.popupNode = tmpNode;
> some line later to '= null' the test works just fine. So why do we need the
> tmpNode here? Can I just remove it from the test?
> 

Yes, the test in question looks to be trying to just hold on to the value of document.popupNode and reseting it back to its original value when done. It doesn't need to do that and just setting it to null is fine.

> The other question is that if it's a deprecated API, is it Ok if it throws
> in (the very rare) case when it would return an orphan node that shouldn't
> be around on the first place?

It should just return null.

> Can't we just null out mPopupNode when the window dies?

You can. It would need to iterate over child frames.
(Assignee)

Comment 127

4 years ago
(In reply to Neil Deakin from comment #126)
> It should just return null.

This means I have to fix this.

> 
> > Can't we just null out mPopupNode when the window dies?
> 
> You can. It would need to iterate over child frames.

So I can do that I guess, but it seems a bit expensive. That would mean every window that dies has to warn the windowroot, which then has to traverse the parent chain of the mPopupNode get the document from it, then get the window from it then check it against the window that dies and null it out if they are the same. Or am I missing something? The other version is some custom hack in the GetPopupNode like the one I posted a few comments above. Which one seems like the least pain?
(Assignee)

Comment 128

4 years ago
Created attachment 732738 [details] [diff] [review]
part3: SandboxPrivate for jsd's global

So I decided to push the patches to inbound without the last one. So I won't have to continuously update them until the last bit gets covered. But now I realized that this patch still has r-, I was too busy with that failing test to notice and file this version it seems. Sorry about that.
Attachment #720588 - Attachment is obsolete: true
Attachment #732738 - Flags: review?(bobbyholley+bmo)
Comment on attachment 732738 [details] [diff] [review]
part3: SandboxPrivate for jsd's global

Review of attachment 732738 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley
Attachment #732738 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 130

4 years ago
So I pushed some patches to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7bc2f7391b4f

https://tbpl.mozilla.org/?tree=Try&rev=06d96cb87bc5

As it turned out the bits that uses the junk scope for documents that has no scope needs to be landed with the nsClassInfo bits, so I shifted those bits from part11 to part12. Not much functional changes landed yet with these patches, but the fact that I don't have to update them all the time if they stick, will give me a lot more time to deal with the final bits and follow-ups. First time I had to deal with this many patches, so I needed to rethink my work flow...
Needs a clobber on Windows, landed:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c96c8b020a27
Filed bug 857985 for fixing the need to clobber in this instance.
https://hg.mozilla.org/mozilla-central/rev/2c3ca8354b1e
https://hg.mozilla.org/mozilla-central/rev/6798397d5e12
https://hg.mozilla.org/mozilla-central/rev/630dd45829a3
https://hg.mozilla.org/mozilla-central/rev/6acc9f720ac7
https://hg.mozilla.org/mozilla-central/rev/3cbcfd5416de
https://hg.mozilla.org/mozilla-central/rev/91e4887f38c9
https://hg.mozilla.org/mozilla-central/rev/721d5133532c
https://hg.mozilla.org/mozilla-central/rev/11370b00b64d
https://hg.mozilla.org/mozilla-central/rev/788e34b8872e
https://hg.mozilla.org/mozilla-central/rev/36fcd3edc6b8
https://hg.mozilla.org/mozilla-central/rev/372ae5eae8b9
https://hg.mozilla.org/mozilla-central/rev/7bc2f7391b4f
https://hg.mozilla.org/mozilla-central/rev/c96c8b020a27
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Comment 134

4 years ago
This is not yet fixed, it was only the first part of the push. Also:
Regression: Mozilla-Inbound - Tp5 Optimized - Linux x64 - 6% increase
---------------------------------------------------------------------
    Previous: avg 307.814 stddev 7.974 of 30 runs up to revision b50cc6512288
    New     : avg 326.273 stddev 3.143 of 5 runs since revision 2a6c83f2499e
    Change  : +18.459 (6% / z=2.315)
    Graph   : http://mzl.la/YzqJDY

If I read this correctly then we should probably back it out :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
What's the actual changeset range for that Tp thing?  There's a known Tp regression being investigated in bug 854799.
(Assignee)

Comment 136

4 years ago
According to the email I've got it looks like this is my bad:

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b50cc6512288&tochange=2a6c83f2499e

Or is this not the changeset range you are looking for? Is the email unreliable about the changeset?
The email is not always reliable about the changeset.  But it's worth backing out just to make sure, yes.
From the graph at http://mzl.la/YzqJDY it is clear that this regression occurred well before this bug landed.  This is the same regression discussed in bug bug 854799 comment 25 through 27.
(Assignee)

Comment 139

4 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #138)
> From the graph at http://mzl.la/YzqJDY it is clear that this regression
> occurred well before this bug landed.  This is the same regression discussed
> in bug bug 854799 comment 25 through 27.

Awesome, thanks Matt!
(Assignee)

Comment 140

4 years ago
Created attachment 734336 [details] [diff] [review]
part12: wrapping nodes into documents compartment

It is all green on try finally. So this patch does a bit more than I wanted, but there is no way to split it up. The nsDocument part is r+ by smaug already, I flagged Neil for r? this time too, to approve the changes on the popupNode getter. But I think Bobby, you should take a look at that part too.
Attachment #721629 - Attachment is obsolete: true
Attachment #734336 - Flags: review?(enndeakin)
Attachment #734336 - Flags: review?(bobbyholley+bmo)
Depends on: 859491
Comment on attachment 734336 [details] [diff] [review]
part12: wrapping nodes into documents compartment

Review of attachment 734336 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/xul/document/src/XULDocument.cpp
@@ +1520,5 @@
>  
> +nsIGlobalObject*
> +GetScopeObjectOfNode(nsIDOMNode* node)
> +{
> +    // Window root occasionally keeps alive a node of a document thats

s/thats/whose/

@@ +1522,5 @@
> +GetScopeObjectOfNode(nsIDOMNode* node)
> +{
> +    // Window root occasionally keeps alive a node of a document thats
> +    // window is already dead. If in this brief period someone calls
> +    // GetPopupNode and we return that node, nsNodeSH::PreCreatewill throw.

space before |will|

@@ +1523,5 @@
> +{
> +    // Window root occasionally keeps alive a node of a document thats
> +    // window is already dead. If in this brief period someone calls
> +    // GetPopupNode and we return that node, nsNodeSH::PreCreatewill throw.
> +    // Because it will not know which scope this node belongs to. Returning

Incomplete sentence.

@@ +1529,5 @@
> +    // this, let's do the same check as nsNodeSH::PreCreate does to 
> +    // determine the scope and if it fails let's just return null in
> +    // XULDocument::GetPopupNode.
> +    nsIDocument* doc = nullptr;
> +    for (nsCOMPtr<nsINode> inode = do_QueryInterface(node); 

There are two trailing whitespaces here (one here, and one above). I know we've talked about this before. The best way to deal with this problem is to configure your editor to make trailing whitespace visible, but not to auto-delete them (since that pollutes other stuff). Can you do that?
Attachment #734336 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 142

4 years ago
(In reply to Bobby Holley (:bholley) from comment #141)
> There are two trailing whitespaces here (one here, and one above). I know
> we've talked about this before. The best way to deal with this problem is to
> configure your editor to make trailing whitespace visible, but not to
> auto-delete them (since that pollutes other stuff). Can you do that?

I did that already, apparently sometimes my eyes just skipping them regardless :S I'll just write a script for it.
Attachment #734336 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 143

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=8b904c5faae6

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce65f4eb1ba
https://hg.mozilla.org/mozilla-central/rev/8ce65f4eb1ba
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(In reply to Ryan VanderMeulen [:RyanVM] from comment #144)
> https://hg.mozilla.org/mozilla-central/rev/8ce65f4eb1ba

Backed out for suspicion of causing bug 860903.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85fd0be102d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 860903
(Assignee)

Comment 146

4 years ago
Created attachment 737520 [details] [diff] [review]
part12: wrapping nodes into documents compartment

Based on the stacks the intermittent crash was from the case where the native global is alive but the js global it holds is not any longer, and I returned null from PreCreate for parent because of this, which caused a null pointer exception.

I guarded against it and got green on try three times on all platform. The 2nd and 3rd run was about testing some simplification in the popupNode hack, as well as adjusting its check accordingly (checking for js global availability not just native global)
Attachment #734336 - Attachment is obsolete: true
Attachment #737520 - Flags: review?(bobbyholley+bmo)
Comment on attachment 737520 [details] [diff] [review]
part12: wrapping nodes into documents compartment

Review of attachment 737520 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley, assuming that the primary change here is the NS_ENSURE_TRUE(*parentObj, ...) is the primary difference here. In future, please attach an interdiff in cases like this.

Note that it's possible that those could still be orange when precreate throws rather than crashes. But hopefully your try pushes indicate that this won't happen. We should keep an eye on this to see if those tests start failing (if they do, they probably need to be fixed).
Attachment #737520 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 148

4 years ago
(In reply to Bobby Holley (:bholley) from comment #147)
> Comment on attachment 737520 [details] [diff] [review]
> part12: wrapping nodes into documents compartment
> 
> Review of attachment 737520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=bholley, assuming that the primary change here is the
> NS_ENSURE_TRUE(*parentObj, ...) is the primary difference here. In future,
> please attach an interdiff in cases like this.

Will do.

> 
> Note that it's possible that those could still be orange when precreate
> throws rather than crashes. But hopefully your try pushes indicate that this
> won't happen. We should keep an eye on this to see if those tests start
> failing (if they do, they probably need to be fixed).

Yes, exactly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/baf184596a7b
(Assignee)

Comment 149

4 years ago
Screwed up the previous push, retrying:

https://hg.mozilla.org/integration/mozilla-inbound/rev/259703edf5af
https://hg.mozilla.org/mozilla-central/rev/259703edf5af
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 874158
You need to log in before you can comment on or make changes to this bug.