Bug 480205 (cow)

Implement a wrapper for exposing chrome objects to content

RESOLVED FIXED in mozilla1.9.2a1

Status

()

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

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
blocking1.9.2 +
blocking1.9.1 -
wanted1.9.1 +
blocking1.9.1.1 -
wanted1.9.0.x +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 -, status1.9.1 wanted)

Details

(Whiteboard: [sg:critical prevention])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
We're tentatively calling this UnsafeJSObjectWrapper, but want a better name. This will allow extensions to safely and easily stick chrome objects into content.

One caveat (as always) is that any methods on objects exposed this way will be available to content (duh) and the implementations must take care not to expect callers to follow the rules.

I'm filing this as security sensitive because I don't want to file a public bug that says, "Hey look at these types of objects for security holes!"
Flags: blocking1.9.1?
(Assignee)

Updated

10 years ago
Blocks: 454363, 479560

Comment 1

10 years ago
(In reply to comment #0)
> We're tentatively calling this UnsafeJSObjectWrapper, but want a better name.
> This will allow extensions to safely and easily stick chrome objects into
> content.

ChromeObjectWrapper?
(Assignee)

Comment 2

10 years ago
(In reply to comment #1)
> ChromeObjectWrapper?

Sold!

Comment 3

10 years ago
I agree this is good... why does it need to block this release, instead of making it this one if there's time, or next one if not?
(Assignee)

Comment 4

10 years ago
See bug 479560, comment 5.

Comment 5

10 years ago
While I don't know what is in 479560, I'll say that the alternative here is very painful. I'd guess the Firebug console was >2 person months vs 2 weeks with something like this. So the potential for extension to forgo security is high.
Whiteboard: [sg:critical prevention]
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.8+
Chrome* is not the best name in light of that other browser, IMHO. What are the WebKit wrappers called (there are several now, they based their work in part on Mozilla precedent)? Can we hope to use common names where possible?

/be
As an alternative to John J. Barton's ChromeObjectWrapper I suggested SystemObjectWrapper to blake in person, to make hopefully make it clearer what it does, as "chrome" is pretty over loaded, and unless you know a bunch about Mozilla already, it might not be clear what "chrome" means. Blake didn't feel strongly that it was a better name, and I can't say I do either. But it would be another alternative name...

Comment 8

10 years ago
(In reply to comment #6)
> Chrome* is not the best name in light of that other browser, IMHO. 

Yea, but that's just because they picked a dumb name ;-) 

PlatformObjectWrapper? BrowserObjectWrapper? or we can go 'non': NonContentObjectWrapper?

Actually I think ChromeObjectWrapper will mean the most to the people who need to understand it. It's how blake explained what it was to us.  We count ;-)
Blocking since this blocks other blocker bugs.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
PrivilegedObjectWrapper? Might imply that the Wrapper itself was privileged, though. I like the System one except I worry addon authors will think that "System" means core browser stuff, excluding their own objects.

AppObjectWrapper? ApplicationOW?

I don't mind the "chrome" name myself, we use that term all over the code. I seriously doubt addon authors will be confused and think that's a wrapper for objects imported from Chromium.

Comment 11

10 years ago
I suggested TrustedObjectProtector over lunch a day or two ago, thinking outside the wrapper-box.  "facade" also seems like a good noun to use somewhere in the wrapper mess, but I'm not sure where yet.  :-)
Flags: blocking1.9.0.8+ → blocking1.9.0.9+
(Assignee)

Comment 12

9 years ago
Created attachment 372152 [details] [diff] [review]
WIP

Here's a work in progress...

I plan on squashing this before the final commit (except for a couple of changesets). I still have to finish the ChromeObjectWrapper for real... There's some tricky stuff regarding when to wrap vp in what and I need to finish wrapping getters properly.
(Assignee)

Updated

9 years ago
Priority: P2 → P1
Anything to post that people can start banging on?  Would like to front-load regression-finding if we're able.
(Assignee)

Comment 14

9 years ago
This wrapper isn't ready for banging-on yet. I'm working on it now, though!
Since we have an alternate fix for bug 479560 we're not in a rush to get this scary-big patch into the 1.9.0 branch. We can take it for a later release when it's gotten sufficient testing.
Flags: blocking1.9.0.10+
Blake: did you put this on tryserver recently? Are we ready to get more people helping with the debug and testing here?
Downgrading to P2 per discussion with mrbkap and beltzner. Given that this wrapper will be (at least initially) very limited in its exposure (will only be created when content accesses an object exposed on a window using the category "JavaScript global property", which in our code pretty much means only the sidebar and feedreader code, we'll take the time to finish this after the beta, write specific tests for this, and then consider taking this for RC, rather than delaying the beta for this.
Priority: P1 → P2
Been some time since we've seen an update here.  How are we doing, Blake?  If this is a "scary-big" patch, we should get this in sooner rather than later, right?
(Assignee)

Comment 19

9 years ago
Yeah, we should. I think I've figured out the final pieces, which I'm implementing today and will push to the try server ASAP.
(Assignee)

Updated

9 years ago
Alias: cow
(Assignee)

Updated

9 years ago
Depends on: 493886
(Assignee)

Comment 20

9 years ago
Created attachment 378695 [details] [diff] [review]
patch v1
Attachment #372152 - Attachment is obsolete: true
Attachment #378695 - Flags: superreview?(jst)
Attachment #378695 - Flags: review?(jst)
Comment on attachment 378695 [details] [diff] [review]
patch v1

- In XPC_COW_toString():

+    NS_NAMED_LITERAL_CSTRING(protoString, "[object XPCCrossOriginWrapper]");

You mean XPCChromeObjectWrapper here, right?

r+sr=jst with that. The rest looks good to me.
Attachment #378695 - Flags: superreview?(jst)
Attachment #378695 - Flags: superreview+
Attachment #378695 - Flags: review?(jst)
Attachment #378695 - Flags: review+
Oh, except for the fact that you'll need a new interface for the nsIXPConnect changes.
But of course only for 1.9.1.
(Assignee)

Comment 24

9 years ago
http://hg.mozilla.org/mozilla-central/rev/1abeb6c87131
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Backed out of mozilla-central due to mochitest failures and leaks (across platforms):

*** 41765 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_allowCurrent.html | Got geolocation notification
*** 41766 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_allowCurrent.html | Error thrown during test: bar is null - got 0, expected 1
*** 41770 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_allowWatch.html | Got geolocation notification
*** 41771 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_allowWatch.html | Error thrown during test: bar is null - got 0, expected 1
*** 41774 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_cancelCurrent.html | Ensure that the error was PERMISSION_DENIED
*** 41777 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_cancelWatch.html | Ensure that the error was PERMISSION_DENIED
*** 41780 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_clearWatch.html | we should not be seeing failures from this watchPosition
*** 41782 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_clearWatch.html | Got geolocation notification
*** 41783 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_clearWatch.html | Error thrown during test: bar is null - got 0, expected 1
*** 41786 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_clearWatch_invalid.html | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMGeoGeolocation.clearWatch]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://localhost:8888/tests/dom/tests/mochitest/geolocation/test_clearWatch.html :: clearWatch :: line 38" data: no] - got 0, expected 1
*** 41802 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_optional_api_params.html | Got geolocation notification
*** 41803 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_optional_api_params.html | Error thrown during test: bar is null - got 0, expected 1
*** 41806 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_timeoutWatch.html | nothing is hooked up to test against.
*** 41810 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_brokenUTF-16.html | Got geolocation notification
*** 41811 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/localstorage/test_brokenUTF-16.html | Error thrown during test: bar is null - got 0, expected 1
*** 44740 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_plugin_clipping.xhtml | Matching regions: expected {100,100,200,200}, got {100,100,100,100}
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 5842383 bytes during test execution

http://hg.mozilla.org/mozilla-central/rev/288e71bdc98a
http://hg.mozilla.org/mozilla-central/rev/7f0905da769c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Some of the tests that failed with this patch were the geolocation tests (most of them). One failure I see in those tests is that the geolocation service is unable to load its provider, which when running mochitest is a provider implemented as a JS component. The failure seems to come from a security check that's failing, the stack for it looks like this:

#0  nsScriptSecurityManager::CheckPropertyAccessImpl (this=0x7fffe4a378e0, 
    aAction=0, aCallContext=0x7fffffffaa30, cx=0x7fffda021800, 
    aJSObject=0x7fffd0d46d40, aObj=0x7fffd09ed900, aTargetURI=0x0, 
    aClassInfo=0x0, aClassName=0x0, aProperty=140737029637524, 
    aCachedClassPolicy=0x0)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:811
#1  0x00007fffe08300ee in nsScriptSecurityManager::CanAccess (
    this=0x7fffe4a378e0, aAction=0, aCallContext=0x7fffffffaa30, 
    cx=0x7fffda021800, aJSObject=0x7fffd0d46d40, aObj=0x7fffd09ed900, 
    aClassInfo=0x0, aPropertyName=140737029637524, aPolicy=0x0)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:3069
#2  0x00007fffe42bce39 in XPCWrappedNative::CallMethod (ccx=@0x7fffffffaa30, 
    mode=XPCWrappedNative::CALL_METHOD)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2078
#3  0x00007fffe42ca677 in XPC_WN_CallMethod (cx=0x7fffda021800, 
    obj=0x7fffd0d46d40, argc=2, argv=0x7fffd0fa3088, vp=0x7fffffffad80)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
#4  0x00007fffe42de265 in XPC_COW_FunctionWrapper (cx=0x7fffda021800, 
    obj=0x7fffd0d46d80, argc=2, argv=0x7fffd0fa3088, rval=0x7fffffffad80)
    at ../../../../../mozilla/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp:242
#5  0x00007ffff7682cd8 in js_Invoke (cx=0x7fffda021800, argc=2, 
    vp=0x7fffd0fa3078, flags=2) at ../../../mozilla/js/src/jsinterp.cpp:1385
#6  0x00007fffe42b5df3 in nsXPCWrappedJSClass::CallMethod (
    this=0x7fffe4acb970, wrapper=0x7fffd0632300, methodIndex=3, 
    info=0x7fffe4ace9e0, nativeParams=0x7fffffffb3c0)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1652
#7  0x00007fffe42ad2ef in nsXPCWrappedJS::CallMethod (this=0x7fffd0632300, 
    methodIndex=3, info=0x7fffe4ace9e0, params=0x7fffffffb3c0)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:561
#8  0x00007ffff714dd95 in PrepareAndDispatch (self=0x7fffd09edb00, 
    methodIndex=3, args=0x7fffffffb540, gpregs=0x7fffffffb4c0, 
    fpregs=0x7fffffffb4f0)
    at ../../../../../../../mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:153
#9  0x00007ffff714ce4b in SharedStub ()
   from /home/jst/work/tip/fb-dbg/dist/bin/libxpcom_core.so
#10 0x00007ffff7122af1 in nsComponentManagerImpl::CreateInstanceByContractID (
    this=0x7fffef489520, 
    aContractID=0x7fffdd83f3b0 "@mozilla.org/geolocation/provider;1", 
    aDelegate=0x0, aIID=@0x7fffdd83f4e0, aResult=0x7fffffffb630)
    at ../../../mozilla/xpcom/components/nsComponentManager.cpp:1684
#11 0x00007ffff71248b4 in nsComponentManagerImpl::GetServiceByContractID (
    this=0x7fffef489520, 
    aContractID=0x7fffdd83f3b0 "@mozilla.org/geolocation/provider;1", 
    aIID=@0x7fffdd83f4e0, result=0x7fffffffb748)
    at ../../../mozilla/xpcom/components/nsComponentManager.cpp:2251
#12 0x00007ffff70b47ac in CallGetService (
    aContractID=0x7fffdd83f3b0 "@mozilla.org/geolocation/provider;1", 
    aIID=@0x7fffdd83f4e0, aResult=0x7fffffffb748)
    at nsComponentManagerUtils.cpp:94
#13 0x00007ffff70b4840 in nsGetServiceByContractID::operator() (
    this=0x7fffffffb730, aIID=@0x7fffdd83f4e0, aInstancePtr=0x7fffffffb748)
    at nsComponentManagerUtils.cpp:278
#14 0x00007fffdd46066b in nsCOMPtr<nsIGeolocationProvider>::assign_from_gs_contractid (this=0x7fffd0d4b090, gs=
      {mContractID = 0x7fffdd83f3b0 "@mozilla.org/geolocation/provider;1"}, 
    aIID=@0x7fffdd83f4e0) at ../../../dist/include/nsCOMPtr.h:1229
#15 0x00007fffdd4606ba in nsCOMPtr<nsIGeolocationProvider>::operator= (
    this=0x7fffd0d4b090, rhs=
      {mContractID = 0x7fffdd83f3b0 "@mozilla.org/geolocation/provider;1"})
    at ../../../dist/include/nsCOMPtr.h:690
#16 0x00007fffdd45dced in nsGeolocationService (this=0x7fffd0d4b060)
    at ../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp:378
#17 0x00007fffdd45dd2e in nsGeolocationService::GetInstance ()
    at ../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp:521
#18 0x00007fffdd45e081 in nsGeolocation (this=0x7fffef540fb0, 
    aContentDom=0x7fffda021400)
    at ../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp:590
#19 0x00007fffdd3d1597 in nsNavigator::GetGeolocation (this=0x7fffef540ec0, 
    _retval=0x7fffffffbab0)
    at ../../../mozilla/dom/base/nsGlobalWindow.cpp:9591
#20 0x00007ffff714cdb7 in NS_InvokeByIndex_P (that=0x7fffef540ed8, 
    methodIndex=3, paramCount=1, params=0x7fffffffbab0)
    at ../../../../../../../mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:208
#21 0x00007fffe42be382 in XPCWrappedNative::CallMethod (ccx=@0x7fffffffbf60, 
    mode=XPCWrappedNative::CALL_GETTER)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2491
#22 0x00007fffe42cf5c3 in XPCWrappedNative::GetAttribute (ccx=@0x7fffffffbf60)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcprivate.h:2316
#23 0x00007fffe42ca4be in XPC_WN_GetterSetter (cx=0x7fffda021800, 
    obj=0x7fffd0d4a040, argc=0, argv=0x7fffd0fa3078, vp=0x7fffffffc130)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1622
#24 0x00007ffff7682cd8 in js_Invoke (cx=0x7fffda021800, argc=0, 
    vp=0x7fffd0fa3068, flags=2) at ../../../mozilla/js/src/jsinterp.cpp:1385
#25 0x00007ffff76835ca in js_InternalInvoke (cx=0x7fffda021800, 
    obj=0x7fffd0d4a040, fval=140736696984128, flags=0, argc=0, argv=0x0, 
    rval=0x7fffffffcc28) at ../../../mozilla/js/src/jsinterp.cpp:1446
#26 0x00007ffff7683850 in js_InternalGetOrSet (cx=0x7fffda021800, 
    obj=0x7fffd0d4a040, id=140736993597332, fval=140736696984128, 
    mode=JSACC_READ, argc=0, argv=0x0, rval=0x7fffffffcc28)
    at ../../../mozilla/js/src/jsinterp.cpp:1509
#27 0x00007ffff769629b in js_GetSprop (cx=0x7fffda021800, 
    sprop=0x7fffd0904e80, obj=0x7fffd0d4a040, vp=0x7fffffffcc28)
    at ../../../mozilla/js/src/jsscope.h:368
#28 0x00007ffff7698165 in js_NativeGet (cx=0x7fffda021800, obj=0x7fffd0d4a040, 
    pobj=0x7fffd0d4a000, sprop=0x7fffd0904e80, vp=0x7fffffffcc28)
    at ../../../mozilla/js/src/jsobj.cpp:4164
#29 0x00007ffff769b02b in js_GetPropertyHelper (cx=0x7fffda021800, 
    obj=0x7fffd0d4a040, id=140736993597332, cacheResult=1, vp=0x7fffffffcc28)
    at ../../../mozilla/js/src/jsobj.cpp:4330
#30 0x00007ffff7669e97 in js_Interpret (cx=0x7fffda021800)
    at ../../../mozilla/js/src/jsinterp.cpp:4440
#31 0x00007ffff7681f56 in js_Execute (cx=0x7fffda021800, chain=0x7fffd0ff17c0, 
    script=0x7fffd09fb780, down=0x0, flags=0, result=0x0)
    at ../../../mozilla/js/src/jsinterp.cpp:1621
#32 0x00007ffff75f5fac in JS_EvaluateUCScriptForPrincipals (cx=0x7fffda021800, 
    obj=0x7fffd0ff17c0, principals=0x7fffd08d87d8, chars=0x7fffd09f2008, 
    length=456, 
    filename=0x7fffd82aca08 "http://localhost:8888/tests/dom/tests/mochitest/geolocation/test_allowCurrent.html", lineno=21, rval=0x0)
    at ../../../mozilla/js/src/jsapi.cpp:5155
...

the subject principal in the security check is that of the testcase (localhost:8888 etc). That's as far as I got, time to get some sleep.
Geolocation provider is a JS component, no?  I'm surprised that ever worked; see the issues we had with the URI JS impl.  Basically, any JS component that can be called into from C++ with content JS on the stack will, generally, malfunction.  At least until we fix xpconnect?
the provider, written in js, basically listens to changes from c++ (wifi access point changes).  when there is a change, it creates a network connection.  when the network connection returns data, we create a js object and pass it back to c++ which then returns the js object to content.

The js objects that get passed back into content are marked as DOM_OBJECTS.  Maybe that gives me the right xpconnect fu?

http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/NetworkGeolocationProvider.js#99
(Assignee)

Comment 29

9 years ago
Created attachment 378906 [details] [diff] [review]
Updated patch

I didn't add code to unwrap COWs when they're passed into C++ because I forgot about UniversalXPConnect. Note that it is important that we do a CanAccessWrapper style security check to prevent someone from passing a COW-wrapped ChromeWindow to a treewalker or other useful C++-implemented API.

Here's an interdiff:
diff --git a/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp b/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp
--- a/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp
+++ b/js/src/xpconnect/src/XPCChromeObjectWrapper.cpp
@@ -158,31 +158,17 @@ GetWrapper(JSObject *obj)
   return obj;
 }
 
 // TODO Templatize, move to XPCWrapper and share with other wrappers!
 static inline
 JSObject *
 GetWrappedObject(JSContext *cx, JSObject *wrapper)
 {
-  if (STOBJ_GET_CLASS(wrapper) != &sXPC_COW_JSClass.base) {
-    return nsnull;
-  }
-
-  jsval v;
-  if (!JS_GetReservedSlot(cx, wrapper, XPCWrapper::sWrappedObjSlot, &v)) {
-    JS_ClearPendingException(cx);
-    return nsnull;
-  }
-
-  if (!JSVAL_IS_OBJECT(v)) {
-    return nsnull;
-  }
-
-  return JSVAL_TO_OBJECT(v);
+  return XPCWrapper::UnwrapGeneric(cx, &sXPC_COW_JSClass, wrapper);
 }
 
 // Forward declaration for the function wrapper.
 JSBool
 XPC_COW_RewrapForChrome(JSContext *cx, JSObject *wrapperObj, jsval *vp);
 JSBool
 XPC_COW_RewrapForContent(JSContext *cx, JSObject *wrapperObj, jsval *vp);
 
@@ -209,42 +195,23 @@ XPC_COW_FunctionWrapper(JSContext *cx, J
 
   JSObject *funObj = JSVAL_TO_OBJECT(argv[-2]);
   jsval funToCall;
   if (!JS_GetReservedSlot(cx, funObj, XPCWrapper::eWrappedFunctionSlot,
                           &funToCall)) {
     return JS_FALSE;
   }
 
-  JSFunction *fun = JS_ValueToFunction(cx, funToCall);
-  if (!fun) {
-    return ThrowException(NS_ERROR_ILLEGAL_VALUE, cx);
-  }
-
   for (uintN i = 0; i < argc; ++i) {
     if (!XPC_COW_RewrapForChrome(cx, obj, &argv[i])) {
       return JS_FALSE;
     }
   }
 
-  XPCCallContext ccx(JS_CALLER, cx);
-  if (!ccx.IsValid()) {
-    return ThrowException(NS_ERROR_FAILURE, cx);
-  }
-
-  JSNative native = JS_GetFunctionNative(cx, fun);
-  NS_ASSERTION(native, "How'd we get here with a scripted function?");
-
-  // A trick! Calling the native directly doesn't push the native onto the
-  // JS stack, so interested onlookers will only see us, meaning that they
-  // will compute *our* subject principal.
-
-  argv[-2] = funToCall;
-  argv[-1] = OBJECT_TO_JSVAL(wrappedObj);
-  if (!native(cx, wrappedObj, argc, argv, rval)) {
+  if (!JS_CallFunctionValue(cx, wrappedObj, funToCall, argc, argv, rval)) {
     return JS_FALSE;
   }
 
   return XPC_COW_RewrapForContent(cx, obj, rval);
 }
 
 JSBool
 XPC_COW_WrapFunction(JSContext *cx, JSObject *outerObj, JSObject *funobj,
diff --git a/js/src/xpconnect/src/XPCWrapper.cpp b/js/src/xpconnect/src/XPCWrapper.cpp
--- a/js/src/xpconnect/src/XPCWrapper.cpp
+++ b/js/src/xpconnect/src/XPCWrapper.cpp
@@ -89,16 +89,19 @@ XPCWrapper::Unwrap(JSContext *cx, JSObje
     }
 
     return wrappedObj;
   }
 
   if (clasp == &sXPC_SOW_JSClass.base) {
     return UnwrapSOW(cx, wrapper);
   }
+  if (clasp == &sXPC_COW_JSClass.base) {
+    return UnwrapCOW(cx, wrapper);
+  }
 
   return nsnull;
 }
 
 static void
 IteratorFinalize(JSContext *cx, JSObject *obj)
 {
   jsval v;
diff --git a/js/src/xpconnect/src/XPCWrapper.h b/js/src/xpconnect/src/XPCWrapper.h
--- a/js/src/xpconnect/src/XPCWrapper.h
+++ b/js/src/xpconnect/src/XPCWrapper.h
@@ -281,16 +281,31 @@ public:
     if (NS_FAILED(rv)) {
       JS_ClearPendingException(cx);
       wrapper = nsnull;
     }
 
     return wrapper;
   }
 
+  static JSObject *UnwrapCOW(JSContext *cx, JSObject *wrapper) {
+    wrapper = UnwrapGeneric(cx, &sXPC_COW_JSClass, wrapper);
+    if (!wrapper) {
+      return nsnull;
+    }
+
+    nsresult rv = CanAccessWrapper(cx, wrapper);
+    if (NS_FAILED(rv)) {
+      JS_ClearPendingException(cx);
+      wrapper = nsnull;
+    }
+
+    return wrapper;
+  }
+
   /**
    * Rewraps a property if it needs to be rewrapped. Used by
    * GetOrSetNativeProperty to rewrap the return value.
    */
   static JSBool RewrapIfDeepWrapper(JSContext *cx, JSObject *obj, jsval v,
                                     jsval *rval, JSBool isNativeWrapper) {
     *rval = v;
     return isNativeWrapper
Attachment #378695 - Attachment is obsolete: true
Attachment #378906 - Flags: superreview?(jst)
Attachment #378906 - Flags: review?(bzbarsky)
(Assignee)

Comment 30

9 years ago
Created attachment 378921 [details] [diff] [review]
Really updated patch

I think I've caught gal's "unable to attach patches" disease.
Attachment #378906 - Attachment is obsolete: true
Attachment #378906 - Flags: superreview?(jst)
Attachment #378906 - Flags: review?(bzbarsky)
Comment on attachment 378921 [details] [diff] [review]
Really updated patch

r+ based on that interdiff

Updated

9 years ago
Attachment #378921 - Flags: superreview+
Any reason why we're not putting this on mozilla-central and then mozilla-1.9.1 in a fashion that approximates expedience?
After several conversations about this issue with JST and bkap, it seems there are no known issues that actually exploit what this bug is hoping to protect us from, correct?  If that's the case, and this is a complicate patch that touches a lot of different things (JS), and isn't properly baked to the point where we're reasonably certain it's not going to break the RC, I'm not sure we should actually do this.  

bkap and jst are now at the point were they think we shouldn't block.  I'm inclined to go with that.  Objections?
(Assignee)

Comment 34

9 years ago
(In reply to comment #33)
> we're reasonably certain it's not going to break the RC, I'm not sure we should
> actually do this.  

To be clear. I still think that COWs should happen. It's just that after they missed beta, pushing them into the RC is really scary. I think everybody would feel better if we get them in on trunk and then into the first dot release after they've been tested sufficiently.

For what it's worth, this is supposed to be a compatible change, modulo any security bugs it fixes, so it should be OK to put on branches.

Comment 35

9 years ago
I'm just offering you the perspective of a potential consumer of this feature.
As far as I understand this innovation, it would allow us to erase a bunch of very ugly code that implements Firebug's console and commandline. But because the current solution works (mostly) and we have other higher priorities, I have that on our plan for 1.6, Jun 2010. (1.5 we hope for Nov.) I do think this is a great direction, but I understand the issues.
blocking1.9.1- per earlier comments and discussions.
Flags: wanted1.9.1.x?
Flags: wanted1.9.1+
Flags: wanted-next+
Flags: blocking1.9.2+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Flags: wanted1.9.1.x? → wanted1.9.1.x+
(Assignee)

Updated

9 years ago
No longer blocks: 479560

Comment 37

9 years ago
Comment on attachment 378921 [details] [diff] [review]
Really updated patch

In XPC_COW_toString():

>+  if (!wrappedObj) {
>+    // Someone's calling toString on our prototype.
>+    NS_NAMED_LITERAL_CSTRING(protoString, "[object XPCCrossOriginWrapper]");

String literal typo: "XPCCrossOriginWrapper" should be "ChromeObjectWrapper".

Also, at the very end of the same function:

>+  XPCWrappedNative *wn =
>+    XPCWrappedNative::GetWrappedNativeOfJSObject(cx, wrappedObj);
>+  return XPCWrapper::NativeToString(cx, wn, argc, argv, rval, JS_FALSE);
>+}

The object being wrapped isn't necessarily a wrapped native; in my case when testing, it was just a normal JS object, and the process segfaulted when I called its toString() method.

Comment 38

9 years ago
Nominating for .1, though with a quick turnaround and no trunk baking yet that may not be feasible.
Flags: blocking1.9.1.1?
(In reply to comment #38)
> Nominating for .1, though with a quick turnaround and no trunk baking yet that
> may not be feasible.

It's not, but we'll definitely consider it for 1.9.1.2 or later.
Flags: blocking1.9.1.1? → blocking1.9.1.1-
(Assignee)

Comment 40

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2cdf489cadce
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
status1.9.1: --- → wanted
Flags: wanted1.9.1.x+
Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)
Keywords: fixed1.9.2
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.2
blocking1.9.1: --- → needed
Are we ready to land COWs on the 1.9.1 branch?
blocking1.9.1: needed → .5+
Apparently not.
blocking1.9.1: .6+ → needed
At this point do we have any plans to land COWs on the 1.9.1 branch, or should that be left as a 3.6-and-later protection? Have to draw the line somewhere.
blocking1.9.1: needed → ?
(Assignee)

Updated

9 years ago
Depends on: 550356
Taking this off the blocking list for now until we get clarity on the question in comment 44.
blocking1.9.1: ? → -
Group: core-security
You need to log in before you can comment on or make changes to this bug.