334875 - Enable components attached to window to know where they live

Status: RESOLVED DUPLICATE of bug 610714

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

Description

[timeless]

note: the sidebar changes here are provided as an example and will enable dom inspector to safely add itself using window.sidebar instead of hacking the rdf resource directly.

cenzic needs the other changes to dom objects in order to replace them.

Comment 1

[timeless]

Attachment: add nsIGlobalScopedObject and refactor various dom objects to use the interface. add support to window.sidebar

the patch to domi is trivial but really belongs in another bug.

Comment 2

[timeless]

we'd actually like this to land on 1.8 branch as it's only adding interfaces, but, let's start w/ getting it into trunk.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 219210 [details] [diff] [review]
add nsIGlobalScopedObject and refactor various dom objects to use the interface. add support to window.sidebar

One potentially bad side effect of this is that random JS can now set/unset the docshell in various window properties, and while that may not immediately seem like a security problem, it does seem like script could trigger odd behaviour at the very least. I'm not sure how much I like that part of this change...

The other thing I'm not very excited about is the name and location of the new interface nsIGlobalScopeObject. Given that all this new interface is is a way to get/set the docshell, a more generic name and location (maybe in docshell/?) would be more appropriate.

Do others have thoughts on this?

Other than that I found these issues while reviewing this patch:

+      nsCOMPtr<nsIGlobalScopedObject> tearoff(do_QueryInterface((nsIDOMNavigator*)mNavigator));
+      if (tearoff)
+        tearoff->SetDocShell(nsnull);

I don't get the naming used here. Why "tearoff"? That's got a very specific meaning in (XP)COM, and this doesn't really seem to imply that in any way. How about renaming all of these to "globalScopeObj" or "gso", or something else, depending on what the name if this interface should be etc?

- In nsGlobalWindow::GetLocation():

   if (!mLocation && mDocShell) {
-    mLocation = new nsLocation(mDocShell);
+    mLocation = new nsLocation(nsnull);
     if (!mLocation) {
       return NS_ERROR_OUT_OF_MEMORY;
     }
+    JSContext *cx = nsnull;
+    if (mContext) {
+      cx = (JSContext *)mContext->GetNativeContext();
+      jsval dummy;
+      ::JS_BeginRequest(cx);
+      JSBool ok = ::JS_GetProperty(cx, mJSObject, "location", &dummy);

This'll need plenty of commenting. As in, why do we do this, etc.

+      JSObject* obj;
+      if (ok &&
+          dummy &&
+          JSVAL_IS_OBJECT(dummy) &&

The last two lines here could be replaced with !JSVAL_IS_PRIMITIVE(dummy), and how about naming "dummy" "val" instead, since you actually do use the variable etc.

+          OBJ_IS_NATIVE(obj = JSVAL_TO_OBJECT(dummy))) {
+        nsCOMPtr<nsISupports> supports = (nsISupports *) JS_GetPrivate(cx, obj);

This is not a safe assumption. You can only safely assume that the private data in a JSObject is nsISupports if it's JSClass flags contain JSCLASS_PRIVATE_IS_NSISUPPORTS.

+        nsCOMPtr<nsIDOMLocation> location = do_QueryInterface(supports);
+
+        if (!location)
+        {
+          nsCOMPtr<nsIXPConnectWrappedNative> xpcNative = 
+                                              do_QueryInterface(supports);
+
+          if (xpcNative) {
+            xpcNative->GetNative(getter_AddRefs(supports));
+
+            location = do_QueryInterface(supports);
+            if (location)
+              mLocation = location;

Wouldn't you want to do this (i.e. store location in mLocation) in the case where the JSObject's private is an nsIDOMLocation implementation as well?

- In nsJSContext::EvaluateString():

     aPrincipal->GetJSPrincipals(mContext, &jsprin);
   }
   else {
     nsCOMPtr<nsIScriptObjectPrincipal> objPrincipal =
       do_QueryInterface(GetGlobalObject(), &rv);
     if (NS_FAILED(rv))
       return NS_ERROR_FAILURE;
     principal = objPrincipal->GetPrincipal();
     if (!principal)
       return NS_ERROR_FAILURE;
-    principal->GetJSPrincipals(mContext, &jsprin);
   }
+  principal->GetJSPrincipals(mContext, &jsprin);

This'll leak jsprin in the case where aPrincipal is non-null. You need to remove the aPrincipal->GetJSPrincipals() call at the top here.

Comment 4

[timeless]

i'll gladly stick the interface anywhere

i don't really care.

i'm perfectly happy calling it gso (or whatever) instead of tearoff. i called it tearoff merely because it has no specific access to the normal methods of the object. whereas before, the object pointer you were using had full access to fun methods.

the why we do this bit is because it allows someone to replace the location object, and a comment about that can certainly be arranged. as to the domlocation as well bit, heh, yeah i suppose i should be nice to c++ implementers - however, i'm not certain that they'd appear as raw objects, i kinda thought that dom was wrapping all objects in the holders and that my impl wasn't being treated specially. jst suggested GetWrappedNativeOfJSObject, which is probably what we really want.

i'm not sure i want to post a new patch until someone decides answers to things (especially interface names).

wrt the principal call at the end, that looks like a bad merge. it looks like i was trying to refactor something. i'm pretty sure it's not really a related change.

Comment 5

[timeless]

turns out that xmlhttprequest needs this too, I'm hoping beaufour will adopt it.

Comment 6

[Pam Greene]

And so does bug 340604...

Comment 7

[Robert Kaiser]

I'm told this also provides a way to do what bug 526209 wants to have...

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(I don't know how this helps with bug 526209)

XHR and other similar C++ object do know their owner via
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIJSNativeInitializer.h

I think we want something similar for JS. window object should be given as a
parameter when the object is initialized or something and the window
should be cleared when the inner window of the outer window changes.

Comment 9

[Matthew Gertner]

It would be great to have something like nsIJSNativeInitalizer in JS. I should note, however, that last time I looked into this, I believe we determined that nsIJSNativeInitializer is not used for global properties (although it is used for global constructors). Would be nice if that were fixed as well for global properties implemented in C++ (this is the case with Prism).

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Bug 610714 suggests a slightly different approach to solving this problem - I think we should just dupe this there and adjust the dependencies.