655878 - Using a jsval for an IDL interface is broken

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Anant Narayanan [:anant]]

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.

Comment 1

[Luke Wagner [:luke]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Alternately, a regression range?

Comment 3

[Luke Wagner [:luke]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Anant Narayanan [:anant]]

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

Comment 6

[Anant Narayanan [:anant]]

Attachment: Generated header for IDL

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Luke Wagner [:luke]]

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

Comment 9

[Anant Narayanan [:anant]]

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/[e-mail]/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!

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Jason Orendorff [:jorendorff]]

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.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Testcase extension

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.

Comment 16

[[:fabrice] Fabrice Desré]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[[:fabrice] Fabrice Desré]

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

of course Boris, /me facepalm

Comment 19

[Bobby Holley (:bholley)]

I'll look into this in a few days.

Comment 20

[Luke Wagner [:luke]]

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

Comment 21

[Bobby Holley (:bholley)]

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

Comment 22

[Bobby Holley (:bholley)]

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