266006 - xpcom/glue should not declare methods with NS_COM

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Darin Fisher]

xpcom/glue should not declare methods with NS_COM.

we should use a different macro, like NS_GLUE, so that when component authors
include our headers, frozen symbols have __declspec(dllimport) applied to them
but unfrozen symbols (like those contained in the glue library) do not.

right now, component authors under windows have to link to the full static glue
library if they want to use any glue methods, but that means additional overhead
that isn't really necessary.  component authors should be able to link to the
normal glue library and then link to xpcom.dll for any frozen symbols that they use.

this works properly under linux since NS_COM is an empty macro there, and it
should be supported under win32 as well.

Comment 1

[Darin Fisher]

Attachment: v1 patch

OK, this patches make the xpcomglue_s.lib that is output into the gecko sdk
usable from windows.  What I've done is to build xpcom/glue three times now
instead of twice (hey, don't laugh!)...

First, we build it to create xpcomglue_s.lib, which should import any frozen
symbols from xpcom.dll.

Second, we build it to create xpcomglue.lib, which should not import any
symbols from xpcom.dll since it defines all of them as stubs.

Finally, we build it into xpcom_core.dll such that the glue functions are seen
as exported functions by the rest of Mozilla.

Doing this yields a xpcomglue_s.lib that can be linked against like this:

  link ... xpcomglue_s.lib xpcom.lib

This allows components to benefit from the glue library without the bloat of
the full glue library.	They pull in frozen symbols from xpcom.dll.

This is already supported under Linux, and this patch makes it work correctly
under Win32 as well.  NOTE: I've made it so that xpcom_core.dll no longer links
to xpcomglue_s.lib.  That library is now generated for the sole purposes of
being output into the Gecko SDK.

Comment 2

[Darin Fisher]

This patch causes problems for those who link against embed_base_s.lib and
xpcom_core.lib ... which is what the ActiveX control currently does. 
embed_base_s.lib is compiled with MOZILLA_STRICT_API defined, which means that
it should not be trying to import symbols from xpcom_core.lib.  However, for
some reason we end up with a few duplicate symbols in embed_base_s.lib and
xpcom_core.lib.  Investigating...

Comment 3

[Darin Fisher]

The problem I just described appears to be unique to debug builds.

The build errors are:

xpcom_core.lib(xpcom_core.dll) : error LNK2005: "public: __thiscall
nsQueryInterface::nsQueryInterface(class nsISupports
 *)" (??0nsQueryInterface@@QAE@PAVnsISupports@@@Z) already defined in
embed_base_s.lib(nsEmbedAPI.obj)
xpcom_core.lib(xpcom_core.dll) : error LNK2005: "public: __thiscall
nsQueryInterfaceWithError::nsQueryInterfaceWithError
(class nsISupports *,unsigned int *)"
(??0nsQueryInterfaceWithError@@QAE@PAVnsISupports@@PAI@Z) already defined in embed
_base_s.lib(nsEmbedAPI.obj)
xpcom_core.lib(xpcom_core.dll) : warning LNK4006: "public: __thiscall
nsQueryInterface::nsQueryInterface(class nsISuppor
ts *)" (??0nsQueryInterface@@QAE@PAVnsISupports@@@Z) already defined in
embed_base_s.lib(nsEmbedAPI.obj); second definit
ion ignored
xpcom_core.lib(xpcom_core.dll) : warning LNK4006: "public: __thiscall
nsQueryInterfaceWithError::nsQueryInterfaceWithErr
or(class nsISupports *,unsigned int *)"
(??0nsQueryInterfaceWithError@@QAE@PAVnsISupports@@PAI@Z) already defined in emb
ed_base_s.lib(nsEmbedAPI.obj); second definition ignored

Looks to me like MSVC is including code for some of these things in
embed_base_s.lib because we are compiling it with NS_COM_GLUE defined to nothing.

Perhaps it is time to make the ActiveX control not link to xpcom_core.dll :-)

Comment 4

[Darin Fisher]

Another solution would be to build a non-MOZILLA_STRICT_API version of
embed_base_s.lib ...

Comment 5

[Darin Fisher]

Attachment: v1.1 patch

OK, the other choice was to simply not use any of the glue code in
embed_base_s, which turned out to be fairly easy since it's so little code.

This seems better than providing multiple different versions of
embed_base_s.lib

Comment 6

[Benjamin Smedberg]

Comment on attachment 163642 [details] [diff] [review]
v1.1 patch

Damn, I thought I reviewed this yesterday, now I have to recreate my review
comments.

>Index: xpcom/build/Makefile.in

>+GARBAGE         += $(XPCOM_GLUE_SRC_LCSRCS) $(wildcard *.$(OBJ_SUFFIX))
>+
>+ifeq ($(OS_ARCH),WINNT)
>+GARBAGE		+= $(addprefix $(srcdir)/,$(XPCOM_GLUE_SRC_LCSRCS))
>+endif

You included these lines twice, and you can remove the WINNT case altogether,
it's build-system archealogy.

>Index: embedding/base/nsEmbedAPI.cpp

>+            // If the application is using an GRE, then, auto register components
>+            // in the GRE directory as well.

In fact we ought to be registering the GRE first, and then the appdir, but
XPCOM does this wrong also:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/build/nsXPComInit.cpp
&rev=1.202&cvsroot=/cvsroot&mark=602,633#602

I wrote a patch which reverses that logic in nsXPCOMInit.cpp and simplifies
that code significantly, I need to go find it. Anyway, that's just an
observation, you don't need to do anything about it.

Comment 7

[Darin Fisher]

fixed-on-trunk

Comment 8

[Darin Fisher]

I think it might be good to land this on the 1.7 branch, so that we can make the
SDK useful for windows extension authors who do not wish to link to the full
XPCOM glue library.