270889 - Review of JavaXPCOM code

Status: RESOLVED FIXED

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

Description

[jhp (no longer active)]

The code for JavaXPCOM is already checked in to the tree (in
extensions/java/xpcom), but has not properly been reviewed.  Darin said he would
be gracious enough to take a look.

Comment 1

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

random nit: Typo in XPCOM.java line 108
      // IID method.  In that case, just return an emptry string.
emptry -> empty

Comment 2

[Darin Fisher]

Attachment: Some quick initial comments

Javier,

Here are some thoughts I had on JavaXPCOM at first glance.  I plan on doing a
thorough code review next.

Comment 3

[Darin Fisher]

nsJavaInterfaces.cpp:

 * The AppFileLocProvider problem needs to be solved.  My suggestion was to
   implement a custom wrapper for nsIDirectoryServiceProvider/2.
 
   Another option would be to change initEmbedding to take neither nsIFile nor 
   nsIDirectoryServiceProvider, but instead to take java.io.File and
   java.util.Map or something like that.

   The point is that initEmbedding does not need to perfectly mirror
   NS_InitEmbedding.  The parameters do not have to correspond directly to
   their XPCOM counterparts.  The point of this language binding is to make
   XPCOM easy for Java programmers ;-)

   If you go with my suggestion then you don't have to worry about interfaces
   being used prior to XPCOM initialization.

nsJavaWrapper:

 * It looks like CallXPCOMMethod would crash if GetMethodInfo returned a null
   valued methodInfo.  Perhaps you should return when you encounter an error.

 * Why the XXX about deleting nsID objects?  (This is inside CallXPCOMMethod.)
   XXX usually indicates a known bug or something that should be changed, but
   this seems like it is doing the right thing, no?

 * CreateJavaWrapper has a call to NewGlobalRef that is commented out.  Can it
   just be removed?

 * Support for array type parameters is probably going to be needed at some
   point.  Consider nsIProperties::getKeys, which is frozen.

 * FinalizeParams has an XXX comment.  Any thoughts on that one?

 * FinalizeParams also has an XXX comment in the T_IID case that corresponds
   to the XXX comment in CallXPCOMMethod.  There's also a commented out delete
   call.  Clean up those comments?

 * SetupParams has an XXX comment on T_VOID.  Is that still an issue?  If so,
   do we need to be concerned?  There's some special casing of intClass and
   intArrayClass there.  What is that all about?

nsJavaXPCOMBindingUtils.cpp:

 * InitializeJavaGlobals should just early return if |initialized| is true.
   Then you'll have less indenting in the rest of the function.

 * FreeJavaGlobals is missing an implementation.

 * If you early return from InitializeJavaGlobals with an error then you'll
   be partially initialized.  Maybe that could lead to problems if the 
   InitializeJavaGlobals function is called again?  Maybe call
   FreeJavaGlobals if InitializeJavaGlobals doesn't succeed?

 * JavaXPCOMInstance constructor has some tab chars in it.  Replace tabs
   with space chars.

 * In JavaXPCOMInstance::InterfaceInfo it may be unnecessary to AddRef and
   Release the nsIInterfaceInfoManager instance returned from 
   XPTI_GetInterfaceManager.

 * JavaXPCOMInstance::InterfaceInfo returns an already addref'd interface
   pointer.  When that is done, sometimes it is better to return it using
   the already_AddRefed helper class defined in nsCOMPtr.h.  That way 
   people who call this function do not forget to call Release.

 * indentation of parameters for GetIIDForMethodParam is off.

nsJavaXPTCStub.cpp:

 * You should check for out-of-memory errors.  In particular, if 
   new nsJavaXPTCStub fails, we'll crash.  This comment may apply elsewhere.
   Best to verify the rest of the source.

 * In CallMethod, it looks like you could make use of AppendLiteral or
   the version of Append that takes a single char.  Same comment applies
   to nsJavaXPTCStub::SetupJavaParams.

 * nsJavaXPTCStub::GetRetvalSig has an XXX comment for T_VOID.  Do we need
   to worry about that?

nsJavaXPTCStubWeakRef.cpp:

 * NS_IMPL_THREADSAFE_QUERY_INTERFACE1 should be NS_IMPL_QUERY_INTERFACE1.
   In fact, there is no such thing as "threadsafe" QI.  AddRef and Release
   may need to be threadsafe, however.  For now, I presume that we are
   limiting these Java bindings to main thread only, right?

XPCOMException.java:

 * There's some commented out code here.  Shouldn't it just be removed?

XPCOM.java:

 * There seems to be some special casing of classnames that are prefixed
   with "ns".  That seems odd.  There's no strict requirement for XPCOM
   interfaces to begin with "ns".

 * There's also a XXX comment in getInterfaceIID about using the Java
   logging service.  File a bug on that?  Probably a good idea to file
   bugs on any XXX comments that remain unresolved.

Comment 4

[jhp (no longer active)]

Darin, thanks for taking a look.  Some questions:

> XPCOM.java:
>  * There seems to be some special casing of classnames that are prefixed
>    with "ns".  That seems odd.  There's no strict requirement for XPCOM
>    interfaces to begin with "ns".

In 'getInterfaceIID'?  I took this code from the 'write_classname_iid_define'
function in xpidl_header.c.  It just spits out "NS_" if the interface name
starts with "ns", but also handles the case where it doesn't start with that string.

Also, I remember you saying something about the member functions of GeckoEmbed
and XPCOM classes.  I think you said that GeckoEmbed should only have
init/termEmbedding, and everything else should be in XPCOM, right?  And should I
keep the class name as GeckoEmbed, or make it just Gecko?  Or something else
altogether?

Comment 5

[jhp (no longer active)]

Attachment: first patch of fixes (checked in)

* Changed initEmbedding() to take java.io.File as the first argument.  I also
backed out the changes to lazily discover the interface info (originally done
so initEmbedding() could take nsILocalFile as first param).
* Created a custom Java interface and C++ proxy for the second parameter of
initEmbedding().  A user would need to create a Java class that implements the
new interface, and the C++ proxy knows how to call the getFile() function in
the Java class.
* The GeckoEmbed class now only contains init/termEmbedding.  The rest of the
functions are in the XPCOM class.  Plus, I added the init/shutdownXPCOM and
getComponentRegistrar functions to the XPCOM class.
* Added some JNI helper functions.

Darin, what do you think of this approach?

Comment 6

[Darin Fisher]

from reading your comments, i like the approach.  i'll try to review your patch
tomorrow.

Comment 7

[Darin Fisher]

Comment on attachment 168903 [details] [diff] [review]
first patch of fixes (checked in)

>Index: AppFileLocProvider.java

>+public interface AppFileLocProvider {
>+
>+  public File getFile(String prop, boolean[] persistent);
>+
>+}

NOTE: nsIDirectoryServiceProvide2 has a getFiles method that returns
an enumeration of nsIFile objects.  I think you should provide a 
similar method here.  Otherwise, there is no way for an embedder
to provide values for array-based directory keys.

Otherwise, this patch looks great.  If you want to do getFiles as
a followup patch, that's fine by me.

r=darin

Comment 8

[jhp (no longer active)]

Attachment: nsIDirectoryServiceProvider2 impl

With this patch, there are now two custom Java interfaces which closely mirror
the nsIDirectoryServiceProvider/2 interfaces;  the only difference is that the
return params for the custom ifaces use java.io.File instead of nsIFile.  The
custom C++ proxy class now looks to see if the incoming Java object implements
AppFileLocProvider2; if so, QI returns 'this' when asked for
nsIDirectoryServiceProvider2.  The GetFiles() implementation calls the
corresponding function in the Java object, and creates an nsISimpleEnumerator
wrapper around the result; this wrapper creates C++ nsIFile objects from the
Java File objects.

Comment 9

[jhp (no longer active)]

Attachment: AppendLiteral changes (checked in)

Is this what you had in mind?  This patch increased the lib size by a couple K.
 Shouldn't the changes decrease the size?  Or was I using Append incorrectly
previously, and this patch makes everything correct?

Comment 10

[Darin Fisher]

Comment on attachment 169330 [details] [diff] [review]
nsIDirectoryServiceProvider2 impl

An embedder should really implement both of these interfaces, so I think it
might be better to just put both methods on AppFileLocProvider instead of
trying to mirror the two XPCOM interfaces.  getFiles would be on
nsIDirectoryServiceProvider, but only for historical reasons are there two
separate interfaces.

Comment 11

[Darin Fisher]

Comment on attachment 169330 [details] [diff] [review]
nsIDirectoryServiceProvider2 impl

if you disagree, then re-request review, otherwise post a new patch, and i'll
review that instead.

Comment 12

[jhp (no longer active)]

(In reply to comment #10)
Hmm, Firefox and Mozilla seem to implement both nsIDirectoryServiceProvider and
nsIDirectoryServiceProvider2, but Camino and the ActiveX control only implement
nsIDirectoryServiceProvider.  The question is, should we require a Java embeddor
to implement both of these interfaces?

Comment 13

[Darin Fisher]

Yes, I think so.  Afterall, they can always make getFiles return null if they
don't have any file lists to specify.

Comment 14

[jhp (no longer active)]

Attachment: handle errors properly (checked in)

This patch implements better error handling, for out of memory conditions as
well as others, in XPCOM and in JNI.  I changed the code in ThrowException to
throw a Java OutOfMemoryError if it gets an rv == NS_ERROR_OUT_OF_MEMORY;
otherwise, it throws an XPCOMException.  In the future, it may be worthwhile to
map other comment rv values to the appropriate Java exception/error.

Comment 15

[jhp (no longer active)]

Attachment: nsIDirectoryServiceProvider2 impl v2 (checked in)

Changes per Darin's comments.

Comment 16

[Darin Fisher]

Comment on attachment 170250 [details] [diff] [review]
handle errors properly (checked in)

>Index: nsAppFileLocProviderProxy.cpp

> nsAppFileLocProviderProxy::GetFile(const char* aProp, PRBool* aIsPersistant,
...
>+  if (!prop)
>+    rv = NS_ERROR_OUT_OF_MEMORY;
>   jbooleanArray persistant = mJavaEnv->NewBooleanArray(1);
>+  if (!persistant)
>+    rv = NS_ERROR_OUT_OF_MEMORY;

it seems like some early returns might be nice here instead.


r=darin

Comment 17

[jhp (no longer active)]

The last few checkins mostly address Darin's review comments.  The only thing
still missing is 'array' parameter support, which I've spun off into bug 278133.
 So resolving this as FIXED.