66610 - xpconnect needs support for nsA{Read,Writ}ableString

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[John Bandhauer]

In bug 66453 we added xpidl and typelib support for these new types and hacked 
xpconnect to gracefully ignore methods that use these types. Now we need to add 
real support for using these types.

Comment 1

[John Bandhauer]

Also note bug 50602 "xpidl/xpconnect needs support to share refcounted strings"

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Can I help here?  I am ready to implement this for Python, but would prefer a 
js implementation to be done before or in parallel.  Im happy to dig into 
xpconnect to try and do this at the same time.

Comment 3

[John Bandhauer]

Attachment: proposed fix. Needs more testing.

Comment 4

[John Bandhauer]

I attached a patch to get this working. It could use a lot more testing. What
with the nasty C casts on pointer in variants this code does I intend to give it
a try under Purify too.

Included is a very simple addition to the xpc echo tests to show this working.

I added checks to the xpidl compiler to make it illegal to use DOMStrings as
inout params and in arrays. I think inout of DOMString would be too ugly for
words. And array support is more work I'd like to put off for a bit.

I think the basic stuff is working. Better sharing of the underlying string data
will come later. Also, this does not support distinguishing 'null' from 'empty
string'. My understanding from jst is that nsA{Read,Writ}ableString will have
such a concept in the near future.

Comment 5

[John Bandhauer]

Attachment: newer patch fixes two problems jst saw

Comment 6

[John Bandhauer]

Two changes in the new patch:
- added missing #include to TestXPC.cpp
- relocated dereference of argv in wrappednativeclass.cpp because 'dippers' are 
marked in the typelib as 'in' params, but are not passed 'in' from JS in the 
retval case. That code assumed that only 'out' params could be retvals (and 
thus not required arguments). Thanks Johnny.

Comment 7

[Brendan Eich [:brendan]]

Ultra-stupid-nit:
+                           "[domstring] types can not be used as inout "
+                           "parameters");

the rest of xpidl uses "cannot" in its error messages.  One of the other, but
not both?

 case nsXPTType::T_DOMSTRING:
+        {
+            const PRUnichar* chars = nsnull;
+            PRUint32 length;
+            JSString* str;
+
+            if(!JSVAL_IS_VOID(s) && !JSVAL_IS_NULL(s) &&
+               nsnull != (str = JS_ValueToString(cx, s)))
+            {

The following code acts on the non-null return value in str, but swallows the
(possibly out of memory) error if JS_ValueToString returned null.

Also, should JSVAL_VOID (undefined) really turn into "", rather than into
"undefined" (as it does in JS)?

+            if(useAllocator)
+            {
+                nsAReadableString* rs = new nsLiteralString(chars, length);
+                if(!rs)
+                    return JS_FALSE;
+                *((nsAReadableString**)d) = rs;
+            }

Does nsLiteralString allocate its own copy?  If not, I don't see any GC root
holding onto str, so that the nsLiteralString's mStart and mEnd pointers won't
dangle should str be GC'd.  What am I missing?

+        if(param.IsDipper())
+            pv = (nsXPTCMiniVariant*) &nativeParams[i].val.p;

Shouldn't the right-hand side just be &nativeParams[i]?

+                        if(!(dp->val.p = new nsString()))
+                        {
+                            JS_ReportOutOfMemory(cx);
+                            goto done;
+                        }
+                        continue;
+                    }
+                    else

else after continue non-sequitur, wahhhhH!

/be

Comment 8

[Brendan Eich [:brendan]]

For that matter, should null convert to "" in a native DOMString, rather than
"null" (again as it does in JS)?  Maybe the DOM JS binding spec (hah!) says
something about this.

/be

Comment 9

[John Bandhauer]

brendan:

> "cannot"

(absolute stop-ship!) fixed

> JS_ValueToString returning null

What can I infer? Does returning null always mean out of memory? It looks like 
this can happen when OBJ_DEFAULT_VALUE fails for unknown cause. Are there no 
cases where returning null does not mean that I want to convert to a null string 
(whether represented as "", "null", or a special nsAReadableString::null?

I'm happy to fix this, but I'm not clea on all the cases.

> JSVAL_VOID (undefined)

I'm not clear on this either. In the 'string' and 'wstring' cases I map this to 
null and throw if the param is a C++ reference (that would not accept null).

> Does nsLiteralString allocate its own copy? 

That case is only used in 'in' params of nsAReadableString calling from JS to 
C++ (or whatever :). nsLiteralString does not copy or own the data. In this case 
the lifetime is that of the call being made and I was assuming that the string 
was rooted from being in argv. I see now that since we are potentially 
converting to a new JSString from some other type that this assumption is not 
safe. I'm going to want to stick the JSString* (as a jsval) into the slot in the 
argv to make this safe. This will requiring passing the jsval* rather than the 
jsval only to JSData2Native. I'll fix that.

> else after continue

another stop-ship. fixed.

> should null convert to "" in a native DOMString, rather than "null" ..

It seemed to me that jst mentioned bugs on this issue where it should *not* 
convert to "null". I *think* we want that magical nsAReadableString::isNull(). I 
could be wrong.

Thanks!

Comment 10

[John Bandhauer]

On the string rooting issue...

It can't always get at a rooted jsval address because of the way we use 
object.value to receive 'out' params. I'm thinking that the bast thing to do is 
have this conversion code conditionally make a copy of the string data (using 
new nsString) in the cases where we are not using the 'emptyString' literal and 
we did get a JSString* but it is not the same as the jsval passed in. I expect 
we'll mostly avoid the copy in normal code. This looks better than rooting to 
me.

Comment 11

[Brendan Eich [:brendan]]

>> JS_ValueToString returning null
>What can I infer?

The JS API never overloads a return value to make errors and non-errors
ambiguous.  Pointer-typed return values where the function takes a cx (is not a
pure accessor) use null to mean error, whether OOM or something else -- and the
error is reported (thrown, given errors as exceptions).

On the 'undefined' from undefined question: let's make it different from null,
unless a w3c spec says otherwise.  undefined == null but undefined !== null, and
it's useful to distinguish until proven harmful.  jst, any spec help?

On the rooting issue: I have a bug (bug 40757) that wants a JNI-like "local
root" model, which I believe would relieve you from having to worry about
rooting a conversion-created copy of a string.  Even now, the newborn will
protect such a string if it has no other refs.  But only until the next string
allocation overwrites the newborn pigeon-hole, and then the GC runs....  I agree
it's safest to make a copy iff JS_ValueToString returned a different string than
the one that came in.

/be

Comment 12

[John Bandhauer]

Attachment: another patch. w/ string copying, "null" and "undefined"

Comment 13

[John Bandhauer]

The latest patch addresses the stuff brendan brought up. I opted to convert to 
the literal strings "null" and "undefined" as appropriate. jst is going to have 
to say if it shouldn't work that way from his point of view. I expect that I 
should make similar changes for 'string' and 'wstring' type params if we decide 
this is the way to go.

Comment 14

[John Bandhauer]

BTW, Purify didn't find anything to complain about when running xpctest_echo.js 
using xpcshell.exe.

Comment 15

[Brendan Eich [:brendan]]

Minor point: I don't see a need for sEmptyString, sNullString, and sVoidString.
Just call JS_ValueToString and let the JS engine return its atomized well-known
strings, which are guaranteed not to be collectable.  Then you'd want to set
isNewString to false for the empty, null, and void cases.  Hmm, maybe this logic
is no simpler than what's there currently -- but the code would be smaller, and
you wouldn't need to duplicate JS's interned strings for "", "null", and
"undefined".  Whaddya think?

/be

Comment 16

[Johnny Stenback (:jst)]

So I've given the null vs. "null" vs. empty string case some thought and here's
what I think.

There's no question about what to do for string properties or methods that
return strings, we must be able to say in C++ code that a null string should be
returned and this should be converted into null in JS. I think we can do this
fairly easily once scc's strings can handle this.

But the setter/in parameter case is more complicated. If someone sets a string
property to null, say input.value = null, we want the input value to be "null",
same for alert(null), that should show "null" in the alert box. But in the
implementation of, say element.setAttributeNS(), I must be able to distinguish
between null, "" and "null" to be able to implement setAttributeNS by the DOM spec.

One possible way to solve the setter/in parameter case would be to use the same
idea that scc will use for passing null in nsAReadableString's, we could have a
helper method in XPConnect (or in the DOM) that always returns the "null" buffer
and XPConnect could wrap this "null" buffer in an nsLiteralString when passing
strings to the DOM code (i.e. this would work today, w/o scc's new string code)
and the DOM code could get the "null" buffer and compare against this buffer to
know if the string parameter is null, or "null".

The "null" buffer could potentially be the buffer from the JS engine, Brendan,
is the buffer returned from JS_GetStringChars(JS_ValueToString(null)) global or
per context? Is it accessable even if there's no JSContext around? Could PyXPCOM
also use that same buffer or do we need to define the "null" buffer in XPConnect
(or the DOM code)?

Does this sound at all reasonable or should I stop dreaming?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Excuse my ignorance here :-)  I see the problem with a NULL string being 
returned via a "dipper" param - ie, the caller has allocated an 
nsWritableString, so we can not simply zap this pointer.

But I dont understand the issue WRT in params and setters.  Why can not the 
nsReadableString pointer itself be NULL?

Feel free to mail me personally if it would add too much noise to the bug.

[FWIW, Python is _nearly_ working here; JS and Python can exchange DOMStrings 
now - one last (confusing) bug and I will make it available...]

Comment 18

[John Bandhauer]

Mark: these DOM interfaces use C++ references not pointers. So a null pointer is 
not an option. If we want to represent null (not "null") in a C++ reference to 
an nsAReadableString, then we need to have some way of getting and setting the 
'nullness' of an instance of nsA{Read,Writ}ableString.

I'm thinking that scc might have in mind a 'special' instance of 
nsAReadableString to represent null and a static getter to get a reference to 
it and a static function to compare any given instance to the null instance. I 
don't think this would do everything we need. An alternative would be the 
ability to create a null string object or assign null into a string object, and 
an accessor to determine if a given string object represents null. I think this 
would be better since it would allow for assigning 'nullness' into an 
nsAWritableString accessed via a C++ reference.

jst: I'm afraid you've lost me. It *sounds* like the native DOM objects should 
do their own determination of when to treat a passed in null as "null". Assuming  
that we have a way to represent null in an nsXXXString, then why would a null 
passed from JS to C++ not just be that nsXXXString null (as far as xpconnect is 
concerned)? If your code wants to interpret the null it receives from 
xpconnect as "null" for its own purposes, then fine. Am, I missing the point?

Also, what about "undefined"? Do you want the string? or null? or what?

brendan: assuming that we end up with a scheme that converts these jsvals into 
strings, then I agree. As long as we are up in the air, I'm inclined to leave 
the code as it is since it is already factored to do specific special handling 
for each of these cases.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

A version of PyXPCOM with DOM string support has been uploaded to:
http://users.bigpond.net.au/mhammond/PyXPCOM-0.92-DOMStrings.tar.gz
and also in .zip as:
http://users.bigpond.net.au/mhammond/PyXPCOM-0.92-DOMStrings.zip

I have verified that Python and JS can pass (non-NULL ;-) DOMStrings around, 
both as in and out params.  I have enhanced my test suite with a .js file, so 
most data conversions between JS and Python should be regularly tested.  Feel 
free to play with any of this!  Only tested on Win2k currently - will do Linux 
as soon as I can convince it that my laptop really does have a monitor plugged 
in!

This has not been uploaded as an "official" version of PyXPCOM yet, so only at 
the above address - feel free to tell anyone who wants to know tho!

I have done a fair bit of looking over jbands code, and can see no issues - and 
with /be on the case, I doubt I would be able to find anything that slipped 
under his radar anyway ;-)

Comment 20

[Brendan Eich [:brendan]]

Responding to jst first:
>The "null" buffer could potentially be the buffer from the JS engine.
>Brendan, is the buffer returned from JS_GetStringChars(JS_ValueToString(null))
>global or per context?

It's per-runtime, which is effectively global (there is usually no good reason
to have more than one JSRuntime per process address space).

>Is it accessable even if there's no JSContext around?

No, it's around until the last context in the runtime goes away.  At that point,
all per-runtime GC things (including atoms) are collected.  If a new context in
the runtime is created later, things will be reinitialized and atoms will be
recreated.

>Could PyXPCOM also use that same buffer or do we need to define the "null"
>buffer in XPConnect (or the DOM code)?

You want a single "null" to be shared by all languages that have such a literal.
Let's see: XPConnect is really JSXPCOM, to align naming conventions with
PyXPCOM.  Each subsystem must fend for itself, unless there is a more primitive
system both are willing to depend on.  XPCOM is obviously such a subsystem.  So
perhaps its string code (which might move into the even more primitive
mozilla/string subsystem, but the point remains) could export a usable constant?
Eww, exported data.  Ok, an accessor function.

Ah, jband beat me to that.  I agree with him that it would be better (if not too
expensive) to be able to mark nsXXXString instances as representing null and not
"null", or undefined and not "undefined".

jband: ok on the current patch -- can you add an XXX comment or something?  Cite
this bug if you like.

Hey Mark, when can we look forward to mozilla/extensions/python/xpcom? :-)

/be

Comment 21

[Johnny Stenback (:jst)]

jband, scc and I discussed this issue a few days ago and I think we have a
solution that is acceptable to all of us and it's flexible enough to let all
languages that are likely to ever use the DOM API in mozilla get the desired
bahavior. The solution requres a new virtual method in our string API's, named
something like IsNull() and scc agreed that he'd add such a method (plus the
required functions that are needed to use this new method, the new virtual
method would most likely be private) to the string API.

With this string API extension XPConnect (or whoever is calling the native DOM
code) can have it's own "special" nsAReadableString implementation (which is
fairly trivial to create) that can carry the "nullness" state in addition to the
string value ("null" for XPConnect when passing in null from JavaScript). The
DOM implementation will then simply ask the string (in the few places where it
matters) if the string is null or not and take appropriate actions. In the cases
where it doesn't matter if the string is null or not the DOM code will simply
use the string value w/o caring if the string was null or not and everything
works just as it does today.

As for PYXPCOM, it can use the same "special" string implementation as XPConnect
uses (or it can create it's own) and pass in null strings with the value "None"
if that's the desired result in Python (I think None is Python's equivalent of
JavaScript's null).

Anyone see any flaws in this solution, other than the fact that with this
solution implemented a string can contain a value and be null at the same time?

Brendan, jband, as for "undefined" the DOM doesn't really care (the spec
surprisingly enough doesn't mention what to do with undefined :-) so passing the
string "undefined" w/o special casing it seems acceptable to me. jband suggested
that XPConnect could throw a JS warning (i.e. put a message on the JavaScript
console) and I think that's a good idea. Should we always flag this warning or
only if the strict error checking is turned on?

Comment 22

[Mark Hammond [:markh] [:mhammond]]

I'm wondering if it is possible to "tickle" this bug by explicitly dropping the 
bug 67876 dependency, thereby ignoring the "DOMString is NULL" issue for this 
particular bug?  Nothing is "broken" by the lack of null strings, rather the 
feature simply has less utility.

I ask mainly for selfish reasons - the Python XPCOM code already has changes 
_required_ by these patches (IS_OWNED->IS_ALLOCD, etc)- as we weren't under the 
tree, I didn't manage the change via patches/bugzilla.  But it also occurred to 
me that waiting for bug 67876 may take a while, and make integration of these 
changes that little bit harder.  I could roll back and re-create the forward  
patch, but that is painful when these patches are basically sound.

Comment 23

[John Bandhauer]

I'm very happy to checkin what I have. We can make changes for null DOMStrings 
whenever scc's stuff is ready. I mostly didn't bother before 'cuz I thought only 
jst cared. And I'm sure he simply hacked the patch into his own tree. But if it 
will help Mark then great. Syncing my tree is good.

I'll attach the full patch again. I changed it to use NS_NAMED_LITERAL_STRING 
along with a bug fix from jag for other broken uses of NS_LITERAL_STRING.

If I can get a r= and sr= then I'll check it in.

Comment 24

[John Bandhauer]

Attachment: currrent full patch for review

Comment 25

[Johnny Stenback (:jst)]

I had a look at the patch and here's what I found:

- Stop-ship boog, what's up with the indentation of your C-style comments?

+ /*
+ * inout is not allowed with "domstring" types
+ */

Keep the *'s lined up like in the other C-style comments in the file, or is
there some logic behind this? :-)

The nsString useage in the diff could possibly be optimized somewhat by using
nsAutoString's and not nsString's in some places (to avoid having to allocate
the internal string buffer in most cases) but that part of this patch will
probably be rewritten anyway once there's support for copy-on-write n' such in
scc's strings, so this is fine with me for now.

Other than that the patch looks good to mee, r=jst (with the stop-ship fixed :-)

Comment 26

[John Bandhauer]

> - Stop-ship boog, what's up with the indentation of your C-style comments?

Fixed. BTW Johnny, Smooth carpool today :)

> The nsString useage in the diff could possibly be optimized somewhat

I'm betting that the case in xpcconvert.cpp is a rarely taken path. The case in 
xpcwrappednativeclass.cpp is tougher. This cooresponds to a string getter call 
from JS to C++. I can't really be making the nsAutoString on the stack here. Are 
you suggesting that I use "new nsAutoString"? People will point at me and laugh! 
I think we are all assuming that the string assigned into this nsString (or 
whatever it will become) will support refcounting and that no buffer copy will 
happen. *And* I'll be able to pass this wrapped refcounted string thing more 
directly into JS and *it* won't need to make a copy either. All in good time!

Thanks for the review.

Comment 27

[Johnny Stenback (:jst)]

Don't get me started (about the carpool :-)

Comment 28

[Brendan Eich [:brendan]]

jband: still looks good to me -- let's get this in, sr=[e-mail].

/be

Comment 29

[John Bandhauer]

The changes are checked in. I spun off bug 69468 to cover the fixes to add
better null support.

Comment 30

[Phil Schwartau]

Marking Verified -