291512 - Use nsIInterfaceInfoManager to generate Java interface files

Status: RESOLVED FIXED

(Core Graveyard :: Java to XPCOM Bridge, defect)

Description

[jhp (no longer active)]

Currently, I am using xpidl to generate the Java interface files.  The main
issue with this is that xpidl doesn't know whether some interfaces are
IDL-defined, so it ends up writing out "nsISupports" for many parameters.  The
InterfaceInfoManager should be able to supply the correct interface names.

Comment 1

[jhp (no longer active)]

Attachment: patch

Comment 2

[jhp (no longer active)]

The attached patch works for the most part, but there are some issues that I
hope bsmedberg, shaver, or biesi can help me on:

1) Several interfaces have param types of non-scriptable interfaces (i.e. some
methods pass nsIAppShell params).  This means that I also need to write out
non-scriptable interfaces, or I can't compile the Java interface files into classes.
2) SWT Browser in Eclipse uses non-scriptable interface nsIAppShell (bug 270892).
3) SWT Browser also uses some [noscript] methods, in particular
nsIBaseWindow.initWindow(), nsIFile.getNativeLeafName(), and
nsIEmbeddingSiteWindow.getSiteWindow().  So I also have to write out [noscript]
methods.
4) Some interfaces have method names that are Java keywords.  In this patch, I
just add an underscore to the end of the method name.  Is there a better solution?
5) Some one suggested that I should create 2 separate JAR files for the Java
interface classes: one each for frozen and unfrozen interfaces.  This would make
any user more conscious about using unfrozen interfaces.  But the
InterfaceInfoManager doesn't have information on whether an interface is frozen.
 Maybe add another bit flag and method to nsIInterfaceInfo?

Comment 3

[Mike Shaver (:shaver emeritus)]

(In reply to comment #2)
> 1) Several interfaces have param types of non-scriptable interfaces (i.e. some
> methods pass nsIAppShell params).  This means that I also need to write out
> non-scriptable interfaces, or I can't compile the Java interface files into
classes.

Yeah, you'll need to mention them, at least.

> 2) SWT Browser in Eclipse uses non-scriptable interface nsIAppShell (bug 270892).
> 3) SWT Browser also uses some [noscript] methods, in particular
> nsIBaseWindow.initWindow(), nsIFile.getNativeLeafName(), and
> nsIEmbeddingSiteWindow.getSiteWindow().  So I also have to write out [noscript]
> methods.

How do they use noscript methods?  Do you have custom marshalling code for those
methods?  I'd not have thought you could safely call them, since they're not
necessarily trading in xptcall-safe types, etc.

> 4) Some interfaces have method names that are Java keywords.  In this patch, I
> just add an underscore to the end of the method name.  Is there a better solution?

We just use an underscore postfix in JS, too.  I think it's fine.

> 5) Some one suggested that I should create 2 separate JAR files for the Java
> interface classes: one each for frozen and unfrozen interfaces.  This would make
> any user more conscious about using unfrozen interfaces.  But the
> InterfaceInfoManager doesn't have information on whether an interface is frozen.
>  Maybe add another bit flag and method to nsIInterfaceInfo?

You'd need to add it to the XPT file format, which I don't think is going to
happen any time soon, in part because xpidl would have to told about frozenness
too.  You could build a secondary dataset out of doxygen output, I guess, but I
don't see this appearing in the IIM even in the 2.0 timeframe.

Comment 4

[jhp (no longer active)]

> > 1) Several interfaces have param types of non-scriptable interfaces (i.e. some
> > methods pass nsIAppShell params).  This means that I also need to write out
> > non-scriptable interfaces, or I can't compile the Java interface files into
> classes.
> 
> Yeah, you'll need to mention them, at least.

What about writing the name of any non-scriptable interface param as simply
"nsISupports"?  That way, we don't have to spit out all of the non-scriptable
interfaces.

Crap!  Why are |nsITooltipListener| and |nsIContentMenuListener| non-scriptable?
 SWT uses both of these (in addition to |nsIAppShell|).

> How do they use noscript methods?  Do you have custom marshalling code for those
> methods?  I'd not have thought you could safely call them, since they're not
> necessarily trading in xptcall-safe types, etc.

|nsIFile.getNativeLeafName()| is used to get the name of a downloaded file in
order to display in the download dialog.  Not sure if they really need the name
in the platform codepage, or if they could get away with just using |getLeafName()|.

|nsIBaseWindow.initWindow()| and |nsIEmbeddingSiteWindow.getSiteWindow()| are
used to mess around with the actual window handle (pointer represented as Java
int).  SWT likes to do that kind of stuff.

Instead of spitting out every non-scriptable interface and method, what about
just special casing the ones that are used by SWT?  It's somewhat hacky, but it
will also make it easier to manage in Javaconnect.

Comment 5

[jhp (no longer active)]

nsITooltipListener, nsIContextMenuListener, nsIContextMenuListener2, and
nsITooltipTextProvider (all in /embedding/browser/webBrowser/) are all declared
as non-scriptable interfaces.  But I can't see the reason why?  Anyone know?

If they were made scriptable (particularly the first two), then I wouldn't need
to output non-scriptable interfaces.  I would only need to special case
|nsIAppShell| (bug 270892).

Comment 6

[Darin Fisher]

(In reply to comment #5)
> nsITooltipListener, nsIContextMenuListener, nsIContextMenuListener2, and
> nsITooltipTextProvider (all in /embedding/browser/webBrowser/) are all declared
> as non-scriptable interfaces.  But I can't see the reason why?  Anyone know?

This is just a theory, but maybe there are some DOM elements that implement
those interfaces?  Maybe noscript exists as a simply way to limit access to
these interfaces by web content?  I'm taking a wild guess really... I might be
way off.

Comment 7

[Darin Fisher]

Actually, I think LXR sort of answered my question.  These interfaces appear to
be used only by the embedding code.  So, yeah... why are they non-scriptable?!?
 It looks like they have been that way since version 1.1.

Comment 8

[jhp (no longer active)]

Attachment: patch v1.1

Changes in this patch:
- Special cased the non-scriptable interfaces used by SWT, so we don't output
all non-scriptable interfaces.	If and when |nsITooltipListener|, et. al. get
fixed, then I'll just need to special case |nsIAppShell|.
- When writing out parameter types, if the type is a non-scriptable interface,
then simply write "nsISupports" for the type.
- Special cased the [noscript] methods to only output them for the interfaces
used by SWT.  SWT uses these methods to deal with native window handles, and
the marshalling code works well for that.
- Use |run-mozilla.sh| to call the util.  Is this a valid cross-platform way to
launch an app?

Comment 9

[timeless]

no one uses run-mozilla.sh on windows (probably not on os/2 either), but
everywhere else, yeah :)

Comment 10

[Darin Fisher]

I think we should just mark those interfaces scriptable.  Maybe send Adam Lock a
direct email to see if he remembers why he made them non-scriptable just in case
there is something we should worry about ;-)

Comment 11

[timeless]

i'd vote for making them scriptable too

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #9)
> no one uses run-mozilla.sh on windows (probably not on os/2 either), but
> everywhere else, yeah :)

what about macos?


I also think we should make those interfaces scriptable, unless there was a good
reason for them not to be.

Comment 13

[jhp (no longer active)]

(In reply to comment #9)
> no one uses run-mozilla.sh on windows (probably not on os/2 either), but
> everywhere else, yeah :)

So what's the recommended cross platform way to run a small utility app from
within a Makefile?  I need to make sure the PATH is setup to point to XPCOM or
libXUL.

Looks like it is used cross platform in
http://lxr.mozilla.org/mozilla/source/xpinstall/packager/Makefile.in#153.

Comment 14

[timeless]

on osx you can use run-mozilla.sh (i do).

on windows i've never needed to use it as long as i ran from the directory where
the app lives. the difference is that on windows, . is in your path.

Comment 15

[timeless]

(that and the fact that .sh isn't handled by the native shell, which makes using
it dubious).

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

make's shell is some POSIXy shell (bash in many cases, I guess)

Comment 17

[jhp (no longer active)]

Attachment: patch v1.2

Makes those 4 embedding interfaces scriptable, and only special cases
'nsIAppShell' interface.  Darin, biesi, if you guys could at least review the
interface changes.  If you have time to take a look at the rest of the code, I
won't mind!

Comment 18

[Darin Fisher]

Comment on attachment 182193 [details] [diff] [review]
patch v1.2

The interface changes look good to me.


>Index: extensions/java/xpcom/tools/genifaces/GenerateJavaInterfaces.cpp

>+#ifdef WRITE_NOSCRIPT_METHODS
>+// SWT uses [noscript] methods of the following interfaces, so we need to
>+// output the [noscript] methods for these interfaces.
>+static char* kNoscriptMethodIfaces[] = {
>+  "nsIBaseWindow", "nsIEmbeddingSiteWindow"
>+};
>+#endif

Good times ;-)	I think we should consider providing alternate interfaces
that are java friendly.  Or, maybe these java bindings could provide a
class or two that wrap these interfaces to support embedding.  I don't
know... or maybe this special casing is just fine.


>+    char buf[32];
>+    switch (aType->TagPart()) {
>+      case nsXPTType::T_I8:
>+        snprintf(buf, 31, "%d", aValue->val.i8);

maybe use |sizeof(buf)-1| instead of |31|?


>+    out->Write(buf, strlen(buf), &count);
>+
>+    return NS_OK;
>+  }

Also, I notice that you often do not check for errors from the Write
method calls.  Is that something to worry about?  Or, is any error
resulting from a partial write or failed write handled elsewhere?


sr=darin (though I didn't really do such a thorough review of the
stuff under extensions/java)

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 182193 [details] [diff] [review]
patch v1.2

+      char* iface_name = nsnull;
...
+      nsMemory::Free(iface_name);

maybe nsXPIDLCString? it seems you have some breaks there that'd skip the
freeing

@@ -1086,20 +1105,41 @@ nsJavaXPTCStub::GetRetvalSig(const nsXPT

aren't you leaking the iface_name here?

extensions/java/xpcom/build/Makefile.in
+	@if test ! -d _javagen/org/mozilla/xpcom; then \
+		touch .done; \
+		$(INSTALL) -m 644 .done _javagen/org/mozilla/xpcom; \
+  fi

fi seems wrongly indented


extensions/java/xpcom/tools/genifaces/GenerateJavaInterfaces.cpp
+      // For example, the |onStreamComplete| method for the interface
+      // |nsIUnicharStreamLoaderObserver| takes a param of
+      // |nsIUnicharInputStream|, which is defined in a simple header file,
not
+      // an IDL file.

I want to fix this specific example :-)

+      *aResult = (char*) nsMemory::Alloc(12 * sizeof(char));
+      strcpy(*aResult, "nsISupports");

how about:
  static const char isupports_str[] = "nsISupports";
 *aResult = nsMemory::Clone(isupports_str, sizeof(isupports_str));

that avoids the hardcoded 12 :)

+    PRUint32 size = sizeof(kJavaKeywords) / sizeof(*kJavaKeywords);

could use NS_ARRAY_LENGTH here

+    mJavaKeywords = new nsCStringHashSet();
+    mJavaKeywords->Init(1);

Why not make this a normal member, instead of a pointer? Also, wouldn't it be
more efficient to Init() with the total number of java keywords? (Or am I
misunderstanding the Init parameter?)

+    nsIEnumerator* etor;
+    rv = iim->EnumerateInterfaces(&etor);

nsCOMPtr? (some early returns seem to leak etor)

+    static char kHeader[] = "/**\n * NOTE: THIS IS A GENERATED FILE. PLEASE "

maybe mark this |const| too

+    PR_Free(iid_str);

interesting, this doesn't use nsMemory?

+	 output_dir->InitWithPath(NS_ConvertASCIItoUTF16(argv[++i]));

InitWithNativePath?

(I didn't really look at the changes to the tests)

r=biesi

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

>>+      case nsXPTType::T_I8:
>>+        snprintf(buf, 31, "%d", aValue->val.i8);
>
>maybe use |sizeof(buf)-1| instead of |31|?

I want to note... snprintf is documented not to write more than size bytes
including the 0 terminator:
       The functions snprintf and vsnprintf do not write more
       than size bytes (including the trailing ’\0’). 
(from man snprintf)

Comment 21

[timeless]

Comment on attachment 182193 [details] [diff] [review]
patch v1.2

>+  static nsresult GetInterfaceName(nsIInterfaceInfo* aIInfo,


null check alloc (or clone):
>+      *aResult = (char*) nsMemory::Alloc(12 * sizeof(char));
>+      strcpy(*aResult, "nsISupports");

>+      *aResult = (char*) nsMemory::Alloc(12 * sizeof(char));
>+      strcpy(*aResult, "nsISupports");

>+      *aResult = (char*) nsMemory::Alloc(12 * sizeof(char));
>+      strcpy(*aResult, "nsISupports");

>+class Generate
>+  Generate(nsILocalFile* aOutputDir)
>+    : mOutputDir(aOutputDir)
>+  {
null check alloc:
>+    mIfaceTable = new nsCStringHashSet();
>+    mIfaceTable->Init(1);

null check alloc:
>+    mJavaKeywords = new nsCStringHashSet();
>+    mJavaKeywords->Init(1);

do you care if this fails?
>+      mJavaKeywords->Put(nsDependentCString(kJavaKeywords[i]));

null check alloc:
>+    mNoscriptMethodsTable = new nsCStringHashSet();
>+    mNoscriptMethodsTable->Init(1);

do you care if this fails?
>+      mNoscriptMethodsTable->Put(nsDependentCString(kNoscriptMethodIfaces[i]));

null check getter:
>+    nsCOMPtr<nsIInterfaceInfoManager> iim = XPTI_GetInterfaceInfoManager();
>+    rv = iim->EnumerateInterfaces(&etor);

do you care if this fails?
>+    mIfaceTable->Put(nsDependentCString(iface_name));

>+  nsresult OpenIfaceFileStream(const char* aIfaceName,
>+                               nsIOutputStream** aResult)
>+  {
>+    nsresult rv;
>+
>+    // create interface file in output dir
>+    nsCOMPtr<nsIFile> iface_file;
check rv:
>+    rv = mOutputDir->Clone(getter_AddRefs(iface_file));
otherwise you crash on failure:
>+    rv = iface_file->Append(filename);

>+  nsresult WriteMethods(nsIOutputStream* out, nsIInterfaceInfo* aIInfo,
>+                        PRUint16 aParentMethodCount)

if this fails (null out):
>+        aIInfo->GetNameShared(&iface_name);
this dependentcstring will be unhappy:
>+        if (!mNoscriptMethodsTable->Contains(nsDependentCString(iface_name)))

check for failure:
>+        output_dir = new nsLocalFile();
>+        output_dir->InitWithPath(NS_ConvertASCIItoUTF16(argv[++i]));

that's not a very nice handling for oom:
>+  if (NS_FAILED(rv)) {

Comment 22

[jhp (no longer active)]

Attachment: patch v1.3

Updated patch with suggestions.

Comment 23

[jhp (no longer active)]

(In reply to comment #18)
> I think we should consider providing alternate interfaces
> that are java friendly.  Or, maybe these java bindings could provide a
> class or two that wrap these interfaces to support embedding.  I don't
> know... or maybe this special casing is just fine.

What specifically did you have in mind?  I don't think there's much we can do
here.  SWT wants to pass around the actual native window handle (as int or
long), and that's what these methods take.

Comment 24

[jhp (no longer active)]

Also, while I have your attention, should I have this app write out any type of
header for the Java interfaces?  At the moment, I have this:

/**
 * NOTE: THIS IS A GENERATED FILE. PLEASE CONSULT THE ORIGINAL IDL FILE FOR
 * THE FULL DOCUMENTION.
 **/

Do I need to also write the MPL at the top?

Comment 25

[timeless]

generators can't easily know what license the item they're generating is under.
we have that problem w/ bad stuff in intl where it improperly licenses stuff.

gcc and other compilers are nice and properly disavow license tainting of their
output (not counting things you link to), which is what you should do.

possibly:
 * NOTE: THIS IS A GENERATED FILE. PLEASE CONSULT THE ORIGINAL IDL FILE FOR
 * THE FULL DOCUMENTION AND LICENSE.

can you attach an example generated file? i'm too tired to try to figure out if
you give a hint as to the location of the original file (is that useful to people?).

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

personally, I think the original filename would be useful, especially as one IDL
file can map to several .java files. (xpidl also writes out the IDL name in .h
files)

Comment 27

[Mike Shaver (:shaver emeritus)]

The IIM doesn't likely know the filename of the original IDL.  xpidl does
because it has opened that file, and was able to keep it around.  You _might_
find the filename in the comment chunks in the XPT files, but I doubt the IIM
keeps that around and exposes it to callers.

It's a generated file, so you should tell people to look at the original IDL
files for documentation and license.  They can find those files any number of
ways; you might refer them to lxr.mozilla.org.

Comment 28

[jhp (no longer active)]

Attachment: sample - nsIFile.java

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 182214 [details] [diff] [review]
patch v1.3

a=asa

Comment 30

[timeless]

easy question:
might someone want to specify a different package? my hsIFoo really belongs in
com.cenzic, although i can obviously manually munge it later. (if the answer is
that only things under org.mozilla.xpcom are reflected, that's fine, but i
suppose i should know that - and if that's the case, it's probably worth noting
that in the generated file.)

last mostly evil question (for the weekend):

suppose an xpcom installation has
nsIBroken.idl:
[scriptable, uuid(00000000-0000-0000-0000-000000000000)]
interface nsIBroken
{
attribute boolean Broken;
}
compiled as nsIBroken.xpt linked into nsbroken.xpt

and

nsIBroken.idl:
[scriptable, uuid(10000000-0000-0000-0000-000000000000)]
interface nsIBroken
{
attribute boolean broken;
}
compiled as nsIBroken.xpt linked into nsbroken2.xpt

the xpcom installation ships w/ nsbroken.xpt and nsbroke2.xpt (not the original
nsIBroken.xpt's). how well does your code deal with this?

afaik it's possible for objects to run around implementing both of these trivial
interfaces in the single xpcom instance, so at least in theory some poor java
app might want to too. it's perfectly reasonable to punt this to another bug.

Comment 31

[jhp (no longer active)]

Checked in.

Comment 32

[jhp (no longer active)]

(In reply to comment #27)
> you might refer them to lxr.mozilla.org.

Unfortunately, doing an identifier search on the interface name doesn't clearly
show the IDL file;  it's listed somewhere in the Referenced section at the
bottom of the page.  Text and file searches aren't any better.

(In reply to comment #30)
> might someone want to specify a different package?

As you said, I think only interfaces in org.mozilla.xpcom need to be reflected.
 If a developer creates their own interface, it will need to extend a known
XPCOM interface in order for XPCOM/Mozilla to be able to deal with it.

> the xpcom installation ships w/ nsbroken.xpt and nsbroke2.xpt (not the original
> nsIBroken.xpt's). how well does your code deal with this?

My code doesn't handle this at all.  It would create one nsIBroken.java file,
and then overwrite it with the next one.  Is this a valid scenario that I should
be worried about?  Or can we assume that all interfaces will have unique names?

Comment 33

[timeless]

it really can happen. but let's worry about it in another bug :).

Comment 34

[jhp (no longer active)]

-> FIXED

Comment 35

[Mike Shaver (:shaver emeritus)]

> Unfortunately, doing an identifier search on the interface name doesn't clearly
> show the IDL file;  it's listed somewhere in the Referenced section at the
> bottom of the page.  Text and file searches aren't any better.

Yeah, but it's better than nothing, and searching for "interface nsIFoo" does
pretty well.

> (In reply to comment #30)
> > might someone want to specify a different package?
> 
> As you said, I think only interfaces in org.mozilla.xpcom need to be reflected.
>  If a developer creates their own interface, it will need to extend a known
> XPCOM interface in order for XPCOM/Mozilla to be able to deal with it.

Sure, if nothing else they'll extend nsISupports.  But I don't think
ibmIEclipsePluginControl should be in org.mozilla.xpcom.  (In fact, I think
calIDateTime should be in org.mozilla.calendar.)  People create their own
interfaces for script to call into their custom components all the time.  I
think we should let them pick a package for emitting this code.  Can you file on
that?

> My code doesn't handle this at all.  It would create one nsIBroken.java file,
> and then overwrite it with the next one.  Is this a valid scenario that I should
> be worried about?  Or can we assume that all interfaces will have unique names?

I think that's fine for now; XPConnect will have the same issue, and it's
something that we want to be doing hard-stop diagnostics anyway if we get "dups"
of name or IID that are not identical.  We had some late-breaking crash bugs in
a 1.0.x cycle due to an autocomplete interface that was being shipped with a
different IID and same name, or some such.  Another bug, for sure.