65762 - xpidl needs support for nsA{Read,Writ}ableString

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[John Bandhauer]

We need to add support for nsAReadableString and nsAWritableString to xpidl 
and typelibs. There are choices to b made about how best to do this.

The DOM uses these new string classes for its strings. We'd like people to 
transition to using the classes for other strings also.

It is way too late in the game (I argue) to hijack the 'string' family of
keywords in xpidl.

The nsA{Readable,Writable}String classes are declared in:

http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsAReadableString.h
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsAWritableString.h

They are declared as abstract classes. They are NOT inherited from nsISupports.

The DOM usage of these classes is consistently like:

 NS_IMETHOD GetFoo(nsAWritableString& aFoo)=0;
 NS_IMETHOD SetFoo(const nsAReadableString& aFoo)=0;

In both cases the param is passed an an 'in' param, though in the 'Get' case
an instance is passed in and filled with the outgoing data. Note that DOM
uses C++ references ('&') when passing these classes. Where these xpcom 
interfaces, then this would be counter to the xpcom rules regarding passsing
interfaces by pointer.

I can see 3 reasonable options for adding support for these classes to xpidl
and typelibs (adding support to xpconnect and other mappings is a mostly 
separate issue):

1) 'opaque-type': Add support for named opaque types in general. 
2) 'special-interface': Treat them as a special interface.
3) 'basic-type': Treat them as a new basic type.

I like the 'basic-type' option best. Here is some backgroud followed by the 
pros and cons of the options in order...

One important consideration is that we want to avoid making an incompatible
change to the typelib format - http://mozilla.org/scriptable/typelib_file.html -
for as long as we can. Some changes are minor. But some changes would produce
typelib files that could not be read by older software. libxpt has a system - 
added at the last minute before NS6 - whereby it will refuse to try to read a
typelib whose major version number is higher than a give number.

Eventually, we will have to go into incompatible typelib versions. When that 
time comes, we will also need to add code to xpidl to allow people to use the
newest version to create *older* versions of typelibs; i.e. a cmdline flag
that says: "I need to make a version 1.x typelib and I promise not to use any
post 1.x features (produce an error if I'm lying)".

The main issue in possible typelib incompatibility is the way type descriptors
are stored. See: http://mozilla.org/scriptable/typelib_file.html#TypeDescriptor

libxpt (the code that reads and writes typelibs) will have no problem with
new types that fit in the basic SimpleTypeDescriptor. But any type that 
requires additional info will need libxpt changes.

See: the code in libxpt for SizeOfTypeDescriptor
http://lxr.mozilla.org/seamonkey/source/xpcom/typelib/xpt/src/xpt_struct.c#472
    

It would be best to avoid that whole set of problems as long as possible.

Option '1' ('opaque-type') would require an incompatible change to the typelib
format. The idea is that now any type that does not fit into the limited set
of supported types gets declared in xpidl as 'native', must be passed by 
pointer, is represented in the typelib as a generic 'void*'. We store no 
additional info about its type. One of Scott Furman's drafts of the typelib
spec suggested supporting 'opaque types'. This meant storing in the typlib the
string for the name of the type. The idea was that we might one day support
passing these around even in languages that could not understand the type;
i.e. an object could receive a 'Foo' pointer and then send it off to another
object -  as long as both the incoming and outgoing interface methods talked
about an opaque type with the same name 'Foo', then it would be a typesafe
transaction. This was also tied into some notions of storing typedef 
hierarchies in the typelibs. We decided that this whole set of features was 
beyond the scope of what we needed to do, so it was not implemented and removed
from newer typelib specs.

Anyway, the 'opaque-type' option would require implementing this feature and
doing special runtime handling in xpconnect (and other mappings) for opaque
types whose names matched 'nsAReadableString' or 'nsAWritableString'.

The pro is that this would be a very generic mechanism.

The cons are: a) this requires an incompatible typelib format change and
b) this means looking at strings of typenames in xpconnect to distinguish
types.

I think con 'a' is a fatal flaw.

Option '2' 'special-interface' would have us treat these classes in xpidl as
we treat xpcom interfaces. They would be declared like:

%{C++
#if 0
%}
[uuid(f4c37ab0-ecb1-11d4-906d-0010a4e73d9a)]
interface nsAReadableString {};

[uuid(f9e72720-ecb1-11d4-906d-0010a4e73d9a)]
interface nsAWritableString {};
%{C++
#endif
%}

They would not inherit from nsISupports. We would not declare their methods 
in xpidl. These declarations would not conflict with the reagular declarations 
in the .h files.

xpconnect (and like technologies) would need to distinguish these special
interfaces by checking every interface it sees for these two (well known) 
uuids.

An additional consideration is that xpidl does not currently support passing
interfaces by C++ reference. But the DOM usage requires that here. So, we'd
need to modify the xpidl compiler - either hardcoded to allow pasing by 
C++ reference for these two special types, or for all interfaces. *I'd* rather
not allow this for all interfaces.

The pros are: 
a) Simple changes.
b) Typelib format is only the nuance of 'interface-passed-by-&'.
c) Hardcoding 'OK to pass these special interfaces by ref' into xpidl compiler.

The cons are:
a) Having to distinguish these special types by uuid at runtime. 

I don't think this would be terrible, but I'd like to avoid the runtime 
special-casing.

Again, I prefer option '3'...

Option '3' Treat these two classes as new 'basic-type's.

This would be done very mush like how we do nsids. We would do 'native' 
declarations of these types in one place (nsrootidl.idl):

  [ref, readable_string] native nsAReadableStringRef(nsAReadableString);
  [ref, writable_string] native nsAWritableStringRef(nsAWritableString);
  [ptr, readable_string] native nsAReadableStringPtr(nsAReadableString);
  [ptr, writable_string] native nsAWritableStringPtr(nsAWritableString);

And then they could be used anywhere:

 void GetFoo(in nsAWritableStringRef aFoo);
 void SetFoo([const] in 
nsAReadableStringRef aFoo);

This would use up two more of our (max) 32 types. But, we are saying that 
these are to be our prefered core string types. I think this is OK.

These declarations fit into SimpleTypeDescriptor without a change to libxpt.
We would increment the typelib minor version number, but I think we could get
away without considering this an incompatible change.

The one messy bit of backward compatibility is the fact that xpconnect in any
shipped product up to now did not adequately allow for any new uses of the 32
possible types. It has a table driven scheme for checking types that can be 
seen at: 
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcconvert.cpp#49

The problem is that I failed to add table entries for as yet unused types. I
will fix that for the future. For now there is a risk that a new typelib
(using one of these two new types) might get installed by some aftermarket
component into an old browser. The code above then might read off the end of 
the table. (At least on Windows) I can deal with this by making the two new 
type identifiers be: T_BSTR and T_PWSTRING_SIZE_IS+1 (i.e. 15 and 23). We can
finally give up on T_BSTR and use it for one of these string classes. In this
table (in shipping browsers) '15' is already marked as being a type that the
code can't handle (0 in the table). And, since this is a table of 23 uint8 
values, the uint8 for the 24th slot is readable and set to 0 (at least on 
windows!). I think we can get away with this and still not crash in the odd 
case where someone adds a typelib with these types into an old browser.


pros:
a) minimal typelib change.
b) adding new basic types that are distinguishable in switch statement.
c) no messy 'pretending its and interface passable by &' hacks.

cons:
a) slight chance of backward compatibility issue on some platforms?

Anyway, I'm hot to implement option '3'.

Discussion?

Comment 1

[John Bandhauer]

Attachment: patch that implements option '3'

Comment 2

[John Bandhauer]

The above patch implements option '3'. It automatically emits 'const' for 
nsAReadableString 'in' params (like we do with string and nsid 'in' params).

I'm also assuming that we are not going to want to support the 8bit ascii 
versions of these string classes: nsA{Read,Writ}ableCString. Or any of the other 
variations on these string classes.

The change that will effect code not in the standard build is the removal of the 
T_BSTR #define.

Support for xpconnect actually *using* the new string classes in not here yet. 
But the xpidl compiler should do the right thing and xpconnect should still 
build and run.

Comment 3

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

I agree this is the most practical solution.  Consider Python support for this 
nearly implemented ;-)

Comment 4

[John Bandhauer]

Crap! Option '3' - as it is - is not going to do what we need. I was overfocused
on matching the C++ usage and not thinking deep enough about the way this would
map into other languages. The DOM guys have set a data exchange pattern that is
unlike any other employed in xpcom. Their getters work by passing *in* an object
owned by the caller to a getter method to be filled by the callee. The clues we
use to tell xpidl about 'out' params don't apply and my implementation of option
'3' does not have support to let xpconnect (et al.) know how to map the "in
nsaWritableString" to a *returned* JSString.

We also really want to do the Get/Set pairs as xpidl attributes to get the right
calling syntax for non-C++ caller. Yet, there are some DOM methods that 'return'
nsAWritableString but also take other params. So, a scheme for [retval] that
does not require an 'out' param is required too.

I have a plan for this (that is only a minor perversion of xpcom/xpidl). But
first I want to verbally abuse the DOM guys for choosing a data exchange scheme
so foriegn to xpcom. Assuming that they remain steadfast in their demand that
their interface signatures should not change (again) then I'll write up my
option '4' and have a go at implementing it.

Comment 5

[John Bandhauer]

Ok. Here's option '4'...

I expect that jst will like this because the declaration syntax is simpler.

We use only one typelib type - TD_DOMSTRING - that replaces TD_BSTR.

Info about whether that type maps to the nsAWritableString or just 
nsAReadableString is implicit in how the type is used in any given param.

We declare this in nsrootidl.idl as:

[ref, domstring] native DOMString(ignored);
[ref, domstring] native DOMStringRef(ignored);
[ptr, domstring] native DOMStringPtr(ignored);

I expect that only 'DOMString' will be used, but the explicit 'Ref' variant
is declared along with the 'Ptr' variant in case anyone wants to pass them
using '*' rather than '&'.

If we want to promote the use of these strings outside the DOM, then we can 
alias easily with any name we like....

[ref, domstring] native ScottCollinsString(ignored);


Given the input:

  DOMString f1();
  void      f2([retval] out DOMString s);
  void      f3(in DOMString s);
  void      f4(in DOMStringRef s);
  void      f5(in DOMStringPtr s);

           attribute DOMString a1;
  readonly attribute DOMString a2;

We generate for C++:

  /* DOMString f1 (); */
  NS_IMETHOD F1(nsAWritableString & _retval) = 0;

  /* void f2 ([retval] out DOMString s); */
  NS_IMETHOD F2(nsAWritableString & s) = 0;

  /* void f3 (in DOMString s); */
  NS_IMETHOD F3(const nsAReadableString & s) = 0;

  /* void f4 (in DOMStringRef s); */
  NS_IMETHOD F4(const nsAReadableString & s) = 0;

  /* void f5 (in DOMStringPtr s); */
  NS_IMETHOD F5(const nsAReadableString * s) = 0;

  /* attribute DOMString a1; */
  NS_IMETHOD GetA1(nsAWritableString & aA1) = 0;
  NS_IMETHOD SetA1(const nsAReadableString & aA1) = 0;

  /* readonly attribute DOMString a2; */
  NS_IMETHOD GetA2(nsAWritableString & aA2) = 0;


Note that DOMString can be used as a return type (no special [foo] is 
necessary that would have forced us to always use [retval]). Also, note that
we use 'out' with DOMString out params even though the actual behavior is to 
pass *in* a class instance to hold the data that is coming out. You'll see
below that for the typeinfo data the 'out' that is declared in idl becomes
'in' in the typelib.

Another point is that the use of the classes 'nsAReadableString' and 
'nsAWritableString' is now hardcoded for the type [domstring] as it needs
to be for the xpconnect-ish clients of typelibs. This is really no different
from [nsid] meaning nsID.

From the idl the typelib gets (in xpt_dump output):

         uint32 f1(in dipper retval DOMString &);
         uint32 f2(in dipper retval DOMString &);
         uint32 f3(in DOMString &);
         uint32 f4(in DOMString &);
         uint32 f5(in DOMString *);
   G     uint32 a1(in dipper retval DOMString &);
    S    uint32 a1(in DOMString &);
   G     uint32 a2(in dipper retval DOMString &);

Note that there is a new param flag called 'dipper' (or XPT_PD_DIPPER in the 
code). The idea is that for the [domstring] type - and maybe more types in the 
future - we support a new way of transferring data *out* during the call via 
a special class that is passed *in* to get data out. Lacking a better name, I 
decided to call this a 'dipper' class: we dip it in to pull stuff out.

Anyway, the xpidl compiler will generate special bits when it sees an 'out' 
pattern used with a [domstring]. It converts 'out' to 'in' and adds the 
'dipper' bit. The retval bit will be used as before. The one rule change is that
it will now be legal for the typelib to combine 'in' and 'retval', but only
if the 'dipper' bit is also set. I will update the typelib spec to reflect
these changes. I don't consider this an incompatible change

I *really* didn't want to make typelib clients deal with cases where the 'out'
bit actually marshalled something 'in'. Passing a dipper class 'in' is pretty
much the same as passing an interface pointer 'in'. So, I'm going with that 
pattern which will not confuse lower level layers like xptcall. The 'dipper' 
bit specifies the real behavior for higher levels like xpconnect.

Note that it is possible to have something like:

 void foo(out DOMString out1, out DOMString out2, out DOMString out3);

i.e. multiple 'out' DOMStrings. Though, we never do this in the DOM.

The changes to support this option are fairly small. It is a big plus that
only the BSTR type id needs to be hijacked. So, the backward compatibility
issues pretty much go away.

xpconnect-ish layers (including xpcom/proxy and PyXPCOM) will need to add 
support. This should not be too tough. For calling 'out' cases, temporary
nsAWritableString instances will be required. But these have a well bounded 
lifetime. Building or extracting data from nsAReadableString is pretty 
straightforward. In the longerterm, we intend to have implementations of these
abstract classes that can wrap immutable data and be refcounted such that
we can interact with script system strings and yet do no copying at all.

I'll attach a patch that has all the xpidl changes and an xpconnect change that 
builds, but does not yet support the new types. I still need to do better 
verification on the xpidl changes to make sure I handled all the issues. But,
the patch should work well enough to get people started with this change.

Comments appreciated.

Comment 6

[John Bandhauer]

Attachment: better patch to implement option '4'

Comment 7

[Johnny Stenback (:jst)]

This sounds absolutely great from a XPIDL user point of view, it does everything
I need to do w/o a need for syntactic modifications to the interfaces defined in
the W3C DOM wrt DOMString's, I like it!

I had a look at the patch and it looks very good to me, please keep in mind that
I don't know the first thing about XPIDL internals. r=jst for what it's worth.

I also applied the patch and did some tests it looks like XPIDL with the patch
generates the correct C++ headers, excellent work!

Comment 8

[Brendan Eich [:brendan]]

jband, looks good to me (dipper, heh) -- how is it testing?  Ob. nit whines:

- 'if(' style is creeping into xpidl sources, which use 'if (' usually.

- Not your doing, but every message of the form:

 IDL_tree_error(method_tree,
                            "methods in [scriptable] interfaces which are "
@@ -477,7 +479,8 @@

should be re-worded to say "... that are" rather than "which are".

/be

Comment 9

[John Bandhauer]

I'm not finding anything wrong with my changes. It seems to do what I expect. I
did a clobbver build and nothing else seems to be broken. jst has been using the
new stuff and the right headers come out.

I'm ready for an sr= and I'll get it into the tree.

I'll attach a patch that fixes the nits brendan pointed out.

Comment 10

[John Bandhauer]

Attachment: same as previous patch, but fixes whitespace and error msg grammar

Comment 11

[Brendan Eich [:brendan]]

sr=[e-mail], cool beans.

/be

Comment 12

[John Bandhauer]

Changes checked in.

I opened bug 66610 to track adding xpconnect support for these types.

Also, I have yet to update the typelib spec to reflect the changes. Coming soon.