604196 - jsval return types in xpidl are broken (non-quickstubs)

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 1

[Luke Wagner [:luke]]

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.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 3

[Luke Wagner [:luke]]

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

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch that can reproduce this

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.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix

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.

Comment 8

[Jason Orendorff [:jorendorff]]

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

[Jason Orendorff [:jorendorff]]

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.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/d3790fe503e5

Comment 11

[Boris Zbarsky [:bzbarsky]]

This caused bug 604196

Comment 12

[Boris Zbarsky [:bzbarsky]]

Er, I meant this caused bug 655878.