Last Comment Bug 460882 - setTimeout loses XPCNativeWrappers
: setTimeout loses XPCNativeWrappers
Status: RESOLVED FIXED
[sg:critical?]
: dev-doc-complete, fixed1.9.1, verified1.9.0.12
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Linux
: P2 normal (vote)
: mozilla1.9.1
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
: 473708 (view as bug list)
Depends on: 396851 435151 478910 479211 479264 479267 479288 479442 480430 496113 CVE-2009-2664 502458
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-20 17:31 PDT by Blake Kaplan (:mrbkap) (please use needinfo!)
Modified: 2011-01-18 12:50 PST (History)
18 users (show)
jst: blocking1.9.1+
dveditz: blocking1.9.0.12+
dveditz: wanted1.9.0.x+
stransky: wanted1.8.1.x-
mrbkap: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, v1 (7.14 KB, patch)
2009-01-14 16:01 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v1.1 (7.19 KB, patch)
2009-01-14 16:26 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v1.2 (8.34 KB, patch)
2009-01-16 15:15 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
mrbkap: review+
mrbkap: superreview+
Details | Diff | Review
Patch, v2 (11.24 KB, patch)
2009-01-20 14:49 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v2.1 (11.71 KB, patch)
2009-01-20 15:49 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v2.2 (28.63 KB, patch)
2009-02-04 14:37 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
mrbkap: review+
mrbkap: superreview+
Details | Diff | Review
patch for bent to try (29.64 KB, patch)
2009-02-10 15:01 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
bent.mozilla: review+
Details | Diff | Review
Patch for the 1.9.0 branch (50.38 KB, patch)
2009-06-18 16:11 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
jst: superreview+
Details | Diff | Review
Patch for the 1.9.0 branch, v2 (58.92 KB, patch)
2009-06-23 17:24 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
jst: superreview+
Details | Diff | Review

Description Blake Kaplan (:mrbkap) (please use needinfo!) 2008-10-20 17:31:08 PDT
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.
Comment 1 Samuel Sidler (old account; do not CC) 2008-10-27 14:46:59 PDT
Blake, should you own this?
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-10-27 14:48:10 PDT
Yeah, I guess so.
Comment 3 Daniel Veditz [:dveditz] 2008-10-27 15:21:19 PDT
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.
Comment 4 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-10-27 15:45:46 PDT
Well, in practice, you have to find a setTimeout call that uses 'this'.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2008-11-06 17:00:45 PST
Blocking on this for now. Blake, speak up if you disagree.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-09 17:09:46 PST
I'm going to steal this from mrbkap.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-14 16:01:16 PST
Created attachment 357055 [details] [diff] [review]
Patch, v1

This should do it!
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-14 16:26:37 PST
Created attachment 357065 [details] [diff] [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 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-01-14 17:01:57 PST
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.
Comment 10 Daniel Veditz [:dveditz] 2009-01-15 13:24:56 PST
*** Bug 473708 has been marked as a duplicate of this bug. ***
Comment 11 Daniel Veditz [:dveditz] 2009-01-15 13:32:05 PST
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)
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-16 15:15:14 PST
Created attachment 357442 [details] [diff] [review]
Patch, v1.2

This should address mrbkap's comments.
Comment 13 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-01-16 15:18:45 PST
Comment on attachment 357442 [details] [diff] [review]
Patch, v1.2

Looks good. Thanks a lot!
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-20 14:49:19 PST
Created attachment 357867 [details] [diff] [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.
Comment 15 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-20 15:49:20 PST
Created attachment 357878 [details] [diff] [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 16 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-21 13:41:42 PST
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.
Comment 17 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-26 11:49:52 PST
Still working on optimizing this. I've made up over half of the original slowdown on some dromaeo runs, still tinkering though.
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-02-02 13:02:55 PST
ss: This won't make 1.9.0.7 since we still don't have a fix for trunk.
Comment 19 Samuel Sidler (old account; do not CC) 2009-02-02 13:58:29 PST
Thanks Ben. Moving to 1.9.0.8.
Comment 20 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-02-04 14:37:58 PST
Created attachment 360588 [details] [diff] [review]
Patch, v2.2

Ok, this patch exhibits no slowdown overall on dromaeo recommended tests.
Comment 21 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-02-04 15:04:00 PST
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.
Comment 22 Johnny Stenback (:jst, jst@mozilla.com) 2009-02-04 15:30:51 PST
(In reply to comment #20)
> Ok, this patch exhibits no slowdown overall on dromaeo recommended tests.

W00t! Awesome work guys!
Comment 23 Brendan Eich [:brendan] 2009-02-04 16:01:31 PST
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
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-02-06 20:59:07 PST
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.
Comment 25 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-02-06 21:43:39 PST
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.
Comment 26 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-02-10 15:01:08 PST
Created attachment 361644 [details] [diff] [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 27 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-02-10 16:25:37 PST
Comment on attachment 361644 [details] [diff] [review]
patch for bent to try

WFM. Ship it.
Comment 28 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-02-16 13:18:59 PST
Pushed changeset 07e345fbdac2 to mozilla-central.
Comment 29 Michael Ryan 2009-02-17 10:39:55 PST
Variant from bug 473708 confirmed fixed in latest trunk nightly. Thanks guys!
Comment 30 Bob Clary [:bc:] 2009-02-17 13:39:00 PST
This caused bug 478910.
Comment 31 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-02-17 15:22:19 PST
Pushed changeset 715126a986e4 to mozilla-1.9.1.

mrbkap has the fix for bug 478910 on the way there too.
Comment 32 Bob Clary [:bc:] 2009-02-19 02:11:13 PST
(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.
Comment 33 Daniel Veditz [:dveditz] 2009-02-23 11:28:56 PST
Can't take this on the 1.9.0 branch until the regressions are under control :-(
Comment 34 Mike Beltzner [:beltzner, not reading bugmail] 2009-02-27 13:35:49 PST
See also bug 480430 - Cannot attach files on gmail, which has been identified as a regression from this fix.
Comment 35 Daniel Veditz [:dveditz] 2009-05-25 09:58:58 PDT
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.
Comment 36 Daniel Veditz [:dveditz] 2009-06-05 10:15:17 PDT
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!
Comment 37 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-06-18 16:11:23 PDT
Created attachment 384023 [details] [diff] [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 38 Johnny Stenback (:jst, jst@mozilla.com) 2009-06-22 18:33:13 PDT
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.
Comment 39 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-06-23 17:24:49 PDT
Created attachment 384781 [details] [diff] [review]
Patch for the 1.9.0 branch, v2

This fixes bug 396851 as well and a bad merge in xpcconvert.cpp.
Comment 40 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-06-23 20:18:16 PDT
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
Comment 41 Bob Clary [:bc:] 2009-06-30 01:58:36 PDT
(In reply to comment #40)

this caused bug 501270
Comment 42 Al Billings [:abillings] 2009-07-06 15:52:21 PDT
Is the verification of bug 501270 enough to verify this fix for 1.9.0?
Comment 43 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-07-06 18:33:16 PDT
It is... on the other hand, I should really have created a mochitest for this for easier verification.
Comment 44 Martin Stránský 2009-07-07 06:21:00 PDT
We don't want this one in 1.8, right?
Comment 45 Eric Shepherd [:sheppy] 2009-08-31 16:41:20 PDT
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?
Comment 46 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-08-31 17:25:32 PDT
This landed for 1.9.1 (so Firefox 3.5) and 1.9.0.12 according to the keywords.
Comment 47 Eric Shepherd [:sheppy] 2011-01-18 12:50:06 PST
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

Note You need to log in before you can comment on or make changes to this bug.