Using a jsval return type in xpidl causes crashynessin XPCConvert::NativeData2JS, because the dereference is **((jsval**)s). Luke tells me this is correct, and that the bug lies in http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#1545

To be a bit more specific, its a disconnect between the code linked by comment 0 and the T_JSVAL case in NativeData2JS. This already happened once in JSData2Native (bug 589329 comment 7) and we chose to fix it by having the T_JSVAL case use either one or two levels of indirection: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcconvert.cpp#630 However, here there is no analogous parameter, so we can fix it in the caller by not stripping the indirection for in/out params that are also jsvals. We could even go back and fix JSData2Native in the same way which seems more consistent.

I'm lost in the above explanation, tbh; I tried the suggested fix on irc at xpcwrappedjsclass.cpp:1545, but it didn't work; reading the code, this is inside an in-param block. The jsval in question here is a pure out param, so we'll never go through this path. This is also inside nsXPCWrappedJSClass, which seems like it's the wrong direction -- I'm making a call from JS to a C++ component, so there's no wrapped JS class.

Sorry, didn't pay attention to the cause, just looked for the same cause as before. comment 1 may actually be a bug though...

This work-in-progress patch can reproduce this bug -- note that it has the workaround that changes **((jsval**)s) -> *((jsval*)s), which seems to work. Remove the workaround to see the original bug, which you can reproduce by opening mozilla-central/content/canvas/test/webgl/conformance/context-attributes-alpha-depth-stencil-antialias.html . The function in question is getContextAttributes, on CanvasRenderingContextWebGL.

Assigning this to Luke, let me know if that's not correct...

Here's my understanding of what happens with jsval, in sort of pseudocode, gleaned from xpc code, looking at xpcwrappednative only: ConvertIndependentParams converts the incoming params for making a native call. The jsval bits look roughly like this: useAllocator = false; if (isout) { dp->SetPtrIsData(); if (is_jsval) { if (is_retval) { dp->ptr = mCallContext.GetRetVal(); } else { dp->ptr = (jsval*) &dp->val.u64 call dp->SetValIsJSRoot() } } if (!isin) continue; } else { if (is_jsval) { dp->SetValIsallocated(); useAllocator = true; } } // actually call JSData2Native for in or inout params JSData2Native(&dp->val, src, type, useAllocator) in JSData2Native, the jsval case is: if useAllocator: *((jsval**)(&dp->val)) = new jsval(s) else: *((jsval*)(dp->val)) = s; From this I get a few things: - For in params, dp->val ends up holding a jsval* unconditionally; useAllocator is set to true, so JSData2Native will allocate a jsval. - For pure out params, dp->ptr will hold either the direct return jsval pointer, or will hold a pointer just reusing &dp->val.u64 space, and rooting it. - For return params, dp->val doesn't seem to be valid - For non-return params, dp->val is a jsval - For *in out* params, we'll treat dp->val as a jsval that we'll assign the incoming jsval to, and dp->ptr will be set correctly to its pointer Or, put another way: - retval: dp->ptr = jsval* dp->val = ignored/invalid - out/inout: dp->val = bare jsval (rooted) dp->ptr = &dp->val - in: dp->val = jsval* dp->ptr = ignored/invalid The problems I think are on the other end; GatherAndConvertResults calls NativeData2JS (again, omitting anything but what we'd hit for T_JSVAL): if (!isout) continue; // make the call to convert, passing in only &dp->val NativeData2JS(&v, &dp->val, T_JSVAL); if (isretval) { if (!cc.return_value_was_set && type != T_JSVAL) { cc.SetRetVal(v); } } and in NativeData2JS, for T_JSVAL: NativeData2JS(d = &v, s = &dp->val, type) *d = **((jsval**)s); First - why do we even call NativeData2JS if the param is a retval? Isn't dp->val bogus in that case, presumably the direct value of dp->ptr was passed to the call (because SetPtrIsData)? I don't see where it was set to anything valid on the incoming side of things. Second - For retval out params, dp->val is a bare jsval, and NativeData2JS gets &dp->val as the 's' arg. So, having it do |d = **((jsval**)s)| is incorrect -- s is just a jsval*, and will always be just a jsval*, never a **. For it to be a jsval**, that would mean that dp->val would be a jsval*, which doesn't happen for the out param case.

Based on the above, I believe this patch is correct. I removed the assert because this code doesn't directly depend on sizeof(jsval) <= sizeof(uint64) -- the code that actually makes that assumption has the assert there.

(In reply to comment #6) > Or, put another way: > - retval: > dp->ptr = jsval* > dp->val = ignored/invalid > - out/inout: > dp->val = bare jsval (rooted) > dp->ptr = &dp->val > - in: > dp->val = jsval* > dp->ptr = ignored/invalid Great. Thanks! > First - why do we even call NativeData2JS if the param is a retval? Because it's simpler to call it than to have yet another special case where it doesn't get called. I'm opposed to adding another if statement to this code unless there's a really really good reason. That NativeData2JS call is one of the few things in here that I'm totally OK with. ;)

Comment on attachment 488079 [details] [diff] [review] fix Sorry for the slow review. I had to stare at a lot of code before this made sense to me.

This caused bug 604196

Er, I meant this caused bug 655878.