xptcall assumes sizeof(jsval) == sizeof(void*)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: jorendorff)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Bug 560643 added jsval to XPCOM interfaces.  However, deep in xptcall, jsval  parameters are copied assuming that they are word-sized.  Since, with fatvals, jsval is always 64-bit, this slices on 32-bit.  Ideally, the C++ parameter type would be const jsval & to avoid depending on the way the ABI passes struct types (jsval and jsid are structs in debug builds to catch insidious implicit conversions).
(Assignee)

Comment 1

7 years ago
Lots of concerns:
  - depending on C++ ABI details
  - cost of fixing it again if we change the layout of jsval again
  - doing whatever's fastest to implement
  - making calls fast at run time
  - being kind to users

Looks like a pretty even split, with items 1 and 2 favoring "const jsval &" as the C++ parameter type, items 3 and probably 5 favoring plain "jsval", and
item 4 a toss-up.  :-\
(Assignee)

Comment 2

7 years ago
OK, I've convinced myself that Luke's right! But I need shaver's help to finish this.
(Assignee)

Comment 3

7 years ago
Tried making all [ref] parameters work this way, such that

  native [ref] foo(foo);

makes in-params `const foo &` and out-params `foo *`. But at least some existing types aren't supposed to behave that way, so I narrowed the hack to operate only on jsval. Building now.
(Assignee)

Comment 4

7 years ago
Created attachment 457455 [details] [diff] [review]
WIP 1 - don't expect too much

Luke wants to take this over, I think.

The main change this makes is:
 - for in parameters, IDL jsval maps to C++ const jsval &
 - for out parameters, IDL jsval maps to C++ jsval *

I'm not sure I like it. The alternative is to split jsval into two types, one scriptable (which maps to C++ const jsval &) and one not (which maps to plain jsval). That would touch less code, but I'm not sure the grass is really greener.
(Assignee)

Comment 5

7 years ago
Wait - this is also missing some code in xpcwrappednative.cpp. And gavin thinks it wouldn't hurt to bump the IIDs of any interfaces we touch (note that fatvals will break binary compatibility for all of them whether we split out a plain jsval type or not).
(Assignee)

Comment 6

7 years ago
Created attachment 457465 [details] [diff] [review]
WIP 2 - still not final

I noticed that in xpcwrappednative.cpp, CallMethodHelper::ConvertIndependentParams assumes the target buffer is large enough to hold a jsval. It is, even with fatvals, fortunately.

This patch might actually work. Honestly I wouldn't count on it though.
Attachment #457455 - Attachment is obsolete: true
(Assignee)

Comment 7

7 years ago
I think you might also want this. Not sure why it wasn't there already, though.

diff -u b/js/src/xpconnect/src/xpcwrappednative.cpp b/js/src/xpconnect/src/xpcwrappednative.cpp
--- b/js/src/xpconnect/src/xpcwrappednative.cpp
+++ b/js/src/xpconnect/src/xpcwrappednative.cpp
@@ -2799,6 +2799,7 @@
                     *rootp = JSVAL_VOID;
                     if (!JS_AddValueRoot(mCallContext, rootp))
                         return JS_FALSE;
+                    dp->SetValIsJSRoot();
                 }
             }
(Reporter)

Comment 8

7 years ago
Hah! I was just about to post and ask why mxr showed no SetValIsJSRoot calls in the tree.
(Reporter)

Comment 9

7 years ago
Oh yeah, and this seems necessary:

diff --git a/js/src/xpconnect/src/xpcwrappednative.cpp b/js/src/xpconnect/src/xpcwrappednative.cpp
--- a/js/src/xpconnect/src/xpcwrappednative.cpp
+++ b/js/src/xpconnect/src/xpcwrappednative.cpp
-            else if(dp->IsValAllocated())
-                nsMemory::Free(p);
-            else if(dp->IsValInterface())
-                ((nsISupports*)p)->Release();
-            else if(dp->IsValDOMString())
-                mCallContext.DeleteString((nsAString*)p);
-            else if(dp->IsValUTF8String())
-                delete (nsCString*) p;
-            else if(dp->IsValCString())
-                delete (nsCString*) p;
-            else if(dp->IsValJSRoot())
-                JS_RemoveValueRoot(mCallContext, (jsval*)dp->ptr);
+            else
+            {
+                if(dp->IsValJSRoot())
+                    JS_RemoveValueRoot(mCallContext, (jsval*)dp->ptr);
+
+                if(dp->IsValAllocated())
+                    nsMemory::Free(p);
+                else if(dp->IsValInterface())
+                    ((nsISupports*)p)->Release();
+                else if(dp->IsValDOMString())
+                    mCallContext.DeleteString((nsAString*)p);
+                else if(dp->IsValUTF8String())
+                    delete (nsCString*) p;
+                else if(dp->IsValCString())
+                    delete (nsCString*) p;
+            }
(Reporter)

Comment 10

7 years ago
Cool, well with the changes to xpc and xpidl you've made, I can watch it successfully pass a value and free stuff on the way out.  Since most of the rest of the changes in the above patches involve jsvals being used as jsids, which get changed to real jsids with fatvals, I'm going to roll this patch into fatvals so fatvals can land.  Thank you so much for your help!
(Reporter)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 585232
You need to log in before you can comment on or make changes to this bug.