67699 - xpcom needs nsIClassInfo accessible from object instances

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[John Bandhauer]

xpconnect interface flattening (bug 13422) requires a scheme for asking any 
given object for info about its class. I intend this to be an interface on the 
object's class object. More to come...

Comment 1

[John Bandhauer]

So, my plan is to extend the nsGenericFactory to also implement nsIClassInfo. 
nsModuleComponentInfo will include additional (hopefully optional) fields that 
will be used when class info is requested.

The nsIClassInfo interface I currently have in mind looks like:

[scriptable, uuid(986c11d0-f340-11d4-9075-0010a4e73d9a)]
interface nsIClassInfo : nsISupports
{
    /**
     * Identifiers for languages 
     */
    const PRUint32 LANGUAGE_CPP         = 0;
    const PRUint32 LANGUAGE_JAVASCRIPT  = 1;
    const PRUint32 LANGUAGE_PYTHON      = 2;
    const PRUint32 LANGUAGE_PERL        = 3;
    const PRUint32 LANGUAGE_JAVA        = 4;
    const PRUint32 LANGUAGE_ZX81_BASIC  = 5;
    const PRUint32 LANGUAGE_JAVASCRIPT2 = 6;
    
    /**
     * Get an ordered list of the interface ids that instances of the class 
     * promise to implement. Note that nsISupports is an implicit member 
     * of any such list and need not be included. 
     *
     * Should set *count = 0 and *array = null and return NS_OK if getting the 
     * list is not supported.
     */
    void GetInterfaces(out PRUint32 count, 
                       [size_is(count), retval] out nsIIDPtr array);

    /**
     * Get a language mapping specific helper object that may assist in using
     * objects of this class in a specific lanaguage. For instance, if asked
     * for the helper for LANGUAGE_JAVASCRIPT this might return an object that
     * can be QI'd into the nsIXPCScriptable interface to assist xpconnect in
     * supplying JavaScript specific behavior to callers of the instance object.
     *
     * Should return null if no helper available for given language.
     */
    nsISupports GetHelperForLanguage(in PRUint32 language);

    readonly attribute string   contractID;
    readonly attribute nsCIDPtr classID;
    readonly attribute PRUint32 implementationLanguage;

    /**
     * Bitflags for 'flags' attribute.
     */
    const PRUint32 THREADSAFE   = 1 << 0;
    const PRUint32 SINGLETON    = 1 << 1;
    // XXX what else might we want to add?

    readonly attribute PRUint32 flags;
};

The main thing we gain by this is necessary support for xpconnect interface 
flattening needed by the DOM conversion to use xpconnect. We also gain a place 
to specify such things as an object's threadsafety and a mechanism for 
determining class information given only a pointer to an object instance.

As it stands nsModuleComponentInfo would require new (optional) callbacks to get 
the interface list, and to get the language helper objects, and a field to 
specify the 'flags'. I think the other info this interface provides is already 
in nsModuleComponentInfo.

The main potentially controversial change is: Since xpconnect needs to be able 
to get the nsIClassInfo directly from object instances and we *don't* want to 
require participating objects to carry an extra vtbl pointer per object, I 
propose to ammend the xpcom identity rules to allow QI (when asked for 
nsIClassInfo) to return an interface pointer that *can't* be QI'd back to the 
original object. That is, if you QI any instance object of a particular class to 
nsIClassInfo then it would each return a pointer to that class's shared class 
object. QI'ing that class object won't get you back to any one of the object 
instances. This would be a special exception to the identity rules for this one 
interface only.

I also intend to add macro support for this QI implementation so that we can 
easily add nsIClassInfo support to *many* classes.

Comment 2

[Mike Shaver (:shaver emeritus)]

Mine.  I'll try for 0.8.1.

Comment 3

[Mike Shaver (:shaver emeritus)]

So, nsIClassInfo has these members:

    /**
     * null if not supported
     */
    readonly attribute string   contractID;

    /**
     * null if not supported
     */
    readonly attribute nsCIDPtr classID;

Is there any reason to believe that a given class will be reachable only through
a single contract ID?  Is it not enough to get the class ID and look up all
matching contract IDs for it?

Comment 4

[John Bandhauer]

Why make anyone do that? I was not thinking that this was the *only* contractID,  
but that the class can let callers know *a* contractID that would work to create 
and instance. The cid is fine for this run, but (I'm thinking) that if someone 
wanted to serialize an object then the contractid is good. This is also 
relatively human readable. We have it in nsModuleComponentInfo, let's expose it.

Comment 5

[Mike Shaver (:shaver emeritus)]

OK, I'll buy that (and update the comment to match).

Comment 6

[Mike Shaver (:shaver emeritus)]

Attachment: step 1: add data to nsModuleComponentInfo, and convert xpcom

Comment 7

[Mike Shaver (:shaver emeritus)]

Step 1: add nsIClassInfo data to nsModuleComponentInfo, and teach the rest of
XPCOM about it.  Along the way, clean up nsXPComInit.cpp something fierce, quell
a warning in the native component loader, and think unChristian thoughts about
the creators of nsPersistentProperties.idl.

I can build --enable-modules=xpconnect with this (and the patch in 71140), and
start up xpcshell.  Haven't tested any further than that yet.

Comment 8

[Mike Shaver (:shaver emeritus)]

Attachment: step 1.5: macros to support classinfo fun, and nsSample usages

Comment 9

[Mike Shaver (:shaver emeritus)]

(The 1.5 patch replaces the 1 patch.)

This patch has all the love from step 1, and also adds macros to support all the
nsIClassInfo infrastructure.  I also taught the nsSample code about it, and it
seems to work pretty well:

js> var s = Components.classes["@mozilla.org/sample;1"].createInstance();
js> s
[xpconnect wrapped nsISupports @ 0x808d998]
js> var ci = s.QueryInterface(Components.interfaces.nsIClassInfo);
js> ci.contractID
@mozilla.org/sample;1
js> ci.classID
{7cb5b7a0-07d7-11d3-bde2-000064657374}
js> ci.getInterfaces({ })
{7cb5b7a1-07d7-11d3-bde2-000064657374}
js>

Now I have to figure out how to make one piece of the code live without the
destructor, and then make all the rest of the macros happen.

I'm amenable to making the _CI variant of the macros go away and having _all_
generic components get flattening.  There's some footprint cost, but probably
not much.  On the other hand, it's no big deal for people to add _CI in one
place and a couple of NS_DECL_CLASSINFO lines elsewhere, if they want to flatten
and introspect and generally play with the cool kids.

Thoughts?

Comment 10

[Mike Shaver (:shaver emeritus)]

OK, so I think I've basically got GenericFactory case licked.  I still don't
know what to do for all the little helper classes (enumerators, for example) and
one-off custom factories (component manager and category manager, for example).

I guess I need some more helper macros for creating the classinfo objects for
these classes, but then I have to figure out the lifecycles.  Static instance
counter for each class?

Comment 11

[Mike Shaver (:shaver emeritus)]

I've checked in the first version of a doc on nsIClassInfo stuff
http://lxr.mozilla.org/mozilla/source/xpcom/doc/nsIClassInfo-overview.html. 
Comments welcome.

Comment 12

[Mike Shaver (:shaver emeritus)]

Attachment: Complete (huge) diff

Comment 13

[Mike Shaver (:shaver emeritus)]

Attachment: htmlparser changes

Comment 14

[Mike Shaver (:shaver emeritus)]

Attachment: intl changes

Comment 15

[Mike Shaver (:shaver emeritus)]

Attachment: mailnews changes

Comment 16

[Mike Shaver (:shaver emeritus)]

Attachment: view changes

Comment 17

[Mike Shaver (:shaver emeritus)]

Attachment: xpcom changes

Comment 18

[Mike Shaver (:shaver emeritus)]

Attachment: xpconnect changes

Comment 19

[Mike Shaver (:shaver emeritus)]

Attachment: misc changes

Comment 20

[Mike Shaver (:shaver emeritus)]

Alright, listen up.  Here's what we've got.

First, a 131K mega-diff that contains all the others.  If you want to test,
apply that one.

But what you really care about, what you're _living_ for, is to review these
babies.  So here's what's in the little patches that you care about:

htmlparser: (peterv?)
  A rewrite of the htmlparser module code.  I would have liked to have done a
  more thorough job with it, but I didn't have the brain cycles to re-engineer
  the initialization code.

intl: (erik?)
  Partial rewrite of the module logic.  Also signature changes and some warning
  cleanups.

mailnews: (alecf?)
  Rewrite of several modules.  I felt like such an archeologist!  Also signature
  changes in lots of importers.

view:
  Tiny change to use nsModuleComponentInfo.

xpcom:
  The meat.  The diff from above to add classinfo happiness, plus a mild module
  overhaul.

xpconnect:
  Signature updates in the loader, and adding CI support to a few test classes
  for, well, testing.

misc:
  Signature updates from across the board.  Lots of little, mechanical changes,
  include the repair of some modeline travis-ties.

I need reviewers!  Bring on your review!

Comment 21

[Erik van der Poel]

Roy, as the module owner for those parts of intl, please review. Then I will
super-review.

Comment 22

[John Bandhauer]

Mike. I still have to read closer. But I like what I see (a couple of notes 
below).

I *wish* we could come up with a scheme for combining CI maps with QI maps. If 
it weren't for the fun static_casting that QI has to do, I think we could manage 
it by building an iid array and having QI iterate the array rather than 
if/elseif. Oh well.

When jst and I talked about the CI map we envisioned a scheme that is a little 
more code intensive, but less fragile: The BEGIN part would declare an 
nsVoidArray, the ELEMENTS would insert pointers into the nsVoidArray (with goto 
error on failure) and the END would use the nsVoidArray info to build the out 
array and clone elements into it (with proper cleanup and failure if out of 
memory). This saves the programmer from having to track the array length or the 
element indexes. The END part could delegate to a shared function if we want to 
cut down space We could make an END marco that used the shared function:

#define NS_CLASSINFO_HELPER_END_HELPER(helper) \
 return helper(theVoidArray, theOutArray); \
}

... and have an end helper built into xpcom for those that want to link to it - 
with an additional macro hard coded to use that end helper.

What do you think?

Issues so far...

Bad bang!
+    if (!mInfo->mGetLanguageHelperProc)
+        return mInfo->mGetLanguageHelperProc(language, helper);

Should be using the shared allocator...
-> nsCRT::strdup(mInfo->mContractID)
-> *aClassID = new nsCID(mInfo->mCID);

Comment 23

[Mike Shaver (:shaver emeritus)]

There are table-driven QI implementations out there, and some of our macros are
set up to permit that case.  (``Essential COM'' has reference to such things,
but, infuriatingly, I can't find them right now.)  If/when we support them, I'll
gleefully convert the CI macros to use the same data.

I've been thinking about a better way to deal with the CI data anyway, which is
less data intensive and still doesn't involve creating an nsVoidArray (which
makes me a little uneasy, if we're going to be making these calls a lot).  I can
attach a separate diff with that setup, but I'd rather get this stuff in now --
once I get it reviewed and tested on Windows/Mac, ahem -- before doing that.

I'm shocked and dismayed to discover that nsCRT::strdup _doesn't_ use the shared
allocator for the |char *| case.  Why the heck not?

Thanks for catching the bad-bang.

Both are fixed in my tree, I'll repost a diff if you want.

Comment 24

[John Bandhauer]

> if we're going to be making these calls a lot

XPConnect will cache the info it gets from this call to reuse for future 
wrappers for the given class. And it will need this info from few classes. So it 
will be making very few of these calls. That is why I opted to not cut any 
corners in the nsIID copying for the array transfer. I don't think a temp 
nsVoidArray costs much. The mind-print cost of knowing what those numbers in the 
map declaration *mean* is high.

I agree that landing big patch soonest is good. I'd also like to see us simplify 
the map declarations before too many get written.

Comment 25

[Mike Shaver (:shaver emeritus)]

I'll cop to premature optimization, but the brainprint is where?  People using
NS_IMPL_ISUPPORTS3_CI have just as many magic constants as those doing it
without classinfo data, right?  The people who explicitly build QI tables with
BEGIN/ENTRY/END will need to specify an extra constant or four, of course.

But, in my relentless quest for self-improvement, I've got a Better Plan.  No
more magic constants, no nsVoidArray, and I'll even throw in an experimental
implementation of a table-driven QI.  You all hurry up with the reviews, and
I'll post an alternate GenericFactory/SupportsUtils patch or something tomorrow
morning.

roy/erik: the piece of this that I'm having the most trouble doing testing of is
the intl code.  Are there test cases that exercise the various converters and
such?  (Where ``exercise'' means ``instantiate and perform trivial
manipulation'', in this case.)

win32 folk: someone who can throw the complete patch on their tree and make sure
that the various importers at least _build_ will win bonus shaver-points. 
Otherwise I'll have to burn 10 hours tomorrow fighting with MSVC and build-tool
installs to do a test build, while the tree mutates beneath me.

Comment 26

[Peter Van der Beken [:peterv]]

Builds and runs on Mac. r=peterv, mac-tested=peterv. I'll do a separate patch
for the stupid NS_PARSER_IID (which should be a CID).

Comment 27

[John Bandhauer]

On NT... For some reason patch.exe did not like the diffs in xpcom/components. 
But I applied the diff to makefile.win (which looked OK to me) by hand and it 
worked.

You need to add:

Index: dlldeps.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/build/dlldeps.cpp,v
retrieving revision 1.55
diff -u -r1.55 dlldeps.cpp
--- dlldeps.cpp	2001/03/06 20:29:18	1.55
+++ dlldeps.cpp	2001/03/09 07:02:34
@@ -134,7 +134,7 @@
     xptc_dummy();
     xptc_dummy2();
     XPTI_GetInterfaceInfoManager();
-    NS_NewGenericFactory(NULL, NULL, NULL);
+    NS_NewGenericFactory(NULL, NULL);
     NS_NewHashtableEnumerator(NULL, NULL, NULL, NULL);
     nsCWeakProxy(0, 0);
     nsCWeakReferent(0);

With that change in place my mozilla NT depend built completed and seems to run 
OK.

Comment 28

[Peter "jag" Annema]

Also see bug 23737: Need table driven implementation of |QueryInterface|

Comment 29

[Mike Shaver (:shaver emeritus)]

Thanks, jband.  I've got that fixed in my tree now, too.

Off to work on a table-driven QueryInterface/GetInterfaceHelper while I wait for
reviews (which are coming soon, right?).

Comment 30

[Alec Flett]

this looks cool - my only question is on the getLanguageHelper() - if
nsIClassInfo is a singleton of sorts, how exactly is getLanguageHelper going to
get back to the original object to get a class such as nsIXPCScriptable? Seems
like you need to somehow access the original instance.. on irc, shaver suggested
nsISupports getLanguageHelper(in unsigned long language, in nsISupports obj)

Comment 31

[Alec Flett]

you're also using "PRUint32" all over your idl - shouldn't that be "unsigned long"?

Comment 32

[Peter "jag" Annema]

r=jag on the "misc changes" (attach_id=27154).

Comment 33

[Peter "jag" Annema]

Oh, and if you feel like it, could you defend the 80th column in the #define(s)
in nsICmdLineHandler.idl?

r=jag on "xpconnect changes" (attach_id=27153)

r=jag on "view changes" (attach_id=27151), with one small comment (per irc):

make it |static const nsModuleComponentInfo ViewManagerCI|

Comment 34

[John Bandhauer]

alecf: XPConnect is happy with language helepers that are not associated 
directly with the particular target native xpcom object. The purpose here is to 
allow language helpers that can be added without making changes to the target 
object implementation. This is good decoupling. This is very good for the DOM. 
In fact, the DOM (or other wrappable classes) could itself support dynamically 
registered plugin helpers that would help in mapping to additional languages 
without building *any* special support into the DOM.

In the XPConnect case, we can share the same helper instance for all object 
instances of a given class. This is good in many ways. The new nsIXPCScritpable 
also supports a flag to tell xpconnect to not even *bother* to QI the instance 
for instance specific helpers.

> you're also using "PRUint32" all over your idl

I don't see anything wrong with this. that is why we defined that alias in 
nsrootidl.idl. FWIW, I think it is better because it reminds people that these 
idl types are of a particular size and not platform dependent like C types.

Comment 35

[Alec Flett]

I think I understand what language helpers are for and why you don't actually
need them to exist in memory on a per-class basis (they shouldn't need to
maintain state) but maybe not - I guess I'll just wait until one gets
implemented .. I guess what I don't understand is this:

var classinfo = foo.QueryInterface(nsIClassInfo);
var helper =
classinfo.getLanguageHelper(LANGUAGE_JAVASCRIPT).QueryInterface(nsIXPCScriptable);

the only bit I don't get is how "helper" knows about "foo" so that if I call
helper[i] it knows to call foo->GetItemAt(i);

my comment is more of a question of "How the heck are you gonna do THIS?" than
anything resembling a code review :)

Comment 36

[John Bandhauer]

alecf: shaver's code that you quoted was just his test. In a given language 
mapping the classinfo might not even be exposed to the language (for instance 
nsIXPCScritpable is going to support hiding QueryInterface so that the helper 
can force JS code to *only* have access to the methods/attributes/consts of the 
interfaces it wants exposed).

The helper is not meant to be used *in* the language it helps.

The scheme xpconnect will use is:

When wrapping an object xpconnect QIs the object for its classinfo (say 'foo'). 
If it has seen that foo classinfo pointer before then it knows that this object 
is a foo and it can reuse all the info is knows about foo objects. If it has 
never seen foo it will gather info (such as the JS helper) and remember that for 
future wrappings of foo objects.

Assuming that the foo helper tells xpconnect to not bother QI'ing each foo 
instance for a per instance helper (or if the instances don't respond to QI for 
the helper) then the same foo helper is used on all calls from JS relating to 
the foo instances. 

*** What you may be missing is that methods on the helper (nsIXPCScriptable in 
the xpconnect case) *all* receive a pointer to the instance wrapper when called. 
I.e. the helper methods have an explicit 'self' param. This allows the helper to 
customize its response for each method call without requiring a helper per 
instance.

When there *are* helpers for other languages we'll encourage the authors of 
those systems to do the same.

Comment 37

[John Bandhauer]

And shaver is thinking: "stop chattering and review my patches!"

Comment 38

[John Bandhauer]

- In nsGenericFactory ctor and dtor you check mInfo for null before using. In 
many other methods of the class you don't. Either way seem OK with me, but I'm 
not seeing why you'd want to mix them.

- odd indent in nsGenericFactory::SetComponentInfo

- some compiler or another is going to warn like hell on the uninit'd new 
members of nsModuleComponentInfo, right?

otherwise... 

sr=jband on xpcom changes.
sr=jband on xpconnect changes.
- other modules certainly subject to closer review from their owners...
sr=jband if that is helpful on the rest of the changes. But, I really didn't 
read them close enough to catch subtle (or not so subtle!) mistakes that may be 
lurking.

It built (depend) and ran on NT4 for me. Good whackage!

I'd love to see us finish sneaking the classinfo (with iid maps!) into just 
about everything.

Comment 39

[John Bandhauer]

> In nsGenericFactory ctor and dtor you check mInfo

Ah, I see. You allow for the case where it is set later. But, you assume that 
callers that would need it to have been set had better not call until it 
actaully *has* been set. I'm OK with that.

Comment 40

[Roy Yokoyama]

Mike:  Intl module: The compiler generates error msg at NS_NewGenericFactory() on
NT. Can you take a look?

Comment 41

[Mike Shaver (:shaver emeritus)]

What error message?  Can you mail it to me, or put it in the bug?  Did you apply
the whole patch, or just the intl one (which is guaranteed to fail)?

Comment 42

[Mike Shaver (:shaver emeritus)]

The uninitialized structure member warnings _suck_.  I hate them, and I hate
being bullied by the compiler into putting more null-noise into everything.

Maybe I'll file a bug against blake to fix 'em after I check in. =)

I just untabified all of nsGenericFactory.cpp.  The hell with the cvsblame.

Comment 43

[Mike Shaver (:shaver emeritus)]

Guys: 0.8.1 closes on Tuesday, and I _really_ want to be on that train with this
stuff.  I'll be pretty upset if I'm now, because the tree is almost certainly
going to mutate underneath me when the 0.9 floodgates open.

r= needed for: intl, mailnews, xpcom.

I'll take jband's proffered global-sr, though erik's sr= on the intl stuff would
be appreciated too.

roy, alecf, scc: is there anything you need from me to perform the review?

A table-driven implementation will come later, when I either trick g++ into
producing good code, or decouple it from the table-driven QI stuff.  I'll file a
follow-on bug to optimize this stuff, once it's in place and jband can start to
use it.

Oh, and I hereby commit to changing any and all consumers of the _CI macros if
and when I update to a table-driven scheme.

Comment 44

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

FWIW, I've given the xpcom changes a review.  I like the table changes, but 
didn't actually confirm you didn't somehow miss a component as I figure that 
will show up pretty quick.

nsGenericFactory.cpp - The constructor, LockFactory, SetComponentInfo (which 
jband mentioned), and nsGenericFactory.h's class def have tab/space mismatches.

*mInfo->mClassInfoGlobal = 0; - I would have expected NULL/nsnull here?

Re comment in LockFactory:
+    // XXX do we care if (mInfo->mFlags & THREADSAFE)?
Do you mean we should check this flag and if set provide a real lock?

Other than that, if you get desperate you can use my review ;)

Comment 45

[Peter "jag" Annema]

r=jag on intl, if you promise to do the long term thing ;-p

Short-term, I think you can get rid of (most of) those NS_DEFINE_CIDs in
nsUConvModule.cpp, if you feel like making that change now.

r=jag on mailnews.

Comment 46

[Mike Shaver (:shaver emeritus)]

Mark:
> nsGenericFactory.cpp - The constructor, LockFactory, SetComponentInfo (which 
> jband mentioned), and nsGenericFactory.h's class def have tab/space 
> mismatches.

Yeah, I reindented the whole shebang to match the mode-line.

> *mInfo->mClassInfoGlobal = 0; - I would have expected NULL/nsnull here?

NULL is poor because some platforms #define it to |(void *)0|, which is a source
of errors on those obscure platforms.

nsnull or 0 are common, and I tend to use 0 for assignments and nsnull for
parameters (to indicate more clearly that it's a pointer-type).  But I know that
scc really dislikes nsnull, and I was hoping to get him to review that patch, so
I used 0 wherever I remembered. =)

jag:
Thanks for the review!  I'll not get rid of those possibly-spare CIDs right now,
just to avoid perturbing my diff this late in the game.  But yes, the codebase
needs to be cleansed of superfluous CID definitions.

Comment 47

[Brendan Eich [:brendan]]

Unlike other big changes looming, which have mostly been waved off (by their
owners) from 0.8.1, this bug seems fixable.  Shaver's patch works, according to
all accounts, and he's solicited module owner review in a timely fashion.  I say
it should go in, and not late tomorrow night.  How about this morning?

/be

Comment 48

[Christopher Blizzard (:blizzard)]

Agreed.

Comment 49

[Mike Shaver (:shaver emeritus)]

I'm getting a headstart from leaf this morning, so I'll be the first one in the
tree.  Updating and doing One Last Verification Build now.

(We're going to make it! Yay!)

Comment 50

[Mike Shaver (:shaver emeritus)]

Roy gave me some unicode tests to run, and we pass with flying colours:

Try the following.  It should cover sufficiently:
1- open your Navigator
2- go to www.netscape.com (which is ISO-8859-1) -> See any problem?
3- from above, go to File/Edit Page to bring up composer
4- go to File/SaveAsCharSet... in Composer
5- Select Western[Windows 1252] in SaveAsCharset dlgbox and make a file (say 
cp1252.html)
6- Select Unicode[UTF8] in SaveAsCharset dlgbox and make a file (say  utf8.html)
7- Select Western[MacRoman] in SaveAsCharset dlgbox and make a file (say 
macroman.html)
8- Select Western[ISO-8859-1] in SaveAsCharset dlgbox and make a file (say 
iso8859.html)
9- Close composer
10- Open newly created files in Navigator.

The above step #5 to #8 tests the Unicode Encoder and step #10 tests the Unicode
Encoder.

Whee!

Comment 51

[Mike Shaver (:shaver emeritus)]

In the tree!  Thanks, everyone!