See bug 470852 comment 17. Is it possible to not quickstub native anon content? Or can we just remove this check in the security manager? If neither, we need to figure out a way to make this work...

Alternately, why exactly do we hand back native anon content from originalTarget? If chrome needs that, fine, but for content we should perform some sort of retargeting, no?

We should give the first non native anon to content, but how to handle cases when there is content JS in the stack and it is C++ which needs the originalTarget.

Create a noscript getter with a different name that doesn't do the security check and migrate all C++ to that?

yeah, ugly, but simple.

Same thing is needed for .relatedTarget. I'll post a patch to Bug 432586.

I'm worried about removing this protection at this point in development. Are we sure we can adequately protect against all places where people can get references to native-anon content? Seems a safer fix is to do what comment 0 says. That said, it'd be great to remove this protection on trunk if it's no longer needed. I think originally the major reason for having it was to prevent access to the inner textfield inside a <input type=file> since that could lead to reading local files. However getting the inner textfield is no longer harmful. I would be worried about setting mutation event listeners on native-anon content though, such as on editor stuff. Since those could execute when it's not safe to run scripts. At least for now.

I think ideally we'd disable quickstubs for now _and_ fix the event stuff. Then make the check an assert and let jesse fuzz it. Then see what we think.

We don't dispatch mutation events on native anon content.

Blocking, and taking bug.

(In reply to comment #7) > I think ideally we'd disable quickstubs for now _and_ fix the event stuff. Do you mean "disable quickstubs for everything", or just for some cases? Disabling quickstubs all over is going to be a massive perf hit at a pretty bad time, so I'm hoping I'm misunderstanding a little. :-/

Disable quickstubs specifically for native anonymous content.

Unfortunately that's not really possible. The quickstubs get installed on the shared prototype objects, and disabling them selectively only for native anonymous content won't work AFAICT. Blake and I talked this over a bunch, and best approach we can think of is to create a dedicated wrapper for this case, let's call it ChromeOnlyWrapper for now, one that's created unconditionally for all native anonymous content nodes, and only lets chrome do anything with them. Reassigning to mrbkap as he'll be doing the work here.

So, I have a wrapper that implements this, and it appears to work. In fact, it works so well, the <video> and <audio> controls don't work at all. So fixing this will also depend on bug 480205, I think.

This is the first (and bigger) half of mrbkap's recent wrapper work. This introduces a new wrapper, called SystemOnlyWrapper, which gets used for all native anonymous content. This wrapper only allows chrome code (system principal), or code originating from a chrome://global/* URI (needed to keep our video controls working).

Comment on attachment 373207 [details] [diff] [review] Introduce SystemOnlyWrapper and use them for native anonymous content. Sayrer, can you look at the last change in this diff, this patch exposed a bug in mochitest itself, where it was throwing JS errors when given objects it doesn't have access to.

Comment on attachment 373207 [details] [diff] [review] Introduce SystemOnlyWrapper and use them for native anonymous content. that's ok, but you need to log() in the catch block, so something silent and perplexing doesn't happen to someone.

sayrer, I'm not sure I follow. I don't see any log() code anywhere in that file, or anywhere close by. Plus, that catch() being empty simply means we fall through to the code below that generates a more general representation (i.e. string) for this object. So I'd be happy to log, but I could use a hint as to how.

(In reply to comment #14) > This wrapper only allows chrome code (system > principal), or code originating from a chrome://global/* URI (needed to keep > our video controls working). So if content gets access to native anon <scrollbar> (for example bug 401549), it may do whatever with it.

(In reply to comment #18) > So if content gets access to native anon <scrollbar> (for example bug 401549), > it may do whatever with it. Isn't this backwards? The running code's filename must begin with chrome://global/. In bug 401549, content won't be able to use the returned object at all.

Comment on attachment 373207 [details] [diff] [review] Introduce SystemOnlyWrapper and use them for native anonymous content. >@@ -6980,7 +6981,8 @@ nsNodeSH::PreCreate(nsISupports *nativeO > > *parentObj = globalObj; > >- return NS_OK; >+ return node->HasFlag(NODE_IS_IN_ANONYMOUS_SUBTREE) ? >+ NS_SUCCESS_CHROME_ACCESS_ONLY : NS_OK; Could you please add some comment to nsIContent::IsInNativeAnonymousSubtree that if it is changed, also this one should be changed.

(In reply to comment #19) > Isn't this backwards? The running code's filename must begin with > chrome://global/. Ah, you mean that only JS *inside* chrome XBL may get access to native anon content? (Even if binding is used in content page.) That sounds good, I think.

> +++ b/dom/base/nsDOMClassInfo.cpp I'd prefer we just move IsInNativeAnonymousSubtree() to nsINode, I think. Failing that, please add the comments smaug asked for. > +++ b/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp >- if (ok && (ok = ::JS_SetReservedSlot(cx, obj, XPCWrapper::sResolvingSlot, >- oldSlotVal))) { >+ JS_SetReservedSlot(cx, obj, XPCWrapper::sResolvingSlot, >+ oldSlotVal); Why is it ok to not check the JS_SetReservedSlot return value here? I understand why we need to make the call even if !ok from the JS_DefineFunction. Should XPCWrapper::sResolvingSlot be renamed to sFlagsSlot? XPC_XOW_NewResolve used to pass JS_FALSE for preserveVal. That's been changed to pass true. Why? > +++ b/js/src/xpconnect/src/XPCSystemOnlyWrapper.cpp > + * Portions created by the Initial Developer are Copyright (C) 2006 Really? Or 2009? I guess you copy/pasted a bunch of this stuff... > + // XXX HACK EWW! Allow chrome://global/ access to these things, Why only "chrome://global/" and not "chrome://" in general? > + JS_ReportError(cx, "Permission denied to access property '%s' from a > non-chrome context", > + JS_GetStringBytes(str)); Not better to do JS_GetStringChars and convert to UTF16? In any case, this is fine for a first cut. If we could log something about the kind of object the access is on, that would be even better. > +XPC_SOW_FunctionWrapper(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, >+ // Allow 'this' to be either a SOW, in which case we unwrap it or something >+ // that isn't a SOW. We disallow invalid SOWs that have no wrapped object. Can you explain why this is the right thing to do (in the comment, ideally)? >+ if (!JS_GetReservedSlot(cx, funObj, XPCWrapper::eWrappedFunctionSlot, >+ &funToCall)) { Are we guaranteed that funObj is a wrapper here? If so, why? If not, why is that ok to do? For example, what guarantees that funToCall will be null for random functions? >+XPC_SOW_AddProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp) > + return XPCWrapper::AddProperty(cx, obj, true, wrappedObj, id, vp); JS_TRUE, right? >+XPC_SOW_DelProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp) > + // XXX: Do we need this ccx here, or any of these places? Good question. You don't seem to use it for anything other than the valid check... why do you need it? > + // Same origin, pass this request along. Not quite. I'd just drop this comment. > +XPC_SOW_Enumerate(JSContext *cx, JSObject *obj) >+ obj = GetWrapper(obj); >+ JSObject *wrappedObj = GetWrappedObject(cx, obj); GetWrapper() can return null, no? >+XPC_SOW_NewResolve(JSContext *cx, JSObject *obj, jsval id, uintN flags, Same issue here. We don't need an AllowedToAct() check in XPC_SOW_Convert? >+XPC_SOW_CheckAccess(JSContext *cx, JSObject *obj, jsval prop, JSAccessMode mode, Can't GetWrappedObject return null here? And if so, is it ok to pass null to JS_CheckAccess? >+XPC_SOW_HasInstance(JSContext *cx, JSObject *obj, jsval v, JSBool *bp) Again, can GetWrappedObject return null here? >+ // GetWrappedObject does an instanceof check. It does? Can you document _why_ we need to unwrap the LHS in this method? >+XPC_SOW_Equality(JSContext *cx, JSObject *obj, jsval v, JSBool *bp) >+ return ((JSExtendedClass *)STOBJ_GET_CLASS(obj))-> >+ equality(cx, obj, OBJECT_TO_JSVAL(test), bp); Can we assert that STOBJ_GET_CLASS(obj) is in fact extended? Also, might be worth documenting why we do the comparison via XPCWrappedNative. Won't this wrapper also be used for chrome-principal non-XPCWrappedNative objects? >+++ b/js/src/xpconnect/src/XPCWrapper.h >+extern JSExtendedClass sXPC_COW_JSClass; What's that? >+ static JSObject *Unwrap(JSContext *cx, JSObject *wrapper); Document, please, especially in terms of how it differs from the other unwrapping functions... > UnwrapSOW So this duplicates existing "get wrapped object" code from the SOW .cpp file but adds a security check? Could we reuse that existing code after doing the security check instead? >+++ b/js/src/xpconnect/src/nsXPConnect.cpp >+ !XPC_SOW_WrapObject(aJSContext, aScope, *_retval, _retval)) That looks like a GC hazard to me. WrapObject assigns its new object to the given slot (so that after that point the |v| passed to it is no longer rooted if this is the caller), then sets private slots... are we guaranteed that said private slot sets don't trigger GC? In any case, it would be safer, imo, to root the in jsval somewhere here. >+++ b/js/src/xpconnect/src/xpcconvert.cpp > + v = OBJECT_TO_JSVAL(destObj); I guess it's ok to clobber v because we're about to return, but it might be cleaner to use a locally-declared jsval here... >+ !XPC_SOW_WrapObject(ccx, xpcscope->GetGlobalJSObject(), >+ v, &v)) && Again, gc-safe? And the '!' before the WrapObject call shouldn't be here, I'd think. Right now this is returning true if WrapObject returns false and vice versa. This should be testable, I would think.

Oh, and document the NS_SUCCESS_CHROME_ACCESS_ONLY thing in the scriptable helper idl, please.

One more comment: can we get some tests here?

I think there are a couple more comments I need to address. I have a comment on my other computer addressing some of the questions.

Comment on attachment 374030 [details] [diff] [review] wip implementing bz's review comments >+++ b/content/base/public/nsINode.h >+ virtual void CheckNotNativeAnonymous() const; Document that this only needs to be virtual so that IsInNativeAnonymousSubtree can be called across module boundaries? >+++ b/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp >+ return XPCWrapper::UnwrapGeneric<&sXPC_XOW_JSClass>(cx, wrapper); Why is that a template? Can't it just be a function with a JS_ExtendedClass* argument? >+ JS_SetReservedSlot(cx, obj, XPCWrapper::sFlagsSlot, > oldSlotVal); Fix the second line's indent here? Or maybe even pull it up to the previous line. My initial comments about checking the return value of JS_SetReservedSlot and the change to XPC_XOW_NewResolve's preserveVal remain. >+++ b/js/src/xpconnect/src/XPCSystemOnlyWrapper.cpp The "chrome://" question remains. >@@ -228,16 +214,18 @@ XPC_SOW_FunctionWrapper(JSContext *cx, J >+ // We do this so that it's possible to use this function with .call on >+ // related objects that are not system only. Aha. Excellent, thank you. This needs a unit test. The comment about why funtoCall is null for random functions remains. >- // A trick! Calling the native directly doesn't push the native onto the >- // JS stack, so interested onlookers will only see us, meaning that they >- // will compute *our* subject principal. ... >- return native(cx, wrappedObj, argc, argv, rval); >+ return JS_CallFunctionValue(cx, wrappedObj, funToCall, argc, argv, rval); Why this change? I was convinced by your earlier comment, and now I'm being somewhat worried about my understanding of the situation here... >@@ -413,21 +370,16 @@ XPC_SOW_Enumerate(JSContext *cx, JSObjec My question about GetWrapper() returning null remains. >@@ -438,52 +390,51 @@ XPC_SOW_NewResolve(JSContext *cx, JSObje And here. > XPC_SOW_Convert(JSContext *cx, JSObject *obj, JSType type, jsval *vp) > { >+ if (!AllowedToAct(cx, JSVAL_VOID)) { >+ return JS_FALSE; >+ } Why is this needed? I know I asked about it, but it really was a question, not a "put it in" comment. If it _is_ needed, we should have unit tests testing those cases. > XPC_SOW_Equality(JSContext *cx, JSObject *obj, jsval v, JSBool *bp) >+ if (!test) { >+ *bp = (v == JSVAL_NULL); Since we only get here when !JSVAL_IS_PRIMITIVE(v), the RHS should just be JS_FALSE, no? >+ } >+ >+ >+ JSClass *clasp = STOBJ_GET_CLASS(test); Nix one line of whitespace? >+ JSClass *clasp = STOBJ_GET_CLASS(test); >+ if (clasp->flags & JSCLASS_IS_EXTENDED) { >+ JSExtendedClass *xclasp = (JSExtendedClass *) clasp; >+ // NB: JSExtendedClass.equality is a required field. >+ return xclasp->equality(cx, obj, v, bp); Shouldn't that be more like: return xclasp->equality(cx, test, OBJECT_TO_JSVAL(obj), bp); or something? That's the equality hook of |test|'s class you're calling. That said, how would that random other class know to unwrap |obj|? I'm really not following the changes here.... >+ test = JSVAL_TO_OBJECT(v); >+ clasp = STOBJ_GET_CLASS(test); >+ if (clasp->flags & JSCLASS_IS_EXTENDED) { >+ JSExtendedClass *xclasp = (JSExtendedClass *) clasp; >+ // NB: JSExtendedClass.equality is a required field. >+ return xclasp->equality(cx, obj, v, bp); Similar issue here. >+++ b/js/src/xpconnect/src/XPCWrapper.cpp >- JSBool preserveVal, JSObject *innerObj, jsval id, >+ JSBool wantDetails, JSObject *innerObj, jsval id, Was this just a change you forgot in the original patch? Or is this a substantive response to one of my comments? >+++ b/js/src/xpconnect/src/XPCWrapper.h > * Used by the cross origin and safe wrappers: the slot that tells the > * AddProperty code that we're resolving a property, and therefore to not do > * a security check. > */ >- static const PRUint32 sResolvingSlot; >+ static const PRUint32 sFlagsSlot; Fix the comment? Especially since your new wrapper uses it too... and since it means something totally different now. > static JSObject *Unwrap(JSContext *cx, JSObject *wrapper); Still needs to be documented. >+ template<JSExtendedClass *xclasp> Like I said earlier, this should imo just take a JSExtendendClass* argument. My comments on nsXPConnect.cpp and xpcconvert.cpp remain.

(In reply to comment #22) > Why is it ok to not check the JS_SetReservedSlot return value here? I > understand why we need to make the call even if !ok from the JS_DefineFunction. JS_SetReservedSlot only returns false if it had to grow (or create) the object's dslots. Since we already called JS_SetReservedSlot on the same slot earlier in the function, we know that this call won't grow or create dslots, and so can't fail. > Why only "chrome://global/" and not "chrome://" in general? IMO this is a really dangerous loophole that we should lock down as tight as we possibly can. chrome://global/ is code that expects to be included by content, so presumably is/can be locked down tighter than chrome code in general. > Not better to do JS_GetStringChars and convert to UTF16? In any case, this is > fine for a first cut. If we could log something about the kind of object the > access is on, that would be even better. I'm going to leave out the object type for now, but I did switch to using %hs and JS_GetStringChars. > Are we guaranteed that funObj is a wrapper here? If so, why? If not, why is > that ok to do? For example, what guarantees that funToCall will be null for > random functions? Note that when you say "wrapper," you *really* mean "function wrapper". We are guaranteed that the callee is one of our function wrappers, for sure, by the JS API. There is no way (outside of misbehaving C++ code, which we sort of have to assume for sanity) to separate a JS(Fast)Native from its callee in JS. > Good question. You don't seem to use it for anything other than the valid > check... why do you need it? I think this might be needed in XOWs (in the cross-origin) case when we call into the scriptable helpers (in order to get the JS_CALLER/NATIVE_CALLER stuff right). Since we don't do any of that, I'm removing the call contexts. > GetWrapper() can return null, no? Hmm, not usually, no. In fact, I don't think we ever need to null check the result of GetWrapper (outside of toString, which someone could use .call on). > In any case, it would be safer, imo, to root the in jsval somewhere here. Hmmm, so in the JS engine, in jsval pointers are usually already rooted. I guess we don't do that in the rest of the world. I'll add some rooting. >> Change to FunctionWrapper to not assume a native function. > Why this change? I was convinced by your earlier comment, and now I'm being > somewhat worried about my understanding of the situation here... I don't remember why I made the change. I just decided at the last minute that it'd be more kosher to use the real API to call the wrapped function instead of this (somewhat confusing) trick. I can revert it if you want, but IMO this is cleaner and easier to read/maintain. > Why is this needed? I know I asked about it, but it really was a question, not > a "put it in" comment. If it _is_ needed, we should have unit tests testing > those cases. This is called for 4 + obj, where |obj| is a SOW. If you don't have access to obj, then IMO you shouldn't be able to do the convesion to number. > Since we only get here when !JSVAL_IS_PRIMITIVE(v), the RHS should just be > JS_FALSE, no? Actually, this is wrong if the right side is an invisible wrapper around null... I'll fix! > Was this just a change you forgot in the original patch? Or is this a > substantive response to one of my comments? Both, actually. The reason that XPC_XOW_NewResolve passes JS_TRUE for the "preserveVal" parameter is because I meant to rename it and use it in this way. I got distracted when I was making that change, though.

>> GetWrapper() can return null, no? >Hmm, not usually, no. Is that because we're generally guaranteed that |obj| has a wrapper on the proto chain? It might be worth documenting that clearly where we define GetWrapper; right now it just says that it might return null. > but IMO this is cleaner and easier to read/maintain. It sure is; as long as it keeps the "doesn't create a stack frame" behavior, I'm all for it. > This is called for 4 + obj, where |obj| is a SOW. So it's not called for string conversions (e.g. |"" + obj| or alert(obj)) or to-object conversions (whenver they happen). I'm probably fine with throwing here even in the alert() case, so the code is fine, but just making sure.

(In reply to comment #28) > Is that because we're generally guaranteed that |obj| has a wrapper on the > proto chain? It might be worth documenting that clearly where we define > GetWrapper; right now it just says that it might return null. Yeah. For most class hooks, I think we're guaranteed that the passed-in object is the object we're looking for. Get/Set are the odd men out in that regard. > It sure is; as long as it keeps the "doesn't create a stack frame" behavior, > I'm all for it. Well, it doesn't, but that doesn't matter. When I wrote the original code in XPCCrossOriginWrapper.cpp, I had grand plans for allowing native frames to participate in subject principal computation. Those plans fell apart (in part because we rely on it in too many other places), so it's OK if our frame exists. > So it's not called for string conversions (e.g. |"" + obj| or alert(obj)) or > to-object conversions (whenver they happen). I'm probably fine with throwing > here even in the alert() case, so the code is fine, but just making sure. Sorry, I missed an "e.g." in that response. It is called for "" + obj.

Blake, I assume you don't have that as an interdiff against the last thing I reviewed (that being the interdiff you posted as the second attachment here)?

OK, there's no way I can make progress on this tonight. I'll look in the morning; if there's an interdiff that will considerably speed up things; otherwise I'll just have to cross-reference line-by-line against the two existing patches and various bug comments... :(

Boris, this should be the changes between the last patch and the first two patches combined.

Comment on attachment 374211 [details] [diff] [review] Interdiff for review. >+++ b/content/base/public/nsINode.h Wed Apr 22 21:51:35 2009 -0700 >+ // modules boundaries. "module" >+++ b/js/src/xpconnect/src/XPCSystemOnlyWrapper.cpp Wed Apr 22 21:51:35 2009 -0700 >@@ -520,26 +520,29 @@ XPC_SOW_Equality(JSContext *cx, JSObject >+ return xclasp->equality(cx, rhs, OBJECT_TO_JSVAL(lhs), bp); So this is basically depending on the fact that OBJECT_TO_JSVAL(nsnull) == JSVAL_NULL, right? Want to PR_STATIC_ASSERT that? >+++ b/js/src/xpconnect/src/XPCWrapper.h Wed Apr 22 21:51:35 2009 -0700 Still need to fix the comment on sFlagsSlot. >+ * Given an arbitrary object, Unwrap will returned the wrapped object if the "return" >+ static JSObject *UnwrapGeneric(JSContext *cx, const JSExtendedClass *xclasp, Document that this will unwrap wrappers of class |xclasp|? >+++ b/js/src/xpconnect/src/xpcconvert.cpp Wed Apr 22 21:51:35 2009 -0700 >+ jsval wrappedObjVal = OBJECT_TO_JSVAL(destObj); >+ AUTO_MARK_JSVAL(ccx, &wrappedObjVal); > if(wrapper->NeedsChromeWrapper()) > { >+ AUTO_MARK_JSVAL(ccx, &v); >+ if(!XPC_SOW_WrapObject(ccx, xpcscope->GetGlobalJSObject(), >+ OBJECT_TO_JSVAL(destObj), >+ &wrappedObjVal)) >+ return JS_FALSE; I'm not sure why we're auto-marking |v|. If the idea is to root destObj and we're guaranteed that |v == OBJECT_TO_JSVAL(destObj)|, then it'd be clearer to just pass |v| to XPC_SOW_WrapObject (and probably assert the above equality). I assume the nsXPConnect.cpp code I commented on for the first patch version is in fact not a GC hazard, since it hasn't been changed? sr=bzbarsky with that clarified, the xpcconvert marking above sorted out, comments fixed. I guess we'll do a followup for the various unit tests here?

(In reply to comment #34) > So this is basically depending on the fact that OBJECT_TO_JSVAL(nsnull) == > JSVAL_NULL, right? Want to PR_STATIC_ASSERT that? We already have 2 (!) JS_STATIC_ASSERT(JSVAL_NULL == 0) in the JS engine and fact that JSVAL_NULL is: #define JSVAL_NULL ((jsval) 0) so I don't think another assertion is necessary. > I'm not sure why we're auto-marking |v|. Oops, left over from one iteration on your previous comments. > I assume the nsXPConnect.cpp code I commented on for the first patch version is > in fact not a GC hazard, since it hasn't been changed? Yeah, I'll add a comment to the idl saying that _retval *must* be a root (currently, the only caller is XPC_WN_ThisObject, which roots retval). > sr=bzbarsky with that clarified, the xpcconvert marking above sorted out, > comments fixed. I guess we'll do a followup for the various unit tests here? Yeah.

I'll post the full diff in a second.

This applies on top of attachment 374238 [details] [diff] [review] and fixes dolske's video control tests by allowing access from C++ (no JS code running at all) through SOWs. I suspect that the real fix for this is somewhere deep in nsXPCWrappedJSClass, but fixing it there at this stage of the game is really scary and hard, so I'll wait.

http://hg.mozilla.org/mozilla-central/rev/da473f63b7ed and http://hg.mozilla.org/mozilla-central/rev/60980742d9da

Looks like the main patch also landed on 191 as: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b96cdb8210fe But the followup fix to deal with video (comment 38) only landed on trunk. I went ahead and pushed it to 191 as a bustage fix: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0e48d1452e28 Since it was causing crashes for me locally, and I'm assuming the tinderbox will start turning lovely fall colors shortly.

Means it is also completely fixed on 1.9.1 now?

Yes.

Blake, We should really have some kind of automated test case for this to prove that normal content can't get to native content via quickstubs (unless you're on our approved set of native anon elements running in chrome context, i.e. video etc). I'm happy to try to pound out a test for this, but I'll need a little more background on what should and shouldn't be allowed.

I don't think there is any way to write a mochitest for this since there shouldn't be a way for content to even get to native anon content. However we've failed at that task many times and that's why we've added this new wrapped. But any time we fail and content can get to native anon content we fix that and so a mochitest can't rely on it. Unless we can drop into UniversalXPConnect mode and use some internal API to reach into an editor or video or something and get hold of a native-anon node, and then drop back out into normal mode and see that we can't use that node. Does anyone know of an API like that?

Yeah, the boxObject APIs.

So to test this something like this should work: (in content code) nativeAnonNode = (function() { netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); node = <boxObject API which bz was referring to> return node; })(); nativeAnonNode.nodeName; ok(false, "previous line should have thrown an exception");

Replace those last two lines with: var threw = false; try { nativeAnonNode.nodeName; } catch (ex if (condition testing that ex is a security exception) { threw = true; } ok(threw, "Should get security exception here"); at least assuming we're talking mochitest here (where uncaught exceptions are treated as test fail).

Comment on attachment 374238 [details] [diff] [review] Final patch [Checkin: Comment 39 & 40] >diff --git a/testing/mochitest/MochiKit/Base.js b/testing/mochitest/MochiKit/Base.js Is this bug/fix specific to Mozilla tree, or could/should it be upstreamed to MochiKit (too)?

That's a good point: it should probably be upstreamed. I don't know the procedure for doing so, though.

Robert, do you recall what the procedure is for upstreaming mochikit stuff?

Sorry, I should have updated the bug... I asked sayrer on IRC, and he pointed me to the MochiKit trac. I'll update the bug again when I file the ticket on their end.

I filed https://trac.mochikit.com/ticket/353