Actual result: 0:04.60 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Error: P ermission denied to create wrapper for object of class UnnamedClass at http://mo chi.test:8888/tests/dom/tests/mochitest/bugs/file_bug641552.xml:10 0:04.60 JavaScript error: http://mochi.test:8888/tests/dom/tests/mochitest/bugs /file_bug641552.xml, line 10: Permission denied to create wrapper for object of class UnnamedClass Expected result: Either allow the access or hide the property from the XBL scope. Throwing an exception is bad because it makes Object.getOwnPropertyNames() unusable in the XBL scope.

Comment on attachment 789625 [details] [diff] [review] patch with a test Review of attachment 789625 [details] [diff] [review]: ----------------------------------------------------------------- Wait, I'm confused. Do we succeed in creating the property in the content scope, but fail when trying to wrap it over to the content scope? That would be very strange, since and XBL scope has strictly higher privileges than the content scope. Or is there something else happening? ::: dom/tests/mochitest/bugs/test_bug641552.html @@ +34,5 @@ > + is(typeof(window.navigator.randomname2), 'object', "navigator.randomname2 should return an object"); > + > + var span = document.createElement("span"); > + span.style.cssText = "-moz-binding: url(file_bug641552.xml)"; > + document.body.appendChild(span); Add a comment here to indicate that the binding calls finish().

(In reply to Bobby Holley (:bholley) from comment #3) > Wait, I'm confused. Do we succeed in creating the property in the content > scope, but fail when trying to wrap it over to the content scope? That would > be very strange, since and XBL scope has strictly higher privileges than the > content scope. Or is there something else happening? Honestly I don't understand what Voodoo caused the failure here.

When wrapping the object on the content scope, JSCompartment::wrap called WrapForSameCompartment and it succeeded. However, when wrapping it on the XBL scope, JSCompartment::wrap called xpc::WrapperFactory::PrepareForWrapping which ended up with failing in nsScriptSecurityManager::CanCreateWrapper because the object didn't implement nsISecurityCheckedComponent. Here is the stack trace (sorry for the partially localized message): > xul.dll!nsScriptSecurityManager::CanCreateWrapper(JSContext * cx, const nsID & aIID, nsISupports * aObj, nsIClassInfo * aClassInfo, void * * aPolicy) 行 2090 C++ xul.dll!XPCWrappedNative::InitTearOff(XPCWrappedNativeTearOff * aTearOff, XPCNativeInterface * aInterface, bool needJSObject) 行 1881 + 0x2b バイト C++ xul.dll!XPCWrappedNative::FindTearOff(XPCNativeInterface * aInterface, bool needJSObject, tag_nsresult * pError) 行 1724 C++ xul.dll!XPCWrappedNative::GetNewOrUsed(xpcObjectHelper & helper, XPCWrappedNativeScope * Scope, XPCNativeInterface * Interface, XPCWrappedNative * * resultWrapper) 行 617 + 0x14 バイト C++ xul.dll!XPCConvert::NativeInterface2JSObject(JS::Value * d, nsIXPConnectJSObjectHolder * * dest, xpcObjectHelper & aHelper, const nsID * iid, XPCNativeInterface * * Interface, bool allowNativeWrapper, tag_nsresult * pErr) 行 895 C++ xul.dll!NativeInterface2JSObject(JS::Handle<JSObject *> aScope, nsISupports * aCOMObj, nsWrapperCache * aCache, const nsID * aIID, bool aAllowWrapping, JS::Value * aVal, nsIXPConnectJSObjectHolder * * aHolder) 行 582 + 0x17 バイト C++ xul.dll!nsXPConnect::WrapNativeToJSVal(JSContext * aJSContext, JSObject * aScopeArg, nsISupports * aCOMObj, nsWrapperCache * aCache, const nsID * aIID, bool aAllowWrapping, JS::Value * aVal, nsIXPConnectJSObjectHolder * * aHolder) 行 634 + 0x20 バイト C++ xul.dll!xpc::WrapperFactory::PrepareForWrapping(JSContext * cx, JS::Handle<JSObject *> scope, JS::Handle<JSObject *> objArg, unsigned int flags) 行 295 C++ mozjs.dll!JSCompartment::wrap(JSContext * cx, JS::MutableHandle<JS::Value> vp, JS::Handle<JSObject *> existingArg) 行 260 + 0x1a バイト C++ Is this an expected behavior?

(In reply to Masatoshi Kimura [:emk] from comment #5) > When wrapping the object on the content scope, JSCompartment::wrap called > WrapForSameCompartment and it succeeded. Well, I'm confused because the call to WrapNative in nsDOMClassInfo.cpp should end up calling NativeInterface2JSObject, which should cause the exact same security check. So I don't understand why the NativeInterface2JSObject call succeeds for the content scope but not for the XBL scope.

(In reply to Bobby Holley (:bholley) from comment #6) > Well, I'm confused because the call to WrapNative in nsDOMClassInfo.cpp IIUC WrapNative will be called only if the property is a primitive value (JSVAL_IS_PRIMITIVE(prop_val) && !JSVAL_IS_NULL(prop_val)). Obviously an nsIPromptService instance is not a primitive, so WrapNative will not be called.

Oh, I missed that prop_val would be initialized only if the object implements nsIDOMGlobalPropertyInitializer...

Yeah, WrapNative is a function that takes a non-primitive XPCOM object (nsISupports) and generates a javascript reflector for it.

WrapNative will call nsXPConnect::WrapNativeToJSVal with passing null to aIID parameter. JS_WrapValue will call WrapperFactory::PrepareForWrapping with passing &NS_GET_IID(nsISupports) (that is, non-null) to aIID paramter. If the iid parameter is non-null, XPCConvert::NativeInterface2JSObject will call XPCWrappedNative::GetNewOrUsed with passing non-null Interface parameter. If the Interface parameter is non-null, XPCWrappedNative::GetNewOrUsed will call FindTearOff which will throw the exception bycalling JS_ReportError(). https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#617 This was the reason WrapNative didn't fail while JS_WrapValue did.

Also, WrapperFactory::PrepareForWrapping didn't call nsXPConnect::WrapNativeToJSVal with aAllowWrapper set to true.

Ahah! Thanks for the excellent detective work. :-) So yeah. The bug here is actually that XPConnect should default to nsISupports, rather than skipping the FindTearOff call. This will likely cause a number of tests to break, though, including test_bug641552.html. We really shouldn't be allowing the creation of nsIPromptService instances in content at all. :-( I whipped up a patch to implement this behavior, and pushed it to try. Let's see what breaks: https://tbpl.mozilla.org/?tree=Try&rev=37e93227c048

Oh cool - looks like there are only a couple of failures. Masatoshi, do you have any interest in applying this patch and digging into the failures?

(In reply to Bobby Holley (:bholley) from comment #13) > Created attachment 791124 [details] [diff] [review] > Masatoshi, do you have any interest in applying this patch and digging into > the failures? The test_bug642026.html failure was the reason I filed this bug in the first place. (The bug was only visible in the XBL scope before the patch.) Once someone uses addCategoryEntry in somewhere in the test suite, all following tests will throw in the getOwnPropertyNames() call. We can't even add a regression test such as: SpecialPowers.addCategoryEntry("JavaScript-global-property", "randomname", "@mozilla.org/embedcomp/prompt-service;1", false, true); var thrown = false; try { window.randomname; } catch (e) { thrown = true; } ok(thrown, "window.randomname should throw"); because it will break all following tests using getOwnPropertyNames().

FYI, I found this bug when I'm rewriting dom/tests/mochitest/general/test_interfaces.html to use getOwnPropertyNames() because Components.interfaces enumeration will not catch WebIDL interface additions.

Well, can we use something other than the prompt service that has a contractID but is also allowed to be instantiated in content scopes? XHR maybe? I bet if you replace the contractId in that test with |@mozilla.org/xmlextras/xmlhttprequest;1| it'll work.

What about the regression test? Maybe addCategoryEntry should do some security checks before registering the interface? (OK in a follow-up bug.)

(In reply to Masatoshi Kimura [:emk] from comment #17) > What about the regression test? Maybe addCategoryEntry should do some > security checks before registering the interface? (OK in a follow-up bug.) Do you mean a regression test for the patch I attached to the bug? We might not need one - we're just tightening down security invariants here and requiring things to be non-null, which isn't something we're likely to regress. If we wanted to write one, we could add something like nsIPrompt to the category manager, enumerate a content window and make sure it throws, and then remove it again.

> and then remove it again. Looks like it's impossible to remove an interface from the category manager (deleteCategoryEntry() had no effect, getOwnPropertyNames() still throws.) But OK, I wouldn't care about the regression test anymore.

(In reply to Masatoshi Kimura [:emk] from comment #19) > > and then remove it again. > > Looks like it's impossible to remove an interface from the category manager > (deleteCategoryEntry() had no effect, getOwnPropertyNames() still throws.) That sounds like a bug! Can you file it with a testcase and I'll have a look?

(In reply to Bobby Holley (:bholley) from comment #20) > (In reply to Masatoshi Kimura [:emk] from comment #19) > > > and then remove it again. > > > > Looks like it's impossible to remove an interface from the category manager > > (deleteCategoryEntry() had no effect, getOwnPropertyNames() still throws.) > > That sounds like a bug! Can you file it with a testcase and I'll have a look? Files bug 906275.

Comment on attachment 791124 [details] [diff] [review] Default the iid to nsISupports in NativeInterface2JSObject, and assert |Interface| when getting an XPCWrappedNative. v1 With this patch, generateCRMFRequest() fails because nsJSContext::ConvertSupportsTojsvals can no longer wrap a nsKeyGenThread object. https://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp?rev=911fcdcadf4d&mark=1294-1295#1292 > xul.dll!nsJSContext::ConvertSupportsTojsvals(nsISupports * aArgs, JS::Handle<JSObject *> aScope, unsigned int * aArgc, JS::Value * * aArgv, mozilla::Maybe<nsRootedJSValueArray> & aTempStorage) 行 1295 C++ xul.dll!nsJSContext::SetProperty(JS::Handle<JSObject *> aTarget, const char * aPropName, nsISupports * aArgs) 行 1199 + 0x1f バイト C++ xul.dll!nsGlobalWindow::DefineArgumentsProperty(nsIArray * aArguments) 行 3172 + 0x1f バイト C++ xul.dll!nsGlobalWindow::SetArguments(nsIArray * aArguments) 行 3156 C++ xul.dll!nsWindowWatcher::OpenWindowInternal(nsIDOMWindow * aParent, const char * aUrl, const char * aName, const char * aFeatures, bool aCalledFromJS, bool aDialog, bool aNavigate, nsIArray * argv, nsIDOMWindow * * _retval) 行 741 + 0xe バイト C++ xul.dll!nsWindowWatcher::OpenWindow(nsIDOMWindow * aParent, const char * aUrl, const char * aName, const char * aFeatures, nsISupports * aArguments, nsIDOMWindow * * _retval) 行 344 + 0x29 バイト C++ xul.dll!nsNSSDialogHelper::openDialog(nsIDOMWindow * window, const char * url, nsISupports * params, bool modal) 行 44 + 0x2e バイト C++ xul.dll!nsNSSDialogs::DisplayGeneratingKeypairInfo(nsIInterfaceRequestor * aCtx, nsIKeygenThread * runnable) 行 448 C++ xul.dll!cryptojs_generateOneKeyPair(JSContext * cx, nsKeyPairInfoStr * keyPairInfo, int keySize, char * params, nsIInterfaceRequestor * uiCxt, PK11SlotInfoStr * slot, bool willEscrow) 行 831 C++ xul.dll!cryptojs_ReadArgsAndGenerateKey(JSContext * cx, JS::Value * argv, nsKeyPairInfoStr * keyGenType, nsIInterfaceRequestor * uiCxt, PK11SlotInfoStr * * slot, bool willEscrow) 行 988 + 0x22 バイト C++ xul.dll!nsCrypto::GenerateCRMFRequest(JSContext * aContext, const nsCString & aReqDN, const nsCString & aRegToken, const nsCString & aAuthenticator, const nsCString & aEaCert, const nsCString & aJsCallback, const mozilla::dom::Sequence<JS::Value> & aArgs, mozilla::ErrorResult & aRv) 行 1998 + 0x24 バイト C++

(morphing this bug to the issue it's tracking now)

This depends on bug 600460, because we need to remove the stuff registered by the category manager in order to prevent subsequent tests from failing.

This fixes the generateCRMFRequest failures. The callsites here are easily greppable (grep for nsNSSDialogHelper::openDialog), and they're all chrome:// XUL documents.

Comment on attachment 794232 [details] [diff] [review] Fix category manager tests to use a contract-id that we can create in content. v1 LGTM

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d587dd711e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d616c2ef4e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/49c8f6b3f062

https://hg.mozilla.org/mozilla-central/rev/a6d587dd711e https://hg.mozilla.org/mozilla-central/rev/d1d616c2ef4e https://hg.mozilla.org/mozilla-central/rev/49c8f6b3f062