173146 - add support to XPConnect for IDispatch interfaces

Status: VERIFIED FIXED

(Core :: XPConnect, enhancement)

Description

[Steve Adamski (gone)]

investigate and implement better support in XPConnect, for IDispatch interfaces.

Comment 1

[David Bradley]

Attachment: Patch with just XPConnect changes

Comment 2

[David Bradley]

Attachment: Full patch contains everything

I've posted two patches, because I thought it might make it easier for the
reviewers to go through and see what in XPConnect is impacted.

The point of this first review is to insure nothing is busted on XPConnect when
we're building without IDispatch. And to make sure I have the hooks into
XPConnect where I need them. Ideally if this part is correct we can change the
IDispatch implementation without affecting XPConnect and needing reviews for
all the interim checkins. One issue that I've become increasing worried about
is how Windows Media Player implements some of its objects. For instance, if
you have an instance of the player, called A. And you ask A for a property that
returns what looks like another object, say A.B. Well the result of that is the
same object as A. This caused problems with the wrapped native map, because the
QI back would always result in the same pointer value. I've fixed that, but I
think there may be other issues, and it may take some additional work to safely
address this issue across the board

We're shooting to get this in before the freeze. I'll keep a close watch on
things and will act on feedback as soon as I se it.

Feel free to comment on the full patch. I'm entering testing and doing cleanup
work on much of the code. Everything but connection points and thread handling
is in there. Will be working on getting the tests cases up and running next.

Comment 3

[Adam Lock]

The new patch works pretty well for me. I'm able to instantiate objects and call
methods & properties on them. I'll review the IDispatch side of the patch for
any activex issues.

In the meantime it would be worth verifying that xpconnect builds when com is
disabled (by default)

Comment 4

[David Bradley]

Right, I've built on Windows without XPC_IDISPATCH_SUPPORT. I was planning on
building on the Mac as well. Would be nice to try this on Linux as well,
lockedup is currently sick, I need to get it back on its feet with a call to the
help desk.

Comment 5

[Adam Lock]

Comment on attachment 102145 [details] [diff] [review]
Full patch contains everything

I've had a scan of the IDispatch code and listed some of the issues I
encountered. 

IDispatchTearOff::~IDispatchTearOff should release mCOMTypeInfo, not delete it.
IDispTypeInfo's destructor should be made protected.

IDispTypeInfo::GetDocumentation & IDispConvert::JSToCOM call _bstr_t::copy().
It may be better to call _bstr_t::Detach() since it saves an allocate, copy and
delete.

Naming convention, IDispConvert, IDispIDArray etc. look like interfaces but are
actually C++ classes.

IDispConvert should be a service.

IDispatchInterface::Member::ParamInfo::InitializeOutputParam  could cast into
var.byref. I'm concerned about what kind of buffer these out params are happy
with. 

IDispNameArray::SetName & related methods shouldn't mention dispid since it's
an index, not a dispid (which can be arbitrary positive or negative values)

IDispJSObjectExt.h appears to contain public: keyword outside the scope of a
class.

IDispatchObject.cpp #defines VARIANT_SIZE incorrectly. A VARIANT is not just
the vt and the data, it has reserved fields too. 

IDispThrower::ThrowError casts a BSTR into a char *. A BSTR is a Unicode
string, so it can't be cast this way. This might happen elsewhere too, e.g.
converting too/from js strings.

Tabs in IDispTypeInfo.cpp, IDispatchInfo.cpp, DispatchInterface.cpp and more.

Use IsEqualIID for comparing IIDs in IDispTypeInfo::QueryInterface and probably
other impls.

IDispTypeInfo should return S_OK, not NS_OK in methods. Also, probably
E_NOTIMPL is more correct anyway since some of these methods have out
parameters which the caller might assume are filled in if you return S_OK...

SetCOMError in IDispatchTearOff does not need to put the strings in to BSTRs
first, it could just say newError->SetSource(L"@mozilla.org/nsIXPCWrappedJS");

Comment 6

[David Bradley]

Attachment: XPConnect only changes with Adam's comments addressed

Comment 7

[David Bradley]

Attachment: Full patch with Adam's comments addressed

Comment 8

[John Bandhauer]

Here's what I have. I don't see much that gets in the way of landing soon (with
this disabled). The wrappednative GetNewOrUsed stuff (discussed below) bugs me
and is the closest thing I see to being something that *might* have changed
behavior unintentionally.

I'm not crazy about the "IDisp*.*" file naming convention.

I don't like header files that are #included into the body of class declarations.
I don't think this helps at all.

You should purge the tabs (maybe you got them all in the patch addressing Adam's
comments?).

You're obviously a bigger fan of nested classes than I.

I'd try to limit the number of new files by #ifdefing in more small code
bits rather than do some of the bending over backwards that is done. Specific
cases mentioned below.

Comments on specific files...

? src/idisp.msg
I don't think this file should exist - just #ifdef inline this stuff in xpc.msg


? src/IDispatchInfo.cpp
I don't think this file should exist.
I think you should just add an idl declared IDispatch interface. It does not
need to have any declared methods and and no one needs to #include the generated
.h file. If you do this then the typelib loader will just do the right thing to 
give you typeinfo for this interface. Is there a reason to not do this?


? src/IDispatchInterface.cpp
Is the JS_CompareStrings in IDispatchInterface::FindMember really necessary? 
Property names in resolvers are always atomized. Is this called from
non-resolver code?

IDispatchInterface::Member::GetValue has a path where argc is not initialized.


? src/IDispatchTearOff.cpp
BuildMessage and SetCOMError should be file static.


? src/IDispCallContext.cpp
This file is unnecessary - why not merge into xpccallcontext.cpp?

? src/IDispConvert.cpp

unused:
// TODO: We need to look into consolidating all these instances
static NS_DEFINE_IID(kISupportsIID, NS_ISUPPORTS_IID);


? src/IDispErrors.h
I don't think this file should exist - just #ifdef inline this stuff in
nsIXPConnect.idl 


? src/IDispIDArray.cpp
A canditate for merging into some other file?


? src/IDispInlines.h
IDispGUIDEqualsIID et al should just do some casting to nsID and let the compiler
do the work using nsIID methods and the compiler generated struct copying. 
There is no reason to be calling memcmp and memcpy.


? src/IDispJSObjectExt.h
I think you should just combine the IDisp and non-IDisp versions of this. 
You can use an #ifdef for one of the flags and for IDisp specific methods.
Actually, I'm not clear on why this needs to be broken out as a class at all. Why
not just leave it as an implmentation detail of XPCWrappedNativeTearOff and
put the IDisp specific methods on XPCWrappedNativeTearOff itself? Nevertheless,
I think it is ugly and unnecessary to conditionally declare the same class
in different places.

? src/IDispObject.cpp
I have no comment


? src/IDispPrivate.h

two copies of...
#include <atlbase.h>
#include <comdef.h>
...and the #undefs

? src/IDispThrower.cpp
I have no comment


? src/IDispTypeInfo.cpp
IDispTypeInfo::QueryInterface is broken


? src/XPCIDispatchExtension.cpp
I have no comment


M idl/nsIXPConnect.idl
unnecessary changes - just inline IDispErrors.h contents


M src/Makefile.in
I think the new exproted .h file is unnecessary.

For the conditional cpp files why not just:

CPPSRCS		= \
		foo.cpp \
		bar.cpp \
ifdef XPC_IDISPATCH_SUPPORT
		IDispFoo.cpp	\
		IDispBar.cpp	\
endif
		$(NULL)
  

M src/nsXPConnect.cpp
unnecessary change if you decalre IDispatch in dil.


M src/xpcconvert.cpp
I'd rather see XPCWrappedNative::GetNewOrUsed switch on IDispatch rather than
making the callers make the distinction and call different methods.


M src/xpcexception.cpp
unnecessary change if idisp.msg goes away


M src/xpcinlines.h
As noted below DoPreScriptEvaluated, DoPostScriptEvaluated, and
IsReportableErrorCode
could live elsewhere.

I like centralizing the thrower inlines.

I don't like the wrappednative inlines. 


M src/xpcprivate.h
You should use xpcforwards.h for forward class declarations.
See above for comments on XPCJSObjectExt


M src/xpcwrappedjsclass.cpp

re: move of DoPreScriptEvaluated and DoPostScriptEvaluated...
When moving these out of file scope they ought to be renamed to have an "xpc_"
prefix. Also, it is worth noting that they were already being called from enough
places tht their 'inline' status was questionable. I suggest that you leave them
where they were, make them not "inline static", rename them with "xpc_" prefix, and
declare them in xpcprivate.h for use outside of their current file.

Same story for IsReportableErrorCode


M src/xpcwrappednative.cpp
Doing the common factor thing with the code in GetUsedOnly as used by 
GetNewOrUsed is good (though I'm not crazy about the name changes). I don't 
like the addition of the inlines and the renamed *Impl parts. Rather than do 
this inlining how about adding a "PRBool useIdentity" param and then doing:

    nsCOMPtr<nsISupports> identity = useIdentity ? 
                                        do_QueryInterface(Object) : Object;

I think this will be clearer, minimize changes to nonIDisp paths, and allow
you to be explicit about what happens in IDisp code paths.


M src/xpcwrappednativejsops.cpp
I have no comment


M tests/TestXPC.cpp
unnecessary comment change

Comment 9

[David Bradley]

>I'd try to limit the number of new files by #ifdefing in more small code
>bits rather than do some of the bending over backwards that is done. Specific
>cases mentioned below.

That's the way I started out. I changed that, because this was being moved to
the trunk earlier than I had expected and I didn't want to get bogged down in
the r/sr process anytime I wanted to check something in for an IDispatch
implementation. And the impression I got from the meeting before last, is that
anything that touches a file in the build would needed to be r/sr'd even if the
change occured in a #ifdef. 

Comment 10

[John Bandhauer]

> And the impression I got from the meeting before last, is that
> anything that touches a file in the build would needed to be r/sr'd even if the
> change occured in a #ifdef.

We can absolutely streamline the review process for code inside
#ifdef XPC_IDISPATCH_SUPPORT blocks. We're talking honor system here.

Comment 11

[Brendan Eich [:brendan]]

Yeah, mozilla.org is gonna clarify its position on ifdef'd code.  For code
within the "super-review cloud" of in-process Mozilla-the-app-suite sources,
you'll need r= and sr= to add #ifdef FOO to some module, but once given, you can
work out a deal to keep adding it.  To unifdef FOO, or define it permanently,
you'll still need to get the code (in its current form, skipping some thrashing
so as to save on re-review) reviewed and super-reviewed.

/be

Comment 12

[Adam Lock]

Comment on attachment 102267 [details] [diff] [review]
Full patch with Adam's comments addressed

r=[e-mail] on the IDispatch code on the basis that it's disabled
in the regular builds. I can't speak for the XPC side of things but I'd like to
see this in so people can start beating on it.

Comment 13

[David Bradley]

Attachment: Patch containing both jband's and Adam's feedback

Everything should be addressed.

Regarding:
For the conditional cpp files why not just:

CPPSRCS 	= \
		foo.cpp \
		bar.cpp \
ifdef XPC_IDISPATCH_SUPPORT
		IDispFoo.cpp	\
		IDispBar.cpp	\
endif
		$(NULL)

That's what I thought at first, but it doesn't work with gmake. You have to use
the +=.

Comment 14

[Brendan Eich [:brendan]]

I think jband should r= the xpconnect changes.

I skimmed and have only nits at a glance (else-after-return all over, but maybe
that's ok in xpconnect code; doc-comments that really aren't doc-comments --
that lack useful @returns statements, etc.).

/be

Comment 15

[David Bradley]

Attachment: Fixes the return/else and a regression I found in XPCDispIDArray::Item

Return/else was just me reverting back to old style habits. I think I got them
all. The comments still need to be addressed, but I want to leave that for the
next round of changes in this code.

Comment 16

[Adam Lock]

Latest patches are missing XPCIDispatchExtension.h. Also I notice that
mozilla/js/src/xpconnect/idl/Makefile.in should probably ignore XPCIDispatch.idl
when XPC_IDISPATCH_SUPPORT is not defined.

Comment 17

[David Bradley]

Attachment: Removed #include

I didn't conditionally compile the nsIDispatch.idl, because there appears to be

no way to handle things in the MANIFEST file conditionally. I figured this
interface was of little danger, so left it in. If people feel otherwise, I'll
move it to its own directory and conditionally compile that directory. Also
from what I saw, people have tried to conditionally compile things in IDL were
using %{C++} and sticking in #ifdef/#endif, but that doesnt' really exclude the
interface definition, just
the generated header file.

XPCIDispatchExtension.h shouldn't exist. Turns out it was empty on my system,
and that let the #include succeed. I removed the #include in this latest patch.

Comment 18

[John Bandhauer]

I didn't look close at any of the #ifdef'd code. This will all need close review 
before being turned on by default.


+interface IDispatch : nsISupports
+{
+    void Dummy();
+};
Why do you need a Dummy method here?


+#define NS_ERROR_XPC_COM_UNKNOWN                       GENERATE_XPC_FAILURE(1000)
+#define NS_ERROR_XPC_COM_ERROR                         GENERATE_XPC_FAILURE(1001)
Why the high numbers? When may want contiguous numbers some day (to fill in an
offset lookup table or something). How about just continuning the sequence?


re: XPCDispPrivate.h

Do you really want to be copying in so much of the xpcom macro stuff? Are not 
addref and release sigs the same for MS and NS? Why not just add new MSCOM QI 
macros and use the existing NS macros for the rest of it? An I missing something?
But really I guess I don't care.


re: XPCIDispatchExtension.cpp

Tab alert. Having tabs is bad. Having tabs whose use is not consistent with the
emacs Mode line is not allowed.


re: xpcprivate.h

+
+#ifdef XPC_IDISPATCH_SUPPORT
+public:
+    static PRBool IsIDispatchEnabled();
+#endif
 };

xpconnect has the convention of putting the data declarations at the end of 
classes. Can you put the inlined method before the data declarations?


+JSExceptionState* XPCDoPreScriptEvaluated(JSContext* cx);
+void XPCDoPostScriptEvaluated(JSContext* cx, JSExceptionState* state);
+JSBool XPCIsReportableErrorCode(nsresult code);

The conventional prefix for global xpc functions is "xpc_".


I like what you did in xpcwrappednative.cpp better than what you had. It is
still pretty cryptic. How about...

    nsCOMPtr<nsISupports> identity;

#ifdef XPC_IDISPATCH_SUPPORT
    // XXX [Note here about why you are going to end up with multiple map
    //      entries for the same objects and what possible bad side effects
    //      that may or may not cause]
    if(nsXPConnect::IsIDispatchEnabled() &&
Interface->GetIID()->Equals(NSID_IDISPATCH))
        identity = Object;
    else
#else
        // The normal case...
        identity = do_QueryInterface(Object);
#endif

    if(!identity)
    {
        NS_ERROR("This XPCOM object fails in QueryInterface to nsISupports!");
        return NS_ERROR_FAILURE;
    }



-                return GetNewOrUsed(ccx, Object, betterScope, Interface,
+                return GetNewOrUsed(ccx, identity, betterScope, Interface,
Agreed - We don't actually need Object here in the nested call.


I'd urge you to make the small changes suggested. I'm happy with the minimal
effect this patch now has with the #ifdef off. I don't see *any* changes that
will impact the default build.

Comment 19

[David Bradley]

Attachment: Same as before but with jband's suggestions added

Comment 20

[John Bandhauer]

Comment on attachment 102904 [details] [diff] [review]
Same as before but with jband's suggestions added

r/sr=jband to land this on the trunk with IDispatch support disabled.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 102904 [details] [diff] [review]
Same as before but with jband's suggestions added

>+++ xpconnect/idl/XPCIDispatch.idl	15 Oct 2002 00:54:24 -0000
>@@ -0,0 +1,49 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ *
>+ * The contents of this file are subject to the Netscape Public
>+ * License Version 1.1 (the "License"); you may not use this file
>+ * except in compliance with the License. You may obtain a copy of
>+ * the License at http://www.mozilla.org/NPL/

New files should use the MPL/GPL/LGPL, not the NPL/GPL/LGPL.  Please change to
use the boilerplate at
http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c.

>+++ xpconnect/src/Makefile.in	15 Oct 2002 00:54:24 -0000
>@@ -82,8 +82,16 @@
> 		xpcwrappednativeinfo.cpp \
> 		xpcwrappednativejsops.cpp \
> 		xpcwrappednativeproto.cpp \
>-		xpcwrappednativescope.cpp \
>-		$(NULL)
>+		xpcwrappednativescope.cpp
>+ifdef XPC_IDISPATCH_SUPPORT
>+CPPSRCS +=	XPCDispObject.cpp	\
>+			XPCDispInterface.cpp	\
>+			XPCDispConvert.cpp \
>+			XPCDispTypeInfo.cpp	\
>+			XPCDispTearOff.cpp \
>+			XPCIDispatchExtension.cpp

Indentation is whacked, use Unix-compatible (8-space-equivalent) tabstops.

>@@ -107,6 +115,10 @@
> ifdef MOZ_XPCTOOLS
> DEFINES         +=  -DXPC_TOOLS_SUPPORT
> REQUIRES	+= xpctools
>+endif
>+
>+ifdef XPC_IDISPATCH_SUPPORT
>+DEFINES			+=	-DXPC_IDISPATCH_SUPPORT
> endif

Ditto.

>+++ xpconnect/src/XPCDispConvert.cpp	15 Oct 2002 00:54:24 -0000
>@@ -0,0 +1,313 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ *
>+ * The contents of this file are subject to the Netscape Public
>+ * License Version 1.1 (the "License"); you may not use this file
>+ * except in compliance with the License. You may obtain a copy of
>+ * the License at http://www.mozilla.org/NPL/

Use MPL/GPL/LGPL, again.

>+        if(JS_GetElement(ccx, obj, index, &val) &&
>+           JSToCOM(ccx, val, arrayVar, err))
>+        {
>+            SafeArrayPutElement(array, &index, &arrayVar);
>+        }
>+        else
>+        {
>+            if(err == NS_OK)
>+                err = NS_ERROR_FAILURE;

Just a dumb question: shouldn't the side effectors (JSToCOM in this case) set
err to a failure code in all false-return cases?

>+JSBool XPCDispConvert::JSToCOM(XPCCallContext& ccx,
>+                              jsval src,
>+                              VARIANT & dest,
>+                              uintN& err)
>+{
>+    err = NS_ERROR_XPC_BAD_CONVERT_NATIVE;
>+    if(JSVAL_IS_STRING(src))
>+    {
>+        JSString* str = JSVAL_TO_STRING(src);
>+        if(!str)
>+            return JS_FALSE;
>+
>+        jschar * chars = JS_GetStringChars(str);
>+        if(!chars)
>+            return JS_FALSE;
>+
>+        CComBSTR val(chars);
>+        dest.vt = VT_BSTR;
>+        dest.bstrVal = val.Detach();
>+    }
>+    else if(JSVAL_IS_INT(src))
>+    {
>+        dest.vt = VT_I4;
>+        dest.lVal = JSVAL_TO_INT(src);
>+    }
>+    else if(JSVAL_IS_DOUBLE(src))
>+    {
>+        dest.vt = VT_R8;
>+        dest.dblVal = *JSVAL_TO_DOUBLE(src);
>+    }
>+    else if(JSVAL_IS_OBJECT(src))
>+    {
>+        JSObject * obj = JSVAL_TO_OBJECT(src);
>+        if(JS_IsArrayObject(ccx, obj))
>+        {
>+            return JSArrayToCOMArray(ccx, obj, dest, err);
>+        }
>+        else
>+        {
>+            // only wrap JSObjects
>+            IUnknown * pUnknown;
>+            XPCConvert::JSObject2NativeInterface(
>+                ccx, 
>+                (void**)&pUnknown, 
>+                obj, 
>+                &NSID_IDISPATCH,
>+                nsnull, 
>+                &err);

Nit: Indent to underhang the first param, which goes on the same line as the
callee?

>+            dest.vt = VT_DISPATCH;
>+            pUnknown->QueryInterface(IID_IDispatch, NS_REINTERPRET_CAST(void**,&dest.pdispVal));
>+        }
>+    }
>+    else if(JSVAL_IS_BOOLEAN(src))
>+    {
>+        dest.vt = VT_BOOL;
>+        dest.boolVal = JSVAL_TO_BOOLEAN(src);
>+    }
>+    else if(JSVAL_IS_VOID(src))
>+    {
>+        dest.vt = VT_EMPTY;
>+    }
>+    else if(JSVAL_IS_NULL(src))
>+    {
>+        dest.vt = VT_NULL;
>+    }
>+    else  // Unknown type
>+    {
>+        return JS_FALSE;
>+    }

Clear err here?

It seems this method can't return false with err == NS_OK, so fix the earlier
bloat in the caller, which tested for err == NS_OK and set err =
NS_ERROR_FAILURE.

>+    return JS_TRUE;
>+}

BTW, COMArrayToJSArray is more like it: set the err uintN& ref param iff
returning false.

>+++ xpconnect/src/XPCDispInlines.h	15 Oct 2002 00:54:24 -0000
>@@ -0,0 +1,424 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ *
>+ * The contents of this file are subject to the Netscape Public
>+ * License Version 1.1 (the "License"); you may not use this file
>+ * except in compliance with the License. You may obtain a copy of
>+ * the License at http://www.mozilla.org/NPL/

MPL/... again.

>+inline
>+jsval XPCDispIDArray::Item(JSContext* cx, PRUint32 index) const
>+{
>+    jsval val;
>+    if (!JS_IdToValue(cx, NS_REINTERPRET_CAST(jsid, 
>+                                         mIDArray.ElementAt(index)), &val))

Break after cx, indent the second param to underhang cx, make it fit on one
line if possible, else its 2nd param should underhang its 1st, etc.

>+++ xpconnect/src/XPCDispInterface.cpp	15 Oct 2002 00:54:24 -0000
>@@ -0,0 +1,262 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ *
>+ * The contents of this file are subject to the Netscape Public
>+ * License Version 1.1 (the "License"); you may not use this file
>+ * except in compliance with the License. You may obtain a copy of
>+ * the License at http://www.mozilla.org/NPL/

MPL again, and so on below.

rs=[e-mail] (taking over jband's sr=, which shoulda been r= :-). 
Pitch it to drivers.

/be

Comment 22

[Mike Shaver (:shaver emeritus)]

Comment on attachment 102904 [details] [diff] [review]
Same as before but with jband's suggestions added

a=shaver for the 1.2beta trunk.

Comment 23

[David Bradley]

Attachment: Patch that was applied to trunk

Patch checked in

I put in the new license template and the other things Brendan mention.

Comment 24

[Steve Adamski (gone)]

milestone changed to mozilla1.2alpha

Comment 25

[Steve Adamski (gone)]

Arrgh! I mean mozilla1.3alpha.

Comment 26

[David Bradley]

Attachment: Adds support for parameterized properties and VARIANT/jsval conversions

I made a few changes to XPConnect. MarkForValidWrapper static designation was
removed, and renamed to XPCMarkForValidWrapper, I use this in the parameterized
property logic.

These are all IDispatch only stuff in xpconnect files:
I added additional IDispatch only cleanup logic of the tearoff to
XPCWrappedNative
Added logic to register the nsIDispatchSupport service in xpcmodule.cpp
Added another class to xpcforwards.h (IDispatch only)
Added initialization of mIDispatchMember in XPCCallContext.

The rest of the patch deals purely with IDispatch files

Comment 27

[David Bradley]

Attachment: Same as previous patch, but with some cleanup

Renamed the function properly to xpc_MarkForValidWrapper. Also cleaned up some
memory leaks I recently found.

Comment 28

[Michael Buckland]

Bulk adding topembed keyword.  Gecko/embedding needed.

Comment 29

[David Bradley]

Attachment: Same as before but has property cleanup logic

Going to obsolete the other patches. This has the proper cleanup logic for the
XPCDispInterface::Member's. This required adding some logic to the destructor
of the existing XPConnect tearoff.

Comment 30

[Adam Lock]

Comment on attachment 104380 [details] [diff] [review]
Same as before but has property cleanup logic

I went quickly through the IDispatch side of things and identified a few
issues.

nsIDispatchSupport.idl specifies native JSVal but uses JSVAL and jsval.

XPCDispInlines.h, this looks very weird  - "mMembers[index].Member::~Member()".
If this is an array why can't you delete [] it and have the members destruct
themeselves. Is this even legal? 

XPCDispInterface.cpp CaseInsensitiveCompare calls _mbsnbicmp. BSTRs and OLESTRs
are wide-strings so it should be using _wcsnicmp. Multi-byte strings are
something totally different altogether.

XPCDispObject::Dispatch doesn't ask for exception information from the call to
Invoke. Does it need to?

XPCDispParams::InsertParam and friends are not using VariantCopy to copy
VARIANTs around. This is recommended as it ensures reference counting and other
features are correctly maintained. This of course means, that VariantClear()
would also have to be called during destruction of these arrays. Alternatively
use _variant_t or CComVariant wrappers.

nsDispatchSupport::GetSingleton calls NS_IF_ADDREF twice. Is that correct? The
static "instance" should be a static member variable of the class.

Comment 31

[David Bradley]

>nsIDispatchSupport.idl specifies native JSVal but uses JSVAL and jsval.

Oh, ick, thanks for that catch.

>XPCDispInlines.h, this looks very weird  - "mMembers[index].Member::~Member()".
>If this is an array why can't you delete [] it and have the members destruct
>themeselves. Is this even legal? 

This was a convention used in existing XPConnect code to avoid yet another
allocation. The array lives off the end of the XPCDispInterface object. In the
original XPConnect code it was a little simpler because the members consisted of
POD's. In my case, I have to explicity call the destructor on the items so they
get cleaned up. And yes it's legal C++.

>XPCDispInterface.cpp CaseInsensitiveCompare calls _mbsnbicmp. BSTRs and OLESTRs
>are wide-strings so it should be using _wcsnicmp. Multi-byte strings are
>something totally different altogether.

Right, I'll change that.

>XPCDispObject::Dispatch doesn't ask for exception information from the call to
>Invoke. Does it need to?

I opted for getting the exception information via GetErrorInfo call.

>XPCDispParams::InsertParam and friends are not using VariantCopy to copy
>VARIANTs around. This is recommended as it ensures reference counting and other
>features are correctly maintained. This of course means, that VariantClear()
>would also have to be called during destruction of these arrays. Alternatively
>use _variant_t or CComVariant wrappers.

Thanks, will clean that up.

>nsDispatchSupport::GetSingleton calls NS_IF_ADDREF twice. Is that correct? The
>static "instance" should be a static member variable of the class.

It only calls it twice the first time. In that first call case, it is addref'd
twice. Once for nsDispatchSupport and a second time for the caller. Subsequent
calls addref only once.

Yes it could be static member in the class, but I figured why waist the CPU
cycles to have this variable initialized on DLL load rather than when it's
actually called for.

Comment 32

[John Bandhauer]

I guess you have another patch coming.

The non-ifdef'd changes look pretty minimal. I wouldn't mind if you built
separate patches for files in and out of the default build to make it easier for
us to look at non-ifdef'd changes - since that is really what is being closely
reviewed for checkin (though Adam is clearly looking at everything).

FWIW, I'd prefer to not see extern method declarations in .cpp files. It is
better to put them in xpcprivate.h and let the compiler detect possible problems
than to rely on the linker to notice mismatches that may occur if changes are
made...

+// Comes from xpcwrappednativeops.cpp
+void
+xpc_MarkForValidWrapper(JSContext *cx, XPCWrappedNative* wrapper, void *arg);

Otherwise, I'm not seeing anything in the non-ifdef'd parts that raises any flags.

Comment 33

[Chris Nalls]

Marking topembed+ as per topembed triage.

Comment 34

[David Bradley]

Attachment: Revised patch (XPConnect only and no whitespace changes)

Comment 35

[David Bradley]

Attachment: Revised full patch

Comment 36

[Adam Lock]

Comment on attachment 104650 [details] [diff] [review]
Revised full patch

r=adamlock

Comment 37

[Johnny Stenback (:jst)]

Comment on attachment 104649 [details] [diff] [review]
Revised patch (XPConnect only and no whitespace changes)

- In XPCWrappedNativeTearOff::DeleteIDispatchInfo():

+    delete GetIDispatchInfo();
+    mJSObject = nsnull;

I'd like to see a comment here explaining that setting mJSObject to nsnull will
clear the IDispatch info.

- In xpcwrappednative.cpp:

@@ -937,6 +937,10 @@
	     if(to->GetJSObject())
	     {
		 JS_SetPrivate(ccx, to->GetJSObject(), nsnull);
+#ifdef XPC_IDISPATCH_SUPPORT
+		 if(to->IsIDispatch())
+		     delete to->GetIDispatchInfo();
+#endif
		 to->SetJSObject(nsnull);
	     }

Wouldn't this be better written as:

@@ -937,6 +937,10 @@
	     if(to->GetJSObject())
	     {
		 JS_SetPrivate(ccx, to->GetJSObject(), nsnull);
+#ifdef XPC_IDISPATCH_SUPPORT
+		 if(to->IsIDispatch())
+		     to->DeleteIDispatchInfo();
+		 else
+#endif
		 to->SetJSObject(nsnull);
	     }

That way you don't sprinkle introduce duplicate call-sites where you delete the
IDispatch info... No big deal though, I'll deal either way...

Other than that, sr=jst

Comment 38

[David Bradley]

Latest patch checked in

Comment 39

[David Bradley]

Attachment: Another round, full patch

This patch adds a bunch of comments, has some fixes for cleaning up some
things. Unfortunately I needed to add some XPConnect error codes so it touches
XPConnect and I added a XPCThrower method to handle both a COM HRESULT and an
nsresult value, to get more than just the basic COM error message I had before.


I'll post an XPConnect only patch as well.

Since the review is next week. We can do one of two things. If I can get a
quick r/sr on this for the XPConnect stuff I'll check it in. Otherwise the
reviewers can review this patch along with the existing IDispatch code. I'll
leave it up to the reviewers as to what they find easier.

Comment 40

[David Bradley]

Attachment: XPConnect only changes

Comment 41

[David Bradley]

Attachment: Updated full patch, contains conversion for DATE and a few other things

This is a full patch. The XPConnect only changes haven't changed.

Comment 42

[Adam Lock]

Comment on attachment 106687 [details] [diff] [review]
Updated full patch, contains conversion for DATE and a few other things

r=adamlock with some minor nits.

COMToJS should probably explicitly call and test
_variant_t::ChangeType(VT_BSTR) to turn the VT_DATE into a string.

XPCDispNameArray::SetName & GetName should assert when dispid is 0.

In XPCDispatchTearOff::GetCOMTypeInfo, using NS_ADDREF / NS_IF_ADDREF to call
AddRef on a COM object seems a little dangerous.

BuildFuncDesc should probably memset the struct unless it is explicitly setting
every single member.

XPCDispTypeInfo uses CComBSTR when _bstr_t makes it more consistent with
elsewhere.

Comment 43

[David Bradley]

>COMToJS should probably explicitly call and test
>_variant_t::ChangeType(VT_BSTR) to turn the VT_DATE into a string.

Oh right, good catch. This throws an exception which in Mozilla will just crash,
IIRC.

>XPCDispNameArray::SetName & GetName should assert when dispid is 0.

Yes, good idea.

>In XPCDispatchTearOff::GetCOMTypeInfo, using NS_ADDREF / NS_IF_ADDREF to call
>AddRef on a COM object seems a little dangerous.

I recently put this in. I believe jband was suggesting to use existing NS macros
where appropriate. I'll remove this to be safe.

>BuildFuncDesc should probably memset the struct unless it is explicitly setting
>every single member.

I'll have to check.

>XPCDispTypeInfo uses CComBSTR when _bstr_t makes it more consistent with
>elsewhere.

I've been trying to stick with the CComXXXX classes rather than _variant_t and
_bstr, although I violated that to get some of the easy conversions that
_variant_t provided. The reason was that I'm pretty sure that _variant_t/_bstr_t
needs an additional DLL. And outside of my code and your code I don't see this
used, but I did see the CComXXXX versions used elsewhere. My fear is that we
might break if a user didn't happen to have that DLL on there system. I have no
idea how likely that is to occur. I suspect it's probably shipped standard with
Windows now, but don't know that for sure.

Comment 44

[David Bradley]

patch checked in (late notice)
that is the patch dated 11/18/02
http://bugzilla.mozilla.org/attachment.cgi?id=106687&action=edit
It was given an sr= by the review committee jst, rpots, jband.
I'll have a patch with the results of the code review shortly.

Comment 45

[David Bradley]

Attachment: Patch with changes from review feedback

This patch contains the changes resulting from the code review.
I made one major change, and that is to not support wrapping arbitrary
JSObject's. There were potential security concerns in doing this. Rather than
deal with those at this point, I turned this off. I put off refactoring the
exception logic in the invoke on that side as well. It will still unwrap
wrapped IDispatch natives, so we can pass them around. So far we haven't
encountered a concrete need for this in any of the ActiveX controls we've
encountered. We can deal with it when the need arrises.

Also the in/out parameters aren't as robust as I had hoped. COM doesn't support
a lot of conversions for byref variants. We already have issues with the JS
syntax for in/out and out parameters anyway, so I didn't want to spend a lot of
effort here yet.

Comment 46

[David Bradley]

Comment on attachment 107678 [details] [diff] [review]
Patch with changes from review feedback

I'll get used to this new review thing yet.

Comment 47

[David Bradley]

Comment on attachment 107678 [details] [diff] [review]
Patch with changes from review feedback

I'll get used to this new review thing yet.

Comment 48

[David Bradley]

Comment on attachment 107678 [details] [diff] [review]
Patch with changes from review feedback

I'll get used to this new review thing yet.

Comment 49

[David Bradley]

Attachment: Minor update to the previous patch that chagned the "guid" conversion functions

This patch is the same except that I changed the "guid" conversion functions to
use the proper MS types and renamed them appropriately. So the changes were in
XPCDispInlines.h and XPCDispTearOff.cpp.

Comment 50

[Johnny Stenback (:jst)]

Comment on attachment 107925 [details] [diff] [review]
Minor update to the previous patch that chagned the "guid" conversion functions

- In nsDispatchSupport::CreateInstance():

+    const nsPromiseFlatString & flat = PromiseFlatString(className);
+    CComBSTR name(className.Length(), flat.get());

className is a const nsAString& here, which means that in theory it could be a
multi-fragmented string, and thus getting the length out of that is potentially
more expensive than getting it out of the flat string you just created. IOW,
use flat.Length() here.

+JSBool XPCConvert::GetNativeInterfaceFromJSObject():

+    nsISupports* iface;

This is used as a pointer to the identity object only AFAICT, wouldn't it make
more sense to name this 'identity'?

...
+    if(GetISupportsFromJSObject(ccx, src, &iface))
+    {
+	 if(iface)
+	     return NS_SUCCEEDED(iface->QueryInterface(*iid, dest));
+	 return JS_FALSE;
+    
+    }
+    return JS_FALSE;

Couldn't you eliminate the nested if there by checking that iface (or,
identity, rather :-), is non-null after the call to GetISupportsFromJSObject()?
If nothing else, you should at least eliminate the nested return since it's not
needed. I realize you're only moving this code, but still :-)

- In XPCDispConvert.cpp:

@@ -56,23 +56,29 @@

-    if(JSVAL_IS_OBJECT(val))
+    if(JSVAL_IS_PRIMITIVE(val))
     {
...
+    else
     {
+	 if(JSVAL_IS_OBJECT(val))

If a jsval is not primitive, it's a non-null JSObject, therefore this check is
not needed.

...
     }
-    if(JSVAL_IS_NULL(val))
+    if(JSVAL_IS_BOOLEAN(val))

Booleans are primitives, this should be moved inside the if
(JSVAL_IS_PRIMITIVE()) check.

- In XPCDispObject.cpp:

	     if(!JS_IdToValue(ccx, 1, &val))
+	     {
+		 // This shouldn't fail
+		 NS_ERROR("JS_IdToValue failed in
XPCDispParamPropJSClass::NewInstance");
+		 XPCThrower::Throw(NS_ERROR_UNEXPECTED, ccx);
		 return JS_FALSE;
+	     }

As your comment says, it shouldn't fail, and if you look at the implementation,
it never does fail. Maybe just assert here and not waste code for something
that will never happen?

+    if(!GetMember(ccx, funobj, iface, member))
+	 NS_ERROR("GetMember faild in XPC_IDispatch_CallMethod");
+    ccx.SetIDispatchInfo(iface, member);

(this appears in two places). I realize the optimizers would probably take out
that unnecessary if check in optimized code, but wouldn't this be cleaner this
way:

#ifdef DEBUG
    PRBool ok =
#endif
    GetMember(ccx, funobj, iface, member);

    NS_ASSERTION(ok, "GetMember faild in XPC_IDispatch_CallMethod");


- In XPCDispPrivate.h:

+    /**
+     * This is used by Find when it doesn't find something
+     * @see XPCDispNameArray::Find
+     */
+    static nsString mEmpty;

I don't know off-hand what the naming convention for static members is in
XPConnect, but normally in Mozilla code is sFoo, not mFoo. Adjust accordingly
unless there's precendence for this scheme in XPConnect code.

     /**
+     * Don't allow copying
+     */
+    XPCDispParams(const XPCDispParams & other) {
+	 NS_ERROR("XPCDispParams can't be copied"); }

Why not leave this un-implemented and let the compiler catch this?

- In BuildMessage():

+    result = nsCString();

No need to construct an empty string object here, just call result.Truncate().

+    // TODO: This exception logic needs to be refactored and shared with
wrapped JS

Is there a bug on file for doing this?

- In XPCDispTypeInfo.cpp:

>+// There's no CPP for XPCDispIDArray so it's just plopped here
>+nsString XPCDispNameArray::mEmpty;

The comment talks about XPCDispIDArray, but the code says XPCDispNameArray?

@@ -114,20 +117,21 @@
					      JSObject* obj, jsval val) : 
     mPropertyType(INVALID), mMemID(memid)
 {
-    const char* chars = xpc_JSString2Char(cx, val);
+    jschar * chars = xpc_JSString2String(cx, val);
     if(!chars)
	 return;

     mName = chars;
+    nsCAutoString name = NS_LossyConvertUCS2toASCII(mName);
     JSBool found;
     uintN attr;
     // Get the property's attributes, and make sure it's found and enumerable 
-    if(!JS_GetPropertyAttributes(cx, obj, chars, &attr, &found) || 
+    if(!JS_GetPropertyAttributes(cx, obj, name.get(), &attr, &found) || 
	 !found || (attr & JSPROP_ENUMERATE) == 0)
	 return;

     // Retrieve the property 
-    if(!chars || !JS_GetProperty(cx, obj, chars, &mProperty) ||
+    if(!chars || !JS_GetProperty(cx, obj, name.get(), &mProperty) ||
	 JSVAL_IS_NULL(mProperty))
	 return;

This can in theory do the wrong thing. Don't convert to ASCII, use
JS_GetUCPropertyAttributes() and JS_GetUCProperty(). Get the length out of the
incoming jsval so that you don't trip on embedded nulls. This code would be
faster that way too.

- In XPCWrappedNativeTearOff::GetJSObject():

+    XPCDispInterface * iface = GetIDispatchInfo();
+    if(IsIDispatch() && iface)
+	 return iface->GetJSObject();
 #endif
+    return mJSObject;

Given that this method is called quite frequently, wouldn't it be slightly
faster written as:

+    if(IsIDispatch()) {
+	 XPCDispInterface * iface = GetIDispatchInfo();
+	 if (iface)
+	     return iface->GetJSObject();
+	 }
+    }
 #endif
+    return mJSObject;

And what if IsIDispatch() is true but iface is null? Then we'll return
mJSObject with the IDISPATCH_BIT set? Doesn't seem right to me... Or can iface
never be null if IsIDispatch() returns true? If so, then no need for the null
check...

sr=jst if the above gets taken care of.

Comment 51

[Brendan Eich [:brendan]]

Pushing the limits of contingent super-review+ -- I personally would prefer (as
a lurking SR) to see a new patch addressing all of jst's comments.  Not that my
two cents matter; I'm just begging for a final patch.

/be

Comment 52

[David Bradley]

Attachment: Previous patch with jst's changes

The only thing I left in was the implementation of the copy constructor and
assignment operator for XPCDispParams. I've seen compilers/linkers in some
cases that when no implementation wasn't present would produce link errors even
when they were not used. It's just a habit I've developed with this pattern and
given these are inlines they don't cost anything code wise, only some time for
the compiler.

I changed mEmpty to sEmpty as that's my preference. There were instances of g,
m, and s used within the existing XPConnect code. g was the predominate one,
but I really prefer s, as I always associate g with something declared outside
any class/struct.

Comment 53

[Johnny Stenback (:jst)]

Comment on attachment 108030 [details] [diff] [review]
Previous patch with jst's changes

I'm fine either way re the copy ctor of XPCDispParams, but we already have copy
ctors w/o implementation, see XPCCallContext, and there's a ton of default
ctors w/o implementations in XPConnect too, see XPCWrappedNativeScope,
XPCJSRuntime, ... Therefore we don't need to worry about compilers/linkers
having issues with this particular one, if we ever need to build on such a
platform we'll need to deal, but I don't see that happening...

Either way, sr=jst

Comment 54

[Adam Lock]

Comment on attachment 108030 [details] [diff] [review]
Previous patch with jst's changes

This is a massive patch, but here are some of the issues I've found going over
it. They're not very major but some probably cause leakage.

nsDispatchSupport::CreateInstance It shouldn't be necessary to pass the length
into CComBSTR since the classname will be null terminated won't it?

No call to SafeArrayUnaccessData to balance the call to SafeArrayAccessData.

Don't you want to call SafeArrayCreateVector with len - index elements?

Is xpc_CopyVariantByRef necessary, doesn't VariantCopy or VariantCopyInd work?

XPCDispConvert::COMToJS VT_INT & VT_I4 both take from plVal, when the int
should come from pintVal. Same issue for UINT, UI4

VT_BOOL in same method will contain a VARIANT_TRUE or a VARIANT_FALSE. These do
not map onto 1 and 0 so the conversion should be explicit, e.g. (boolVal ==
VARIANT_TRUE) ? JS_TRUE : JS_FALSE

"while(enumCATID->Next" should take NULL, not nsnull though it shouldn't make a
difference

"@@ -372,35 +376,37 @@" No calls to "delete params" in a couple of the return
conditions.

"mInserted = PR_TRUE;" Appears in an #ifdef block but the NS_ASSERTION above is
outside. It should also be inside for consistency even it does compile away to
nothing in release mode.

"mRefCnt = 1; /* stabilize */" I don't understand what this is for. Couldn't
this cause Release to do the wrong thing if it's called twice?

"const VARIANT & GetParamRef()", it's probably safer to not return VARIANTS
like this in future since it's problematic from a refcounting / copy point of
view.

"static inline PRUint32 RefBufferSize". The inline keyword shouldn't be
necessary if this is declared in the scope of the class

"result.Truncate();" shouldn't be necessary since the next line assigns the
string a new value.

"nsString XPCDispNameArray::sEmpty;". Would a named literal string be more
appropriate?

xpcprivate.h #includes and #undefines some MS stuff. Doesn't XPCDispPrivate.h
already do this?

XPCThrower::ThrowCOMError doesn't appear to free bstrDesc & bstrSource. Perhaps
they can be made into _bstr_t

Comment 55

[David Bradley]

>nsDispatchSupport::CreateInstance It shouldn't be necessary to pass the length
>into CComBSTR since the classname will be null terminated won't it?

It was done for two reasons. One, it should be more efficient, since CComBSTR
doesn't have to scan for the null terminator. And it avoids the issue of
embedded nulls, but I would highly doubt we'll get embedded nulls here.

>Is xpc_CopyVariantByRef necessary, doesn't VariantCopy or VariantCopyInd work?

VariantCopyInd copies the source if it's byref or not, to a non byref variant. I
need to copy to a variant that is byref. I tried a simple test program just to
make sure VariantCopy/VariantCopyInd didn't do the right thing, the docs were a
little unclear. But the copy can never be byref.

>"mRefCnt = 1; /* stabilize */" I don't understand what this is for. Couldn't
>this cause Release to do the wrong thing if it's called twice?

This is macro code was copied directly from the XPCOM stuff. So I'm pretty
trusting. I don't claim to fully understand what's going on either.

>"const VARIANT & GetParamRef()", it's probably safer to not return VARIANTS
>like this in future since it's problematic from a refcounting / copy point of
>view.

Actually this is no longer needed.

>"nsString XPCDispNameArray::sEmpty;". Would a named literal string be more
>appropriate?

Possibly, but I wanted a default constructed string, rather than an "empty"
string. This reminds me this should be const.

>xpcprivate.h #includes and #undefines some MS stuff. Doesn't XPCDispPrivate.h
>already do this?

Yes, unfortunately I needed EXCEPINFO declaration in xpcprivate.h, and that can
be included without xpcdispprivate.h being included. EXCEPINFO is a typedef, so
I couldn't just forward declare it :-(

>XPCThrower::ThrowCOMError doesn't appear to free bstrDesc & bstrSource. Perhaps
>they can be made into _bstr_t

It does, but not on all paths.

Items not commented here, will be addressed. I'll get jst's offline style stuff
in as well.

Comment 56

[David Bradley]

>"@@ -372,35 +376,37 @@" No calls to "delete params" in a couple of the return
>conditions.

Note the comment. I'll convert this to a auto pointer scheme once dbaron
finishes it. That would save a lot of grief.
+        // NewInstance takes ownership of params

Comment 57

[David Bradley]

Attachment: Patch to address Adam's comments, and some additional formatting cleanup from jst

Comment 58

[Johnny Stenback (:jst)]

Comment on attachment 108529 [details] [diff] [review]
Patch to address Adam's comments, and some additional formatting cleanup from jst

sr=jst

Comment 59

[Adam Lock]

Comment on attachment 108529 [details] [diff] [review]
Patch to address Adam's comments, and some additional formatting cleanup from jst

r=adamlock, except for bstrSource & bstrDesc still look like they will leak.
Declare these as _bstr_t. They can still be passed to GetSource &
GetDescription and they will clean themselves up. The current code copies the
result to another _bstr_t but this leaves the original BSTR unfreed.

e.g.

_bstr_t bstrSource;
if(SUCCEEDED(pError->GetSource(&bstrSource)) && bstrSource)
etc.

Comment 60

[David Bradley]

The compiler doesn't seem to like _bstr_t pointers for parameters taking
BSTR*'s. I get a compiler error when compiling your code, which is why I
originally didn't do that. Seemed a little bad to cast it. In any case, the
_bstr_t construct I'm using doesn't do a copy, it takes ownership of the BSTR
passed in and cleans it up, unless I misunderstood the meaning of the boolean
parameter.

c:/mozilla/main/mozilla/js/src/xpconnect/src/xpcthrower.cpp(302) : error C2664:
'GetSource' : cannot convert parameter 1 from 'class _bstr_t *' to 'unsigned sho
rt ** '

Comment 61

[Adam Lock]

Comment on attachment 108529 [details] [diff] [review]
Patch to address Adam's comments, and some additional formatting cleanup from jst

Apologies my mistake, the copy flag on the _bstr_t constructor should ensure
they're released. The flag is of type 'bool' however, so you should pass in
'false' instead of 'FALSE'.

Comment 62

[David Bradley]

Oh, good point. Dang, imagine that, MS code using actual standard types. I'll
get those changed before I check in.

Comment 63

[Asa Dotzler [:asa]]

Comment on attachment 108529 [details] [diff] [review]
Patch to address Adam's comments, and some additional formatting cleanup from jst

a=asa for chekin to 1.3a

Comment 64

[David Bradley]

patch checked in

Comment 65

[Phil Schwartau]

Rubber-stamp vrfy -

Comment 66

[David Bradley]

Comment on attachment 108030 [details] [diff] [review]
Previous patch with jst's changes

Cleaning up review requests