460882 - setTimeout loses XPCNativeWrappers

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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)]

Blake, should you own this?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, I guess so.

Comment 3

[Daniel Veditz [:dveditz]]

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) (inactive)]

Well, in practice, you have to find a setTimeout call that uses 'this'.

Comment 5

[Johnny Stenback (:jst)]

Blocking on this for now. Blake, speak up if you disagree.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm going to steal this from mrbkap.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This should do it!

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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) (inactive)]

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 11

[Daniel Veditz [:dveditz]]

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!)]

Attachment: Patch, v1.2

This should address mrbkap's comments.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

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!)]

Attachment: 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!)]

Attachment: 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!)]

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!)]

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!)]

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)]

Thanks Ben. Moving to 1.9.0.8.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2.2

Ok, this patch exhibits no slowdown overall on dromaeo recommended tests.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

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)]

(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]]

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-4, no longer working on Mozilla)]

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!)]

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) (inactive)]

Attachment: 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!)]

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!)]

Pushed changeset 07e345fbdac2 to mozilla-central.

Comment 29

[Michael Ryan]

Variant from bug 473708 confirmed fixed in latest trunk nightly. Thanks guys!

Comment 30

[Bob Clary [:bc] (inactive)]

This caused bug 478910.

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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] (inactive)]

(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]]

Can't take this on the 1.9.0 branch until the regressions are under control :-(

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

See also bug 480430 - Cannot attach files on gmail, which has been identified as a regression from this fix.

Comment 35

[Daniel Veditz [:dveditz]]

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]]

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) (inactive)]

Attachment: 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)]

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) (inactive)]

Attachment: 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) (inactive)]

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] (inactive)]

(In reply to comment #40)

this caused bug 501270

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Is the verification of bug 501270 enough to verify this fix for 1.9.0?

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

It is... on the other hand, I should really have created a mochitest for this for easier verification.

Comment 44

[Martin Stránský [:stransky] (ni? me)]

We don't want this one in 1.8, right?

Comment 45

[Eric Shepherd [:sheppy]]

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) (inactive)]

This landed for 1.9.1 (so Firefox 3.5) and 1.9.0.12 according to the keywords.

Comment 47

[Eric Shepherd [:sheppy]]

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