589329 - crash in [@ XPCConvert::JSData2Native]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Dahl :ddahl]

with new DOM interface patch and JS console component patch in bug 568629

I got this when running the tests like this: 
TEST_PATH=toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js make mochitest-browser-chrome

Comment 1

[David Dahl :ddahl]

Attachment: Backtrace

Comment 2

[Luke Wagner [:luke]]

I'll look into this.

Comment 3

[Luke Wagner [:luke]]

Does "with new DOM interface patch" refer to some patch I need to apply, or is this patch already committed?

Comment 4

[David Dahl :ddahl]

(In reply to comment #3)
> Does "with new DOM interface patch" refer to some patch I need to apply, or is
> this patch already committed?

No, I have applied this: https://bug568629.bugzilla.mozilla.org/attachment.cgi?id=463676

and this: https://bug568629.bugzilla.mozilla.org/attachment.cgi?id=466510

both of which are blocking beta 5.

All of the tests mentioned above passed before the last tm-merge.

Comment 5

[David Dahl :ddahl]

Attachment: Backtrace 2

So I rolled back my repo to the patch before the latest tm-merge and I get the same crash. crap.

New backtrace, but looks like the old one to me.

Comment 6

[David Dahl :ddahl]

Current changeset:
% hg parent 
changeset:   51134:b22e82ce2364
user:        Luke Wagner <[e-mail]>
date:        Thu Aug 19 18:02:17 2010 -0700
summary:     Bug 589015 - js_watch_set doesn't need that crazy dummy frame (r=mrbkap)

Comment 7

[Luke Wagner [:luke]]

Attachment: a fix

This is why the patch in bug 585232 was named "maybe fix" ;)  Looking at the value of d in the debugger, the "obvious" fix for JSData2Native is:

-  **((jsval**)d) = s;
+  *(jsval*)d = s;

This is bizarre, considering that all the JSData2Native callers are passing the address of the variant.  Or at least, it looks like they are, however, nsXPCWrappedJSClass::CallMethod() contains a special case where the *value* of the variant is cast as a pointer to the variant and passed to JSData2Native.  I believe the reason for this insanity is that it allows JSData2Native to use a single path for both inparams and outparams for primitive parameters like ints where the inparam type is T and the outparam type is T*.  jsval is an exception though: jsval* is the type of both inparams AND outparams.

The fix attached runs browser_ConsoleApiTests.js without error.  My question to the jst and mrbkap is: is the fix to make T_JSVAL a "dipper" type or is it the one attached?  What I don't like about the fix attached is that it is using useAllocator to choose whether d is a jsval** or jsval*, which does not seem to be equivalent to the real question: did the caller pass the address of the variant or the variant's value?

Comment 8

[Dietrich Ayala (:dietrich)]

Blocking a blocker for b5, so marking blocking+.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 467990 [details] [diff] [review]
a fix

Probably the right fix :)

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

--> beta6+ as per the other blocker it blocks

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 467990 [details] [diff] [review]
a fix

r=jst, and I filed bug 592150 as a followup to this where we can avoid the allocator overhead in some cases here.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 467990 [details] [diff] [review]
a fix

Updating attachment title to reflect the fact that this probably no the wrong fix :)

Comment 13

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/a55126a26fa2

Comment 14

[Jason Orendorff [:jorendorff]]

AIUI the deal is, if we're passing a jsval in from JS to a C++ method, we need
    *(jsval **)d = &some_jsval;
because the type of the in parameter is `const jsval &`.

But if we're passing a jsval out from a JS method to C++ caller, we need
    **(jsval **)d = some_jsval;
because the C++ type of the out parameter is `jsval *` and d points to the parameter itself.

So why does the patch say
    *(jsval *)d = some_jsval;
instead?

And is useAllocator really a solid indicator of whether we're going in or out? If so, can we just call it "in" or maybe "dir"?

Comment 15

[Luke Wagner [:luke]]

Because xpcwrappedjsclass.cpp strips off a level of indirection for non-dipper outparams.  See xpcwrappedjsclass.cpp:1757 and comment 7.

Comment 16

[Jason Orendorff [:jorendorff]]

Disregard the first question -- for out parameters d is the value of the out parameter, not its address.

The other two questions still worry me though.

Comment 17

[Luke Wagner [:luke]]

(In reply to comment #14)
> And is useAllocator really a solid indicator of whether we're going in or out?
> If so, can we just call it "in" or maybe "dir"?

I had the same concern in comment 7.  jst looked into it and came back with a solid 'probably' ;)  Which is to say, "its part of the caller contract".  I think it would really benefit the understandability of this code to split the conversion into several functions, for the different valid ways it may be called, with real parameter types which made it clear whether we had a ptr-to-variant, contents of variant, etc.

Comment 18

[Robert Sayre]

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