Closed
Bug 460882
Opened 16 years ago
Closed 16 years ago
setTimeout loses XPCNativeWrappers
Categories
(Core :: XPConnect, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: mrbkap, Assigned: bent.mozilla)
References
Details
(Keywords: dev-doc-complete, fixed1.9.1, verified1.9.0.12, Whiteboard: [sg:critical?])
Attachments
(2 files, 7 obsolete files)
29.64 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
58.92 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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.
Updated•16 years ago
|
Whiteboard: [sg:high]
Comment 3•16 years ago
|
||
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.
Reporter | ||
Comment 4•16 years ago
|
||
Well, in practice, you have to find a setTimeout call that uses 'this'.
Comment 5•16 years ago
|
||
Blocking on this for now. Blake, speak up if you disagree.
Flags: blocking1.9.1? → blocking1.9.1+
Updated•16 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Assignee | ||
Comment 6•16 years ago
|
||
I'm going to steal this from mrbkap.
Assignee: mrbkap → bent.mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•16 years ago
|
||
This should do it!
Attachment #357055 -
Flags: superreview?(mrbkap)
Attachment #357055 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•16 years ago
|
||
This removes one needless assertion and replaces another with code that asserts but fails gracefully if we don't have a frame pointer.
Attachment #357055 -
Attachment is obsolete: true
Attachment #357065 -
Flags: superreview?(mrbkap)
Attachment #357065 -
Flags: review?(mrbkap)
Attachment #357055 -
Flags: superreview?(mrbkap)
Attachment #357055 -
Flags: review?(mrbkap)
Reporter | ||
Comment 9•16 years ago
|
||
Comment on attachment 357065 [details] [diff] [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.
Attachment #357065 -
Flags: superreview?(mrbkap)
Attachment #357065 -
Flags: review?(mrbkap)
Comment 11•16 years ago
|
||
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)
Updated•16 years ago
|
Whiteboard: [sg:high] → [sg:critical?]
Assignee | ||
Comment 12•16 years ago
|
||
This should address mrbkap's comments.
Attachment #357065 -
Attachment is obsolete: true
Attachment #357442 -
Flags: superreview?(mrbkap)
Attachment #357442 -
Flags: review?(mrbkap)
Reporter | ||
Comment 13•16 years ago
|
||
Comment on attachment 357442 [details] [diff] [review]
Patch, v1.2
Looks good. Thanks a lot!
Attachment #357442 -
Flags: superreview?(mrbkap)
Attachment #357442 -
Flags: superreview+
Attachment #357442 -
Flags: review?(mrbkap)
Attachment #357442 -
Flags: review+
Updated•16 years ago
|
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.7?
Assignee | ||
Comment 14•16 years ago
|
||
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.
Attachment #357442 -
Attachment is obsolete: true
Attachment #357867 -
Flags: superreview?(mrbkap)
Attachment #357867 -
Flags: review?(mrbkap)
Assignee | ||
Comment 15•16 years ago
|
||
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.
Attachment #357867 -
Attachment is obsolete: true
Attachment #357878 -
Flags: superreview?(mrbkap)
Attachment #357878 -
Flags: review?(mrbkap)
Attachment #357867 -
Flags: superreview?(mrbkap)
Attachment #357867 -
Flags: review?(mrbkap)
Assignee | ||
Comment 16•16 years ago
|
||
Comment on attachment 357878 [details] [diff] [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.
Updated•16 years ago
|
Flags: blocking1.9.0.7? → blocking1.9.0.7+
Assignee | ||
Comment 17•16 years ago
|
||
Still working on optimizing this. I've made up over half of the original slowdown on some dromaeo runs, still tinkering though.
Assignee | ||
Comment 18•16 years ago
|
||
ss: This won't make 1.9.0.7 since we still don't have a fix for trunk.
Assignee | ||
Comment 20•16 years ago
|
||
Ok, this patch exhibits no slowdown overall on dromaeo recommended tests.
Attachment #357878 -
Attachment is obsolete: true
Attachment #360588 -
Flags: superreview?(mrbkap)
Attachment #360588 -
Flags: review?(mrbkap)
Attachment #357878 -
Flags: superreview?(mrbkap)
Attachment #357878 -
Flags: review?(mrbkap)
Reporter | ||
Comment 21•16 years ago
|
||
Comment on attachment 360588 [details] [diff] [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.
Attachment #360588 -
Flags: superreview?(mrbkap)
Attachment #360588 -
Flags: superreview+
Attachment #360588 -
Flags: review?(mrbkap)
Attachment #360588 -
Flags: review+
Comment 22•16 years ago
|
||
(In reply to comment #20)
> Ok, this patch exhibits no slowdown overall on dromaeo recommended tests.
W00t! Awesome work guys!
Comment 23•16 years ago
|
||
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
Keywords: dev-doc-needed
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.
Assignee | ||
Comment 25•16 years ago
|
||
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.
Depends on: 435151
Reporter | ||
Comment 26•16 years ago
|
||
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.
Attachment #360588 -
Attachment is obsolete: true
Assignee | ||
Comment 27•16 years ago
|
||
Comment on attachment 361644 [details] [diff] [review]
patch for bent to try
WFM. Ship it.
Assignee | ||
Updated•16 years ago
|
Attachment #361644 -
Flags: review+
Assignee | ||
Comment 28•16 years ago
|
||
Pushed changeset 07e345fbdac2 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 29•16 years ago
|
||
Variant from bug 473708 confirmed fixed in latest trunk nightly. Thanks guys!
Comment 30•16 years ago
|
||
This caused bug 478910.
Assignee | ||
Comment 31•16 years ago
|
||
Pushed changeset 715126a986e4 to mozilla-1.9.1.
mrbkap has the fix for bug 478910 on the way there too.
Keywords: fixed1.9.1
Comment 32•16 years ago
|
||
(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.
Updated•16 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?] also requires bug 479288
Comment 33•16 years ago
|
||
Can't take this on the 1.9.0 branch until the regressions are under control :-(
Flags: blocking1.9.0.8+
Comment 34•16 years ago
|
||
See also bug 480430 - Cannot attach files on gmail, which has been identified as a regression from this fix.
Depends on: 480430
Updated•16 years ago
|
Whiteboard: [sg:critical?] also requires bug 479288 → [sg:critical?] include regression fixes when taking on branches
Comment 35•16 years ago
|
||
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.
Flags: blocking1.9.0.12?
Comment 36•16 years ago
|
||
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!
Flags: blocking1.9.0.12? → blocking1.9.0.12+
Reporter | ||
Comment 37•16 years ago
|
||
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.
Attachment #384023 -
Flags: superreview?(jst)
Attachment #384023 -
Flags: review?(jst)
Updated•16 years ago
|
Whiteboard: [sg:critical?] include regression fixes when taking on branches → [sg:critical?] needs r/sr=jst for 1.9.0
Comment 38•16 years ago
|
||
Comment on attachment 384023 [details] [diff] [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.
Attachment #384023 -
Flags: superreview?(jst)
Attachment #384023 -
Flags: superreview+
Attachment #384023 -
Flags: review?(jst)
Attachment #384023 -
Flags: review+
Reporter | ||
Comment 39•16 years ago
|
||
This fixes bug 396851 as well and a bad merge in xpcconvert.cpp.
Attachment #384023 -
Attachment is obsolete: true
Attachment #384781 -
Flags: superreview?(jst)
Attachment #384781 -
Flags: review?(jst)
Updated•16 years ago
|
Attachment #384781 -
Flags: superreview?(jst)
Attachment #384781 -
Flags: superreview+
Attachment #384781 -
Flags: review?(jst)
Attachment #384781 -
Flags: review+
Reporter | ||
Comment 40•16 years ago
|
||
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
Keywords: fixed1.9.0.12
Comment 41•16 years ago
|
||
(In reply to comment #40)
this caused bug 501270
Updated•16 years ago
|
Depends on: CVE-2009-2664
Updated•16 years ago
|
Whiteboard: [sg:critical?] needs r/sr=jst for 1.9.0 → [sg:critical?]
Comment 42•16 years ago
|
||
Is the verification of bug 501270 enough to verify this fix for 1.9.0?
Reporter | ||
Comment 43•16 years ago
|
||
It is... on the other hand, I should really have created a mochitest for this for easier verification.
Flags: in-testsuite?
We don't want this one in 1.8, right?
Flags: wanted1.8.1.x-
Updated•16 years ago
|
Keywords: fixed1.9.0.12 → verified1.9.0.12
Updated•16 years ago
|
Group: core-security
Comment 45•15 years ago
|
||
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?
Reporter | ||
Comment 46•15 years ago
|
||
This landed for 1.9.1 (so Firefox 3.5) and 1.9.0.12 according to the keywords.
Comment 47•14 years ago
|
||
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
Updated•14 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Updated•7 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•