setTimeout loses XPCNativeWrappers

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
XPConnect
P2
normal
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: mrbkap, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({dev-doc-complete, fixed1.9.1, verified1.9.0.12})

Trunk
mozilla1.9.1
x86
Linux
dev-doc-complete, fixed1.9.1, verified1.9.0.12
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.12 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Whiteboard: [sg:high]
Blake, should you own this?
Flags: blocking1.9.1?
(Reporter)

Comment 2

9 years ago
Yeah, I guess so.
Assignee: nobody → mrbkap
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

9 years ago
Well, in practice, you have to find a setTimeout call that uses 'this'.
Blocking on this for now. Blake, speak up if you disagree.
Flags: blocking1.9.1? → blocking1.9.1+

Updated

9 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
I'm going to steal this from mrbkap.
Assignee: mrbkap → bent.mozilla
Status: NEW → ASSIGNED
Created attachment 357055 [details] [diff] [review]
Patch, v1

This should do it!
Attachment #357055 - Flags: superreview?(mrbkap)
Attachment #357055 - Flags: review?(mrbkap)
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.
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

9 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)
Duplicate of this bug: 473708
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)
Whiteboard: [sg:high] → [sg:critical?]
Created attachment 357442 [details] [diff] [review]
Patch, v1.2

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

9 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+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.7?
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.
Attachment #357442 - Attachment is obsolete: true
Attachment #357867 - Flags: superreview?(mrbkap)
Attachment #357867 - Flags: review?(mrbkap)
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.
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)
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.
Flags: blocking1.9.0.7? → blocking1.9.0.7+
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.
Flags: blocking1.9.0.7+ → blocking1.9.0.8+
Created attachment 360588 [details] [diff] [review]
Patch, v2.2

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

9 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+
(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
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.
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

9 years ago
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.
Attachment #360588 - Attachment is obsolete: true
Comment on attachment 361644 [details] [diff] [review]
patch for bent to try

WFM. Ship it.
Attachment #361644 - Flags: review+
Pushed changeset 07e345fbdac2 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 29

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

Updated

9 years ago
Depends on: 478910

Comment 30

9 years ago
This caused bug 478910.
Pushed changeset 715126a986e4 to mozilla-1.9.1.

mrbkap has the fix for bug 478910 on the way there too.
Keywords: fixed1.9.1

Updated

9 years ago
Depends on: 479211

Comment 32

9 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.
Depends on: 479288
Whiteboard: [sg:critical?] → [sg:critical?] also requires bug 479288
Depends on: 479442
Can't take this on the 1.9.0 branch until the regressions are under control :-(
Flags: blocking1.9.0.8+

Updated

9 years ago
Depends on: 479264
See also bug 480430 - Cannot attach files on gmail, which has been identified as a regression from this fix.
Depends on: 480430

Updated

8 years ago
Depends on: 479267
Whiteboard: [sg:critical?] also requires bug 479288 → [sg:critical?] include regression fixes when taking on branches
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?
Depends on: 496113
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

8 years ago
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.
Attachment #384023 - Flags: superreview?(jst)
Attachment #384023 - Flags: review?(jst)
Whiteboard: [sg:critical?] include regression fixes when taking on branches → [sg:critical?] needs r/sr=jst for 1.9.0
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)

Updated

8 years ago
Depends on: 396851
(Reporter)

Comment 39

8 years ago
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.
Attachment #384023 - Attachment is obsolete: true
Attachment #384781 - Flags: superreview?(jst)
Attachment #384781 - Flags: review?(jst)

Updated

8 years ago
Attachment #384781 - Flags: superreview?(jst)
Attachment #384781 - Flags: superreview+
Attachment #384781 - Flags: review?(jst)
Attachment #384781 - Flags: review+
(Reporter)

Comment 40

8 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

8 years ago
(In reply to comment #40)

this caused bug 501270

Updated

8 years ago
Depends on: 501270
Whiteboard: [sg:critical?] needs r/sr=jst for 1.9.0 → [sg:critical?]
Depends on: 502458
Is the verification of bug 501270 enough to verify this fix for 1.9.0?
(Reporter)

Comment 43

8 years ago
It is... on the other hand, I should really have created a mochitest for this for easier verification.
Flags: in-testsuite?

Comment 44

8 years ago
We don't want this one in 1.8, right?
Flags: wanted1.8.1.x-
Keywords: fixed1.9.0.12 → verified1.9.0.12
Group: core-security
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

8 years ago
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.