93792 - nsIClassInfo::classID uses unsupported nsID passing convention

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[John Bandhauer]

Brendan, when you told me of this change the other day I just nodded. But I
thought about it later and realized that only C++ supports passing 'out' nsID
values the way you changed this code.

You've made this so the caller supplies an address of space to hold an nsID. The
callee then copies the data directly into that address.

The *supported* mechanism for nsID 'out' params is for the caller to supply the
address of a pointer sized location and for the callee to use nsMemory to clone
the nsID and return to the caller the address of the newly alloc'd block. The
caller is then responsible for freeing the new block. xpconnect *expects* that.
I imagine the other xptcall based layers do also.

I realize that you've streamlined things in this case on purpose. But you've
broken the non-C++ callers to this scriptable interface. xpconnect will crash on
this pattern.

Note that you did this with a readonly attribute. But if the attribute had been
writeable then xpidl would have generated a setter that passes an nsID struct by
value! The platform specific xptcall implementation are not up to that task at all.

I have to argue that the change you made to nsIClassInfo::classID and its
callers should be backed out. The line you added to nsrootidl.idl should be
removed. *And* the xpidl compiler should error out when it sees an [nsid] that
is not also marked [ptr] or [ref]. Note that bug 5416 suggests an xpidl error
for structs passed by value. It is an old bug.

Either we undo your change or *someone* needs to think through and fix our rules
about passing nsIDs and add support to *all* xptcall based code to not blowup
when it sees this pattern. xpidl would need to be changed to ensure it does not
generate sigs calling for the passing of nsID structs by value; i.e. more
special casing for 'in' v. 'out' cases for this one type. 

If you want to see this crash xpconnect then apply the patch in bug 93790 and
try the following in xpcshell:

var foo = new Components.Exception("foo exception");
var ciFoo = foo.QueryInterface(Components.interfaces.nsIClassInfo);
print(ciFoo.classID)

Comment 1

[John Bandhauer]

Assuming that the performance in this case (as used by the fastload mechanism)
is an overriding concern, then I have an alternate proposal... Revert this
particular scriptable method to its previous state and extend the nsIClassInfo
interface by adding a new getter method with a new name and mark it [notxpcom]
to support this instance of non-standard nsID 'out' passing... 

  [notxpcom] nsCID getClassIDFast();

We'd still want to add the check to the xpidl compiler to only allow this [nsid]
param passing convention in [notxpcom] methods.

If we choose this route, I'd also like to see the nsCID declaration moved out of
nsrootidl.idl to reduce the chance of future misuse.

Comment 2

[Brendan Eich [:brendan]]

jband: I am happy to change how nsISerializable users get a CID given an
interface pointer, but I'm not happy with a completely gratuitous malloc for
every object written to the FastLoad file (few now, many thousands later).  I
guess that means a [notxpcom] readonly attribute in addition to the restored
classID one, as your second comment proposes -- wheee!

One of Java's blunders was a limited memory model where every object is in the
GC heap.  XPCOM should try to use the stack for primitive types, where possible.
I know, it's more work for the (few, < 3?) xptcall users.  Tough.

Separately, I consolidated nsCID typedefs since there were several, and the
comment before the one in xpcom/components suggested moving the typedef to
nsrootidl.idl.  I argue that if xpidl reports errors as it should, there is no
harm in consolidating this typedef in nsrootidl.idl.  So I'd rather fix xpidl
than copy around duplicate typedefs.  What do you say?

/be

Comment 3

[Brendan Eich [:brendan]]

Targeting.  Patches soon.

/be

Comment 4

[John Bandhauer]

Brendan, I'm good with all that. I didn't realize that the declaration of nsCID 
was already extant.

I assume you don't plan to do the xpidl hacking. dbradley or I can get on that. 
We should also figure out why nothing in the xpidl->xpconnect chain was 
asserting on this pattern. I suspect that the xpidl compiler was setting the 
'ptr' flag in the generated xpt file when it should not have been. I *think* the 
code in xpconnect would have disallowed access for an 'out' nsid without the ptr 
bit set.

The [nsid] 'out' passing convention we have is my fault. It was patterned on the 
rules for string and wstring. But, given that nsID is of a known size (assuming 
that no one inherits from the class!), I could have allowed for the convention 
you are using. I think that we *should* add that support so that use of [nsid] 
w/o [ptr] or [ref] will look exactly the same as now for 'in' params, but will 
not do a malloc for 'out' params. We can leave the existing convention in place 
and add the new convention as an extension. We need an xpidl error (if not 
marked [notxpcom]) until that support is in place. We should add the error code 
immediately and the additional support when we can. I'm not going to volunteer 
for the larger task at this point. I'll do the error part unless dbradley is 
going to jump on it.

Comment 5

[Brendan Eich [:brendan]]

jband: thanks, and 128-bit architectures will thank you some day :-).

/be

Comment 6

[David Bradley]

I guess the question is what turn around time is needed here. I can probably get
this done either later today or early tomorrow (The smaller task ;-). I'm more
than happy to tackle it. I'll go ahead and start, jband yell if you've already
started or would rather do it to get it done in a shorter time frame.

So for the first part. 
Flag [nsid] native nsCID(nsCID); as an error
Allow [nsid, notxpcom] native nsCID(nsCID);

I'm a little unclear about the second part. We're adding support for [nsid]
native nsCID(nsCID); right?

Comment 7

[John Bandhauer]

dbradley: I'm happy to let you do this. A day or five is fine.

It is not the 'native' declaration that needs to be checked, but the actual use
of the type in method declarations. I think this means adding a block to
verify_method_declaration that will error if any param (or the return type) is
marked [nsid] but not [ptr] or [ref] UNLESS the current method is marked
[notxpcom]. Note that fixing this includes doing a little testing to make sure
you got it right, fixing (by adding [notxpcom]) any idl files in the tree that
would cause the error to fire, and also a full build to verify that you got it
right.

Also, xpt_dump of xpcom_components.xpt shows:

   G     uint32 classID(out retval nsIID *);

I believe that this shows the [ptr] bit is erroneously set in the typelib. It
looks like the code at...
http://lxr.mozilla.org/seamonkey/source/xpcom/typelib/xpidl/xpidl_typelib.c#826
...is making a bad assumption when it finds [nsid]. That could use some cleanup.
Thanks.

Comment 8

[Brendan Eich [:brendan]]

Attachment: my part of the fix, plus nsIClassInfo::EAGER_CLASSINFO

Comment 9

[Brendan Eich [:brendan]]

I'm combining a patch spun off from bug 92859, to allow generic module
components to request that their factories be created eagerly, at module init
time, instead of lazily, on first CreateInstance of the component's CID or
contractid.  This is needed for bug 68045.

The other change in the patch is the restoration of readonly attribute nsCIDPtr
classID to nsIClassInfo, and the addition of [notxpcom] readonly attribute nsCID
classIDNoAlloc.  Ugly name, but it was the clearest I could think of.  I made
the two nsIClassInfo implementors that I could find
(xpcom/components/nsGenericFactory.cpp and dom/src/base/nsDOMClassInfo.cpp)
implement both attribute-getters, and I changed js/src/xpconnect/src/xpcjsid.cpp
and xpcom/io/nsFastLoadFile.cpp to call the NoAlloc one.

Looking for r= and sr= tonight.

/be

Comment 10

[Chris Waterson]

sr=waterson

Comment 11

[Johnny Stenback (:jst)]

r/sr=jst

Comment 12

[Brendan Eich [:brendan]]

Patch is in.  dbradley, over to you for xpidl love -- thanks.

/be

Comment 13

[David Bradley]

Here's the patch. Some issues, it appears that references must also be marked as
pointers. I originally tried to set just reference or pointer flag in the
xpidl_typelib.c based on [ptr] or [ref], and Mozilla through fits. So the patch
sets the pointer flag if ptr or ref is used, and adds the reference flag if ref
is used, but does not set the pointer flag if it's neither a pointer or reference.

I encountered a couple of files nsIComponentManager.idl and
nsIObjectInputStream.idl. I added the [notxpcom] attribute but had to adjust the
stated return types since they no longer implicitly returned nsresults. Double
check this to make sure my understanding of XPCOM/XPIDL is correct.

I've applied the patch on Windows and Linux with clobber builds. Tested the ptr
and ref paths. Unfortunately I wasn't able to run the Linux version, because of
an X-Windows issue. I think Mac is probably the next big test. I'll need someone
to pitch in and build with the patch on the Mac.

Comment 14

[David Bradley]

Attachment: Patch to xpidl side and affected IDL files

Comment 15

[John Bandhauer]

In the typelib is_reference *requires* is_pointer...
http://mozilla.org/scriptable/typelib_file.html#is_reference
So, in the idl declarations [ref] *implies* [ptr].
It looks to me like you got the code right once you figured that out.

BTW, (from xpconnect's point of view) [notxpcom] implies [noscript]. But
[notxpcom,noscript] is perhaps helpfully verbose.
The patch looks good to me. Though I think you ended up with an unused...
+    gboolean is_ref;

I assume you ran a series of tests to show it doing the expected things. I wish
we had a battery of regression tests  - including error cases - to be updated.

Thanks.

r/sr=jband.

Comment 16

[Brendan Eich [:brendan]]

Looks good, 'cept for that stray gboolean is_ref -- one ultra-nit or two:

+    if (attr_type != NULL && IDL_tree_property_get(ident, "notxpcom") == NULL &&
+        IDL_tree_property_get(attr_type,"nsid") != NULL &&
+        IDL_tree_property_get(attr_type,"ptr") == NULL &&
+        IDL_tree_property_get(attr_type,"ref") == NULL)

+    {

+        IDL_tree_error(attr_tree,

+                       "Feature not currently supported: "

+                       "attributes with a type of nsid must be marked "

+                       "either [ptr] or [ref] or "

+                       "else must be marked [notxpcom]\n");

+        return FALSE;

+    }

+

This follows an if-then whose condition's first clause tests attr_type != NULL,
so maybe factor into if (attr_type != NULL) { if (...) {...} if (...) {...} }?

Also, how about a comma after the "...[ptr] or [ref]" and before the second " or "?

r=[e-mail], thanks for taking this.

/be

Comment 17

[David Bradley]

Thanks I missed the is_ref when I reorganized some of my logic.

I also removed the noscript. Out of curiousity I had went back to check uses of
notxpcom and there are several places that use [notxpcom, noscript] combination
so I let it stand thinking there might be some issue I was missing. Given your
explanation, I think it's just noisy and states what should be implicitly known.

Thanks for the if suggestion, good idea. And if anyone wants to offer better
error messages, I'll be more than happy to alter the patch again.

Slightly altered patch forth coming.

Comment 18

[David Bradley]

Attachment: Patch with jband's and Brendan's suggestions

Comment 19

[John Bandhauer]

sr=jband. I'm sure brendan's r= still applies to this slightly changed patch.

Comment 20

[David Bradley]

I forgot to ask, do you want me to check in both patches or just the one I created?

Comment 21

[John Bandhauer]

dbradley: We lost track of one thing we really want to catch here... [nsid]
structures passed by value. This is in general bug 5416. Some structs we allow
to be passed by value (in non-scriptable interfaces) e.g. nsPoint. In that case
the struct is no bigger than a pointer and we can more-or-less reasonably not
require [notxpcom]. But for structs in general this is problematic. Anyway, bug
5416 is the place to address those general isseus.

Nevertheless, for [nsid], we really want to catch the case where non-[ptr]
[nsid]s are passed as 'in' params and make that an error regardless of the
[notxpcom] status. This also means that if that type is used in an 'attribute'
it *must* be a 'readonly' attribute. I think we should address this now before
the use of non-[ptr] [nsid] grows.

Comment 22

[David Bradley]

Attachment: Added check for in nsid parameters and non-readonly nsid attributes

Comment 23

[David Bradley]

I meant to add an additional comment. Pay particular attention to the boolean
logic in the check of the attribute and method declaration verfications in
xpidl_util.c. I've tested it against all the permutations that I believe exist,
but I might have missed one.

Comment 24

[David Bradley]

I'd like another r= and sr= for this updated patch. It's not that much
different, but I could use a sanity check on it to make sure I didn't botch the
boolean logic. As I said I tested all the variations I could come up with and it
checked out ok.

Comment 25

[John Bandhauer]

r/sr=jband on the last patch.

This looks right to me.

The text could make it a little clearer that the [notxpcom] attribute would have
to apply to the method decl, while the [ptr] or [ref] would have to apply to the
'native' declaration of the type (not as a modifier of the method decl). But,
I'm not going to press that. This stuff is just plain confusing anyway. The
current text is OK.

MarkH: didn't I see you made a change in the PyXPCOM stuff to deal with the
change to nsIClassInfo::classID that brendan's most recent patch has since undone? 

Comment 26

[Brendan Eich [:brendan]]

Still looks good to me, I suggested applying De Morgan's theorem to avoid
!notxpcom (separating the ! and notxpcom by a parenthesis), and preserving the
sacred 80th column, but otherwise, sr=[e-mail] -- testing is the key,
and it sounds like the latest patch is well-tested.

/be

Comment 27

[David Bradley]

Attachment: patch with Brendan's suggestions

Comment 28

[Brendan Eich [:brendan]]

+        if ((IDL_tree_property_get(ident, "notxpcom") == NULL ||
!(IDL_ATTR_DCL(attr_tree).f_readonly)) &&

way over column 80, but now I'm a whining dinosaur!

Ship it!

/be

Comment 29

[David Bradley]

Patch checked in
Sorry brendan, your comment made it to me after the checkin. I don't consider
you a whining dinasour, I try and keep things less than 80 as well. This somehow
slipped passed my radar. I'll get my pruning sheers out next time I'm in there.