I hit shaver's assertion that we're not throwing when we process a return value. I tracked the problem down to XPCWrappedNative::GetNewOrUsed not propagating the return value from the PostCreate hook down to its caller. Having fixed that, I noticed that we were stomping over the nice exception that the script security manager was throwing (in this case) in favor of the much less informative NS_ERROR_UNEXPECTED message. I fixed the version of XPCThrower::ThrowException to not clobber existing exceptions, but the version right below it has a similar problem, so I fixed that too. Patch upcoming.

Comment on attachment 211230 [details] [diff] [review] Proposed fix >+XPCThrower::CheckForPendingException(nsresult result, XPCCallContext &ccx) >+{ >+ nsXPConnect* xpc = nsXPConnect::GetXPConnect(); >+ if(!xpc) >+ return JS_FALSE; >+ >+ nsCOMPtr<nsIException> e; >+ xpc->GetPendingException(getter_AddRefs(e)); >+ if(!e) >+ return JS_FALSE; >+ xpc->SetPendingException(nsnull); >+ >+ nsresult e_result; >+ if(NS_SUCCEEDED(e->GetResult(&e_result)) && e_result == result) >+ { >+ if(!ThrowExceptionObject(ccx, e)) >+ JS_ReportOutOfMemory(ccx); >+ return JS_TRUE; >+ } >+ >+ return JS_FALSE; Nit: best to make all early returns have the same sense -- return JS_FALSE. /be

Comment on attachment 211230 [details] [diff] [review] Proposed fix sr=shaver

Fix checked into trunk.

I had to back this out because it turned the tinderbox orange.

I'm mainly requesting review on the nsDOMClassInfo changes. The changes in xpconnect are unchanged from above. The problem here is that someone is trying to set window.location.href in a window that isn't of the same origin. Resolving window.location causes us to try to wrap the location object, which causes resolution of its constructor Location. Since we're looking at cross-site scripting, resolving the constructor is causing a security check to fail, and we're throwing. This is easily fixed by toggling nsDOMClassInfo's sDoSecurityCheckInAddProperty member. The second problem is that nsDOMClassInfo::PostCreate tries to call GetProperty on the property that it just created. Because we're in a different domain, the security check in GetProperty throws, and we still fail. Because the call to GetProperty can call getters, we can't have a sDoSecurityCheckInGetProperty (since we don't want to let getters do arbitrary cross-site scripting). I've removed the GetProperty here to fix that problem, but nobody seems to be really sure why it was there in the first place (or why we thought we needed it there anyway). fwiw, the browser starts and runs without any noticeable ill-effects from this change.

Comment on attachment 217072 [details] [diff] [review] Proposed fix without tbox orange My memory is clearly failing me here as I have no recollection of *why* I added that JS_GetProperty() call there. I'm fine with landing this in hope that we learn it's not needed :) r+sr=jst

Fix checked into trunk, again.

The JS_GetProperty thing is bug 297145. See in particular bug 297145 comment 2.

I backed this patch out again since it broke setting functions on <foo>.prototype.

Interdiff between attachment 217072 [details] [diff] [review] and the even better version: diff -u dom/src/base/nsDOMClassInfo.cpp dom/src/base/nsDOMClassInfo.cpp --- dom/src/base/nsDOMClassInfo.cpp 3 Apr 2006 21:24:28 -0000 +++ dom/src/base/nsDOMClassInfo.cpp 28 Apr 2006 01:29:03 -0000 @@ -3351,10 +3358,20 @@ nsDOMClassInfo::PostCreate } #endif + // Look up the name of our constructor in the current global scope. We do + // this because triggering this lookup can cause us to call + // nsWindowSH::NewResolve, which will end up in nsWindowSH::GlobalResolve. + // GlobalResolve does some prototype magic (which satisfies the if condition + // above) in order to make sure that prototype delegation works correctly. + // Consider if a site sets HTMLElement.prototype.foopy = function () { ... } + // Now, calling document.body.foopy() needs to ensure that looking up foopy + // on HTMLBodyElement.prototype will find the right function. This + // LookupProperty accomplishes that. + // XXX This shouldn't need to go through the JS engine. Instead, we should + // be calling nsWindowSH::GlobalResolve directly. JSObject *global = GetGlobalJSObject(cx, obj); - jsval val; - if (!::JS_GetProperty(cx, global, mData->mName, &val)) { + if (!::JS_LookupProperty(cx, global, mData->mName, &val)) { return NS_ERROR_UNEXPECTED; } jst: Does this look good to check in? I've tested to make sure that it allows the bloat tests to work and the testcase in the comment works as well.

This was checked in for bug 340340.