We tried to add new interface: interface nsIDOMOpenWebapp : nsISupports { boolean isInstalled(in jsval manifest); }; The argument is a jsval, but the component that implements the interface (in JS) does not receive the correct value. A (seemingly) arbitrary float value is received instead. The reverse -- i.e. sending a jsval from a component via a callback is also broken. It seems like this used to work before the mozilla-central merge with mobile, and is a regression.

Interesting. Do you suppose you could give us a minimal test case that we can use to debug?

Alternately, a regression range?

Its most likely fatvals (9c869e64ee26). The testcase is useful since Anant seems to be hitting one (of the like 16) paths through xpcconvert that apparently hasn't been taken yet. Different paths like to do fun things like change the level of indirection of the void* being converted which is probably what is producing the weird double.

Hmm. So this is a JS-implemented thing taking jsval. The relevant part of XPCConvert::NativeData2JS is: 299 case nsXPTType::T_JSVAL : 300 { 301 *d = *((jsval*)s); 302 if(!JS_WrapValue(cx, d)) 303 return JS_FALSE; 304 break; 305 } The caller in xpcwrappedjsclass looks like this: 1555 if(!XPCConvert::NativeData2JS(ccx, &val, &pv->val, type, 1556 &param_iid, nsnull)) where |&val| is passed in as |d| and |&pv->val| is passed in as |s|. |val| is a javal. |pv| is an nsXPTCMiniVariant* set like so: pv = &nativeParams[i]; pv->val is a union of all sorts of things. We're passing a pointer to this union, then treating it as a pointer to jsval. The pointer is actually a pointer to nativeParams[i]; nativeParams is an array of nsXPTCMiniVariant. So the question is how nativeParams is set up... That seems to be happening in xptcall land. For non-arithmetic types (and that includes jsval), we seem to set dispatchParams[i].val.p = (void*)args[i]. So that all matches up as far as I can tell. Anant, the callee here is JS-implemented, right? Is the caller JS or C++? Can you attach the generated C++ header file for your idl file?

(In reply to comment #4) > Anant, the callee here is JS-implemented, right? Is the caller JS or C++? > Can you attach the generated C++ header file for your idl file? The caller and callee are both JS. I will attach the generated header file and whip up a test case.

The amInstalled() method in the generated file takes an nsIOpenWebAppsCallback, which I experimented with because the earlier jsval approach was not working. nsIOpenWebAppsCallback only has one method 'completed' which takes a jsval, but the callback approach did not work either.

OK. So from the point of view of the C++, the jsval is being passed by reference.... I wonder what xptcall ends up seeing for that. Luke, do you want to try stepping through that?

You bet (especially if that test case comes along soon; my JS-xpcom-component knowledge is shallow).

I tried to make a patch & testcase to mozilla-central to illustrate the problem, but could not get it to work :( So instead I made a small add-on: http://proness.kix.in/misc/jsval.xpi Install the add-on, restart, open up chrome://test_jsval/content/test.html in a new tab. Then, open the error console and there should be something similar to: "Error: WebApp: Recieved: 2.313796035e-314 Source File: file:///Users/anant/Library/Application%20Support/Firefox/Profiles/ae4wcj4n.default/extensions/test_jsval@labs.mozilla/components/OpenWebApps.js Line: 8" As you can see, I get a float value and not the actual JS object that I pass in test.html. Let me know if there's anything I can do to help fix this!

OK, a bit more data. First of all, Anant put a breakpoint in NativeData2JS and it looks like *(jsval*)s is not very useful but **(jsval**)s is the right thing in his testing. His callstack looks like this (some frames elided): #0 XPCConvert::NativeData2JS .. #2 0x0000000100f7a7c8 in nsXPCWrappedJSClass::CallMethod .. #4 0x00000001016cd876 in PrepareAndDispatch #5 0x00000001016cc2eb in SharedStub #6 0x00000001016cc23d in NS_InvokeByIndex_P ... #9 0x0000000100f874c8 in XPCWrappedNative::CallMethod which is exactly what I would expect from a double-wrapped situation: a call from JS going through XPCWrappedNative and then the xptcall stuff out to the XPCWrappedJS stuff. Now CallMethodHelper::ConvertIndependentParams (which I believe is the argument conversion in XPCWrappedNative code before it calls what it thinks is the C++ method) for the case of an in jsval arg does: 2956 if(type_tag == nsXPTType::T_JSVAL) { 2957 dp->SetValIsAllocated(); 2958 useAllocator = JS_TRUE; 2959 } and JSData2Native does for jsval: 666 if (useAllocator) { 667 // The C++ type is (const jsval &), which here means (jsval *). 668 jsval *buf = new jsval(s); 669 if(!buf) 670 return JS_FALSE; 671 *((jsval**)d) = buf; 672 } else { 673 *((jsval*)d) = s; 674 } (incidentally, I think it will then proceed to free the memory via nsMemory::Free if I read CallMethodHelper::~CallMethodHelper correctly, so the |new| thing there is kinda bogus too). So at least the path from JS into C++ is trying to put a jsval*, effectively, on the native stack. That matches the |jsval&| signature in the attached header. On the other hand, the *(jsval*)s thing assumes that the stack has a jsval directly on it, which is why things fail. If we can assume that actual calls from C++ will behave just like the XPCWrappedNative call (I suspect a good assumption), then NativeData2JS should be using **(jsval**)s, I think.

Oh, and I bet the !useAllocator path in JSData2Native is never reached.

OK, so this _used_ to be a jsval** until the fix for bug 604196.

So. jsval in params are |const jsval&|. jsval return values are |jsval*|. NativeData2JS is used for both conversions (jsval in param to an XPCWrappedJS and jsval return value). The in param conversion call is: XPCConvert::NativeData2JS(ccx, &val, &pv->val, type, &param_iid, nsnull)) where pv is an nsXPTCMiniVariant. Since in params are jsval& the value in pv->val is a jsval*. This is all described in bug 604196 comment 6. The key problem is that for both the in param and the out case we pass in &dp->val but the type stored in dp->val is _different_ in those cases. _That_ is what needed to be fixed in bug 604196. For strings this is handled by storing the same thing in the union there in all cases: an nsAString*. This corresponds to the |const nsAString&| and |nsAString&| C++ types. We heap-allocate them as needed to make this true. The result is that when passing a pointer to the union we always pass nsAString**. For string out params we always force allocation to make this work For jsvals, however, we do NOT force allocation for the out param case. So we end up with with out params that are jsval instead of jsval*, which is why the resulting NativeDataToJS needs to assume the union is storing a jsval directly.... or something. Long story short: we should make jsval work just like strings do, since that's a solved problem that has the same signatures we want here.

Thanks for sorting this out, bz. Bug 604196 gave me a funny tingly feeling I was never able to put my finger on, and all this sounds just right.

So next time I should search for the bug first and then create the testcase. Anyway, if its of any use here's the testcase extension I created. I would appreciate this bug being fixed, it would ease the introduction of js-ctypes for the Calendar Project.

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=540bdb31642a is a try build with a simple patch using **(jsval**) in NativeData2JS. This allows the jsval to be sent back to js correctly. Looks like this makes lots of tests to fail but I'm having a hard time relating the failures to this patch.

Well, if nothing else that would regress bug 604196, no?

(In reply to Boris Zbarsky (:bz) from comment #17) > Well, if nothing else that would regress bug 604196, no? of course Boris, /me facepalm

I'll look into this in a few days.

bholley: if you throw up in your mouth and feel like rewriting NativeData2JS and JSData2Native to be sane, that would be awesome... just sayin'

This is now fixed in my patches for bug 683802. The work over there is still ongoing, and is available as a branch on my github fork: https://github.com/bholley/mozilla-central/commits/xpcwn_refactor > Install the add-on, restart, open up chrome://test_jsval/content/test.html For reference, this should be: chrome://test-jsval-idl/content/test-jsval-idl.xul

Bug 683802 landed, which contains changeset http://hg.mozilla.org/mozilla-central/rev/e4248ea9a714 , which fixes this bug!