Bugzilla@Mozilla – Bug 460882
setTimeout loses XPCNativeWrappers
Last modified: 2011-01-18 12:50:19 PST
Summon comment box
I'm spinning this bug off of bug 459906. The problem is that session store has (basically) this set up: aContent = XPCNativeWrapper(contentWindow); aContent.setTimeout(function(){this.document.designMode}, 0); The expectation is that 'this' will be an XPCNativeWrapper. However, it isn't because setTimeout goes through the JS timeout handler stuff, which calls into nsJSContext::CallEventHandler, which has no way of wrapping 'this'. We should provide an API in XPConnect to allow callers to wrap native objects like XPConnect was about to call into a function.
Blake, should you own this?
Yeah, I guess so.
If we're losing wrappers isn't that likely an arbitrary code execution bug? All you have to do is find another likely setTimeout() in our chrome or a popular addon.
Well, in practice, you have to find a setTimeout call that uses 'this'.
Blocking on this for now. Blake, speak up if you disagree.
I'm going to steal this from mrbkap.
Created attachment 357055 [details] [review] Patch, v1 This should do it!
Created attachment 357065 [details] [review] Patch, v1.1 This removes one needless assertion and replaces another with code that asserts but fails gracefully if we don't have a frame pointer.
Comment on attachment 357065 [details] [review] Patch, v1.1 >diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp >+ else >+ { >+ if(XPC_XOW_WrapObject(ccx, aScope, &val)) >+ destObj = JSVAL_TO_OBJECT(val); >+ } You need to check if we're switching up scopes here, and if not, check XPC_XOW_ClassNeedsXOW to see if we need a same-origin XOW (for windows and the like). Otherwise, we'll end up with too many XOWs running around, which is bad for perf. Looks good otherwise. I'm sure I'll stamp the next one.
*** Bug 473708 has been marked as a duplicate of this bug. ***
Variant testcase from bug 473708, chrome code operating on a browser window 1. hack chrome code (browser.xul? an addon?) and add something that does the equivalent: var main = window.content.document.getElementById("main"); alert("main: "+main); main.addEventListener("click", function(event){ alert(this);}, false); 2. find or create a page with a "main" element 3. click on main Expected results: both alerts should show XPCNativeWrappers. Actual Results: main: [object XPCNativeWrapper [object HTMLDivElement @ 0xaf018680 (native @ 0xaf2e5220)]] [object HTMLDivElement @ 0xaf018680 (native @ 0xaf2e5220)] (only debug builds will have the binary addresses)
Created attachment 357442 [details] [review] Patch, v1.2 This should address mrbkap's comments.
Comment on attachment 357442 [details] [review] Patch, v1.2 Looks good. Thanks a lot!
Created attachment 357867 [details] [review] Patch, v2 Grr, the patch v1 series didn't fix the problem dveditz identified in comment 11. Apparently we need to do this at a lower level. mrbkap, what do you think about this? I'll run dromeo numbers now to see how badly it hurts.
Created attachment 357878 [details] [review] Patch, v2.1 Ok, this should be ok, I think. Removes the security manager dance in favor of the fast getter (which I'm told we rely on everywhere), adds a comment explaining the XOW wrapping cases, and makes sure that 'thisp' stays rooted when calling the thisObject callback.
Comment on attachment 357878 [details] [review] Patch, v2.1 http://dromaeo.com/?id=58697,58699 run 58697 is without this patch, run 58699 contains it. Looks like some things do slow down a little.
Still working on optimizing this. I've made up over half of the original slowdown on some dromaeo runs, still tinkering though.
ss: This won't make 1.9.0.7 since we still don't have a fix for trunk.
Thanks Ben. Moving to 1.9.0.8.
Created attachment 360588 [details] [review] Patch, v2.2 Ok, this patch exhibits no slowdown overall on dromaeo recommended tests.
Comment on attachment 360588 [details] [review] Patch, v2.2 I like it. For posterity: brendan said in person that the jsscope.h changes were OK by him. We'll put this change up on devmo as soon as this lands.
(In reply to comment #20) > Ok, this patch exhibits no slowdown overall on dromaeo recommended tests. W00t! Awesome work guys!
I doubt any embedder implemented JSObjectOps.thisObject -- indeed I believe and hope few implement JSObjectOps. But dev-doc-needed is the keyword to use. /be
I backed this out because the chrome and a11y mochitests were hanging/crashing on startup. It was either this or bug 467900, and I guessed this looked more likely (it's bigger!). If I'm wrong, I'll back the other one out.
We're getting this exception on chrome test startup: Error: function eval must be called directly, and not by way of a function of another name Source File: chrome://mochikit/content/harness-overlay.xul Line: 57 Looks like we're now wrapping something in an XPCSafeJSObjectWrapperand seeing the latter part of bug 435151.
Created attachment 361644 [details] [review] patch for bent to try This *should* fix mochichrome, but when I run mochichrome, I fail a test and crash horribly in npruntime land. I'm hoping things work out better for bent.
Comment on attachment 361644 [details] [review] patch for bent to try WFM. Ship it.
Pushed changeset 07e345fbdac2 to mozilla-central.
Variant from bug 473708 confirmed fixed in latest trunk nightly. Thanks guys!
This caused bug 478910.
Pushed changeset 715126a986e4 to mozilla-1.9.1. mrbkap has the fix for bug 478910 on the way there too.
(In reply to comment #31) > Pushed changeset 715126a986e4 to mozilla-1.9.1. this caused bug 479211 on mozilla-1.9.1 but not the other branches from what i can tell atm.
Can't take this on the 1.9.0 branch until the regressions are under control :-(
See also bug 480430 - Cannot attach files on gmail, which has been identified as a regression from this fix.
Do we have enough of the regressions nailed now to take this on the 1.9.0 branch? All the flagged ones, but there's always the worry about regressions that weren't identified as such.
It looks like we're ready to take this on the 1.9.0 branch, and given the scary nature (multiple regressions) we'd prefer it to land sooner than later. Please work up a 1.9.0 backport. One that includes the regression fixes would be preferable, but if it's less confusing to backport them individually that could be OK too. Thanks!
Created attachment 384023 [details] [review] Patch for the 1.9.0 branch Still to do: * Finish testing that this actually fixes the bug. * Move the QI to nsIScriptSecurityManager_1_9_0 out of the XPC_WN_ThisObject. The browser actually works with this patch applied.
Comment on attachment 384023 [details] [review] Patch for the 1.9.0 branch nsXPConnect::GetWrapperForObject(): +#if 0 + // XXX XPCPerThreadData::IsMainThread doesn't exist on the 1.9.0 branch. But you add it later on in this patch, so might as well leave this assertion in :) - In XPCConvert::NativeInterface2JSObject(): + if(sameOrigin) + return JS_TRUE; We need to set *dest to wrapper before returning here it seems. - In XPC_WN_JSOp_ThisObject(): + // XXX Expensive? + nsCOMPtr<nsIScriptSecurityManager_1_9_0_BRANCH> secMan = + do_QueryInterface(XPCWrapper::GetSecurityManager()); Yeah, probably expensive. Make XPCWrapper::GetSecurityManager() store and return the right type directly, per your earlier suggestion in person. r+sr=jst with that.
Created attachment 384781 [details] [review] Patch for the 1.9.0 branch, v2 This fixes bug 396851 as well and a bad merge in xpcconvert.cpp.
Checking in caps/idl/nsIScriptSecurityManager.idl; /cvsroot/mozilla/caps/idl/nsIScriptSecurityManager.idl,v <-- nsIScriptSecurityManager.idl new revision: 1.80; previous revision: 1.79 done Checking in caps/include/nsScriptSecurityManager.h; /cvsroot/mozilla/caps/include/nsScriptSecurityManager.h,v <-- nsScriptSecurityManager.h new revision: 1.117; previous revision: 1.116 done Checking in caps/src/nsScriptSecurityManager.cpp; /cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v <-- nsScriptSecurityManager.cpp new revision: 1.364; previous revision: 1.363 done Checking in dom/src/base/nsDOMClassInfo.cpp; /cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v <-- nsDOMClassInfo.cpp new revision: 1.551; previous revision: 1.550 done Checking in js/src/jsdbgapi.c; /cvsroot/mozilla/js/src/jsdbgapi.c,v <-- jsdbgapi.c new revision: 3.152; previous revision: 3.151 done Checking in js/src/jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.507; previous revision: 3.506 done Checking in js/src/jsobj.c; /cvsroot/mozilla/js/src/jsobj.c,v <-- jsobj.c new revision: 3.487; previous revision: 3.486 done Checking in js/src/jsscope.h; /cvsroot/mozilla/js/src/jsscope.h,v <-- jsscope.h new revision: 3.65; previous revision: 3.64 done Checking in js/src/xpconnect/idl/nsIXPCSecurityManager.idl; /cvsroot/mozilla/js/src/xpconnect/idl/nsIXPCSecurityManager.idl,v <-- nsIXPCSecurityManager.idl new revision: 1.13; previous revision: 1.12 done Checking in js/src/xpconnect/idl/nsIXPConnect.idl; /cvsroot/mozilla/js/src/xpconnect/idl/nsIXPConnect.idl,v <-- nsIXPConnect.idl new revision: 1.72; previous revision: 1.71 done Checking in js/src/xpconnect/shell/xpcshell.cpp; /cvsroot/mozilla/js/src/xpconnect/shell/xpcshell.cpp,v <-- xpcshell.cpp new revision: 1.112; previous revision: 1.111 done Checking in js/src/xpconnect/src/XPCCrossOriginWrapper.cpp; /cvsroot/mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp,v <-- XPCCrossOriginWrapper.cpp new revision: 1.42; previous revision: 1.41 done Checking in js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp; /cvsroot/mozilla/js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp,v <-- XPCSafeJSObjectWrapper.cpp new revision: 1.32; previous revision: 1.31 done Checking in js/src/xpconnect/src/XPCWrapper.h; /cvsroot/mozilla/js/src/xpconnect/src/XPCWrapper.h,v <-- XPCWrapper.h new revision: 1.15; previous revision: 1.14 done Checking in js/src/xpconnect/src/nsXPConnect.cpp; /cvsroot/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,v <-- nsXPConnect.cpp new revision: 1.173; previous revision: 1.172 done Checking in js/src/xpconnect/src/xpccallcontext.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpccallcontext.cpp,v <-- xpccallcontext.cpp new revision: 1.26; previous revision: 1.25 done Checking in js/src/xpconnect/src/xpcconvert.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpcconvert.cpp,v <-- xpcconvert.cpp new revision: 1.131; previous revision: 1.130 done Checking in js/src/xpconnect/src/xpcinlines.h; /cvsroot/mozilla/js/src/xpconnect/src/xpcinlines.h,v <-- xpcinlines.h new revision: 1.20; previous revision: 1.19 done Checking in js/src/xpconnect/src/xpcprivate.h; /cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v <-- xpcprivate.h new revision: 1.285; previous revision: 1.284 done Checking in js/src/xpconnect/src/xpcruntimesvc.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpcruntimesvc.cpp,v <-- xpcruntimesvc.cpp new revision: 1.28; previous revision: 1.27 done Checking in js/src/xpconnect/src/xpcwrappednativejsops.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,v <-- xpcwrappednativejsops.cpp new revision: 1.92; previous revision: 1.91 done Checking in js/src/xpconnect/src/xpcwrappednativescope.cpp; /cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp,v <-- xpcwrappednativescope.cpp new revision: 1.75; previous revision: 1.74 done Checking in js/src/xpconnect/tests/mochitest/Makefile.in; /cvsroot/mozilla/js/src/xpconnect/tests/mochitest/Makefile.in,v <-- Makefile.in new revision: 1.9; previous revision: 1.8 done RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/mochitest/inner.html,v done Checking in js/src/xpconnect/tests/mochitest/inner.html; /cvsroot/mozilla/js/src/xpconnect/tests/mochitest/inner.html,v <-- inner.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/js/src/xpconnect/tests/mochitest/test_bug396851.html,v done Checking in js/src/xpconnect/tests/mochitest/test_bug396851.html; /cvsroot/mozilla/js/src/xpconnect/tests/mochitest/test_bug396851.html,v <-- test_bug396851.html initial revision: 1.1 done
(In reply to comment #40) this caused bug 501270
Is the verification of bug 501270 enough to verify this fix for 1.9.0?
It is... on the other hand, I should really have created a mochitest for this for easier verification.
We don't want this one in 1.8, right?
What version(s) does this patch take effect in? Looks like it was taken on one of the 1.9.0 patches, as well as 1.9.1. True? If so, which 1.9.0 patch?
This landed for 1.9.1 (so Firefox 3.5) and 1.9.0.12 according to the keywords.
I've created a crude nsIXPConnect page that covers the API changes here (and the fact that some of them have already become obsolete since this patch). If you happen to look at it and know details not covered on that page (it's pretty much just an auto-crib from the IDL right now), please feel free to fill things out. https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIXPConnect