262218 - libxpcom.so should only export frozen symbols

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Darin Fisher]

libxpcom.so should only export frozen symbols.

I believe that we should rename libxpcom.so to libxpcom-private.so (or whatever
name we like), and create a new libxpcom.so that only exports frozen symbols. 
libxpcom.so should have a direct link time dependency on libxpcom-private.so. 
libxpcom.so would simply forward all calls to libxpcom-private.so, making it
nothing more than a shim library.

This is imperative because we too often find folks improperly linking to
libxpcom.so for non-frozen symbols because they forget to put -lxpcomglue before
-lxpcom on the link line.  We should be providing better documentation of course
to combat this problem as well, but we can also improve the technology to
encourage proper usage.

Going forward, this would give us the option to replace libxpcom-private.so with
libgecko.so (or libxul.so -- whatever you want to call it) that includes not
just the core xpcom code but all of Gecko.  DSO consolidation is important to
improving the performance of Gecko when not statically linked, and if
applications link to libxpcom.so, then those applications would be able to
pre-bind to Gecko while still having an indirect link time dependency on it.

I believe that this may be "the ticket" to building a performant version of
Firefox that loads an embeddable version of Gecko.

Comment 1

[Darin Fisher]

XPTC_InvokeByIndex and friends are interesting non-frozen exports.  We really
should think about either freezing the typelib system.  Shaver?

Comment 2

[Darin Fisher]

... either ..., or live with having language bindings be tightly coupled to the
Gecko version with access to private interfaces, privy to libxpcom-private.so, etc.

Comment 3

[Mike Shaver (:shaver emeritus)]

Freezing the typelib system is no small job.  I would consider it, along with an
overhaul of our current typelib types, for mozilla2.  For now, I think we
language-binding authors really just end up taking our lumps.

(I'd also like to make some extensions to that API before freezing it, to reduce
the thunk-count for some common operations.)

Comment 4

[Darin Fisher]

Attachment: v0 patch

this patch is just a first pass.  i'd still need to patch all of the fun
packaging scripts.  but, this works.  everything that is in libxpcom.so today
is moved to libxpcom_core.so, and libxpcom.so contains nothing but frozen
symbols.

in order for this stub library to work properly, all of the actual definitions
of the symbols need to be renamed internally.  so, with some preprocessor fu i
managed to make all of the internal versions of the functions end with "_P".  i
keyed off of MOZILLA_STRICT_API to control whether or not the function names
get mapped to the _P versions.

if anyone has a better suggestion for how to do this, please let me know.

Comment 5

[Benjamin Smedberg]

Comment on attachment 160614 [details] [diff] [review]
v0 patch

>Index: xpcom/build/nsXPCOM.h

>+// Map frozen functions to private symbol names if not using strict API.
>+#ifndef MOZILLA_STRICT_API
>+# define NS_InitXPCOM2               NS_InitXPCOM2_P

I think we should be using an "opt-in" define, instead of the opt-out
MOZILLA_STRICT_API

/me suggests XPCOM_UNFROZEN_API

Secondly, you changed XPCOM_LIBS to use the "_core" lib, but we will (hopefully
soon) start having some components/programs that only use the frozen API and
can link directly against libxpcom. Maybe we should add another
configure/Makefile var for that case now?

Comment 6

[Darin Fisher]

> I think we should be using an "opt-in" define, instead of the opt-out
> MOZILLA_STRICT_API
>
> /me suggests XPCOM_UNFROZEN_API

That would be nice, but we wouldn't want to define XPCOM_UNFROZEN_API in
mozilla-config.h because that is something users of the Gecko SDK have to
include in order to use the Gecko SDK.  I guess it would not be hard to arrange
for that with some rules.mk hacking.

However, it might be even better if we modified the headers included in the
Gecko SDK in such a way to force the definition of MOZILLA_STRICT_API (and
probably XPCOM_GLUE as well).

Basically, to correctly use the Gecko SDK today, one has to define
MOZILLA_STRICT_API and XPCOM_GLUE, so I'm not really adding a new requirement
with this patch.  I'm causing users of the Gecko SDK who do not define
MOZILLA_STRICT_API and XPCOM_GLUE to see a link time error about missing
symbols.  That's better than the status quo IMO.


> Secondly, you changed XPCOM_LIBS to use the "_core" lib, but we will (hopefully
> soon) start having some components/programs that only use the frozen API and
> can link directly against libxpcom. Maybe we should add another
> configure/Makefile var for that case now?

Those components/programs should be built on top of dist/sdk only.  We should
probably invent a makefile variable that enables components/programs in the tree
to be easily built against just what is provided under dist/sdk.

In short, changing XPCOM_LIBS seems appropriate to me assuming we have another
set of makefile variables that get enabled when building against dist/sdk.

Comment 7

[Mike Shaver (:shaver emeritus)]

Wouldn't we just add -DXPCOM_UNFROZEN_API to our CFLAGS for the Mozilla build,
and be done?

Comment 8

[Darin Fisher]

> Wouldn't we just add -DXPCOM_UNFROZEN_API to our CFLAGS for the Mozilla build,
> and be done?

Sure, but what do we do about MOZILLA_STRICT_API and XPCOM_GLUE, which should be
specified by those using the Gecko SDK?  Those using the Gecko SDK need to
define those anyways, so why not piggy back on those defines?

Perhaps we should simply deprecate MOZILLA_STRICT_API and XPCOM_GLUE, and switch
everything over to XPCOM_UNFROZEN_API.  I'll go with that unless someone has a
better suggestion.

Comment 9

[Benjamin Smedberg]

It is not always necessary to use XPCOM_GLUE when using the SDK: components can
safely link directly against xpcom.dll instead of using the glue.

Comment 10

[Darin Fisher]

(In reply to comment #9)
> It is not always necessary to use XPCOM_GLUE when using the SDK: components can
> safely link directly against xpcom.dll instead of using the glue.

you're right!  in fact, XPCOM_GLUE only affects nscore.h, which defines NS_COM.
 on platforms such as linux, defining XPCOM_GLUE has no real affect.  on windows
however defining XPCOM_GLUE means that one must link not to xpcomglue.a but to
xpcomglue_s.a.  i mistook XPCOM_GLUE for meaning that one is linking to either
version of the glue library, but that is not the case.

ok, forget i mentioned XPCOM_GLUE :)

let XPCOM_UNFROZEN_API mean !MOZILLA_STRICT_API, and we are done.

Comment 11

[Darin Fisher]

Attachment: v1 patch

here's a complete version of the patch sans XPCOM_UNFROZEN_API.  (i plan on
implementing that define, and deprecating MOZILLA_STRICT_API, in a follow-up
patch.)  i still contend that you must define MOZILLA_STRICT_API today in order
to use the Gecko SDK, so additional dependency on that define is no worse off.

i need to figure out a good way to undefine the globally defined
XPCOM_UNFROZEN_API for certain modules (xpfe/bootstrap, etc.) within our source
tree before i go down that path, so i'd prefer to keep it to a follow-up patch.


this patch also includes changes to make mozilla-bin use the new string API
from the Gecko SDK instead of linking a copy of libstring.a -- it's about time
;)

Comment 12

[Darin Fisher]

it looks like some powerplant project files will also need to be updated for
this change, but i don't know exactly how to do that.

Comment 13

[Mike Pinkerton (not reading bugmail)]

those projects are bit-rotting already. nobody is actively maintaining the
carbon powerplant embedding layer. i guess conrad "owns" that, i'll cc him.

Comment 14

[Benjamin Smedberg]

Comment on attachment 161109 [details] [diff] [review]
v1 patch

>Index: xpcom/build/nsXPCOMPrivate.h

>+// Map frozen functions to private symbol names if not using strict API.
>+#ifndef MOZILLA_STRICT_API
>+# define NS_RegisterXPCOMExitRoutine        NS_RegisterXPCOMExitRoutine_P
>+# define NS_UnregisterXPCOMExitRoutine      NS_UnregisterXPCOMExitRoutine_P
>+# define NS_GetFrozenFunctions              NS_GetFrozenFunctions_P
>+#endif

The exitroutine stuff is not frozen, right? I don't think it should be, unless
there's a usage I missed.

>Index: embedding/qa/testembed/Tests.cpp

Is this an accident? seems unrelated

>Index: xpinstall/packager/xpcom-win.pkg

>+bin\xpcomcor.dll

I thought we were using xpcom_core.dll on windows...

>Index: xpfe/bootstrap/nsStringSupport.h

>+#define EmptyCString() nsCString()
>+#define EmptyString() nsString()

Does this actually work? I didn't think you were allowed to declare macros with
an empty paramlist. I would prefer either
#define EmptyCString nsCString
or
typedef nsCString EmptyCString

r=me if you remove the exitrountine, or else a really good explanation why
you're treating it like a frozen function.

Comment 15

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

I like nsStringSupport.h, a neat trick ;)

+      void AssignWithConversion(const char *data)
+        {
+          NS_CStringToUTF16(nsEmbedCString(data), NS_CSTRING_ENCODING_ASCII,
*this);
+        }
+      void AssignLiteral(const char *data)
+        {
+          AssignWithConversion(data);
+        }

maybe it would be better to implement the non-deprecated function
(AssignLiteral) directly, and the deprecated one (AssignWithConversion) using
AssignLiteral... (or both using a new AssignASCII)

Comment 16

[Darin Fisher]

> The exitroutine stuff is not frozen, right? I don't think it should be, unless
> there's a usage I missed.

they would appear to be private, frozen xpcom apis (much like
NS_GetFrozenFunctions).  the glue library uses those functions, so if you link
against xpcomglue.a, you will have a libxpcom.so dependency on the exit routine
stuff in your code.  perhaps those functions should be documented as frozen! ;)

notice: those functions are included in the return value from NS_GetFrozenFunctions.


> >Index: embedding/qa/testembed/Tests.cpp
> 
> Is this an accident? seems unrelated

yeah, that's unrelated.  sorry, ignore that.



> >Index: xpinstall/packager/xpcom-win.pkg
> 
> >+bin\xpcomcor.dll

oops.. thanks for catching that.

 
> >+#define EmptyCString() nsCString()
> >+#define EmptyString() nsString()
> 
> Does this actually work? I didn't think you were allowed to declare macros 
> with an empty paramlist.

It works for me with GCC, and I think MSVC wouldn't mind either.  But, I can
change it if you think it breaks some preprocessor.

Comment 17

[Darin Fisher]

> maybe it would be better to implement the non-deprecated function
> (AssignLiteral) directly, and the deprecated one (AssignWithConversion) using
> AssignLiteral... (or both using a new AssignASCII)

sure, i could flip those around.  np.

Comment 18

[Benjamin Smedberg]

(In reply to comment #16)
> > The exitroutine stuff is not frozen, right? I don't think it should be, unless
> > there's a usage I missed.
> 
> they would appear to be private, frozen xpcom apis (much like
> NS_GetFrozenFunctions).

Has any version of the glue linked against them directly, or only through
NS_GetFrozenFunctions? It turns out they are marked frozen-but-internal,
http://lxr.mozilla.org/mozilla/source/xpcom/build/nsXPCOMPrivate.h#59

> It works for me with GCC, and I think MSVC wouldn't mind either.  But, I can
> change it if you think it breaks some preprocessor.

I don't care, I guess, as long as you watch ports for sillyness.

Comment 19

[David Bradley]

I don't think C++ says much about how the preprocessor should handle this. 
Leaves it up to the implementation's preprocessor. MSVC 6/7 handles it fine, 
appears to treat it as if it didn't have parens.

It makes me a little nervous to use EmptyString as a macro. I think that's 
potentially a common name someone could get bitten if they chose to use that 
elsewhere. Anyone who has used Win32 can appreciate the dangers.

Comment 20

[Brendan Eich [:brendan]]

There's nothing wrong with empty formal parameter lists in C pre-processor
macros and I know of no bugs in any C or C++ implementations.  However, dbradley
has a point.  The way to avoid trouble is this:

#define EmptyCString nsCString

Then even if we invade some other EmptyCString's namespace, we'll only change
the symbol that the compiler and linker (and debugger) sees, and nothing should
break at build-time.

/be

Comment 21

[Brendan Eich [:brendan]]

> Then even if we invade some other EmptyCString's namespace, we'll only change
> the symbol that the compiler and linker (and debugger) sees, and nothing
> should break at build-time.

I'm assuming the other namespace is static, or somehow sealed in its own library
so that the two nsCString symbols that result (ours and the other namespace's
once the preprocessor has replaced EmptyCString with nsCString) don't collide.

Name collision on nsCString is less likely than on EmptyCString due to the ns,
so it's not a big worry in the cases where static and library sealing (weak
symbols or whatever) don't apply.

/be

Comment 22

[Darin Fisher]

> Has any version of the glue linked against them directly, or only through
> NS_GetFrozenFunctions? It turns out they are marked frozen-but-internal,
> http://lxr.mozilla.org/mozilla/source/xpcom/build/nsXPCOMPrivate.h#59

no, directly.  grep for ExitRoutine in xpcom/glue and see for yourself ;-)

in fact, if i build a component and link with -lxpcomglue -lxpcom, then i will
have dependencies on the ExitRoutine stuff from my component.  i.e., they are
frozen ;-)(In reply to comment #19)


> It makes me a little nervous to use EmptyString as a macro. I think that's 
> potentially a common name someone could get bitten if they chose to use that 
> elsewhere.

please note that this header file is not for general purpose use.  it is for
xpfe/bootstrap only.  there might be value in providing a generic, full featured
version of nsStringSupport.h for use by embedders and component authors so that
they can use the "familiar" internal mozilla string API while depending on only
the frozen mozilla string ABI, but that's outside the scope of this bug ;-)

Comment 23

[Darin Fisher]

it turns out that we need to make XPCOM_LIBS include both -lxpcom and
-lxpcom_core to ensure that libxpcom is loaded into memory for use by plugins
and extensions.

new patch coming up.

Comment 24

[Darin Fisher]

Attachment: v1.1 patch

revised per previous comment

Comment 25

[Darin Fisher]

Attachment: v2 patch

ok, this patch includes changes to make mfcembed use only frozen strings.

i've also moved nsStringAPI.cpp into the stub library (xpcom.dll) instead of
having thunks for each of the string API functions.  there did not seem to be
much need to compile nsStringAPI.cpp into xpcom_core.

Comment 26

[Darin Fisher]

Ugh.. this patch requires a slight modification to build under OSX.  It is
necessary when linking libxpcom.dylib to include on the link line all of the
NSPR libraries even though no NSPR methods are used by libxpcom.dylib itself. 
Instead, it appears that the linker needs to process all of the dependencies
when linking.
Ok, no problem, but...

This means that the Gecko SDK needs to include libxpcom_core.dylib under OSX. 
Or else it means that we need to ship a dummy libxpcom.dylib that has no
dependencies.  I'm presuming that at runtime OSX is able to deal with the
dependency of a dependent DYLIB changing.  Hmm...

Comment 27

[Benjamin Smedberg]

Comment on attachment 162233 [details] [diff] [review]
v2 patch

As you wish, change _IMPL_NS_STRINGAPI as well as _IMPL_NS_COM and
_IMPL_NS_COM_OBSOLETE to avoid the reserved-for-compilers underscore prefix.

Comment 28

[Darin Fisher]

> As you wish, change _IMPL_NS_STRINGAPI as well as _IMPL_NS_COM and
> _IMPL_NS_COM_OBSOLETE to avoid the reserved-for-compilers underscore prefix.

underscore issue is quite pervasive in xpcom... punting to bug 266011 :-)

Comment 29

[Darin Fisher]

fixed-on-trunk

Comment 30

[Darin Fisher]

I had to disable mfcembed in static builds as a result of this patch.  We have
to decide if it is worth it to support static builds of mfcembed -- afterall,
mfcembed is supposed to be an example of how to properly embed gecko! ;-)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Either this patch or the patch for bug 263957 or the patch for bug 263903 caused
a Tp regression on btek...

Comment 32

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

(In reply to comment #11)
> this patch also includes changes to make mozilla-bin use the new string API
> from the Gecko SDK instead of linking a copy of libstring.a -- it's about time
> ;)

Does this mean nsStringStats can get rid of the test for zero alloc count in the
dtor?
(http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsSubstring.cpp#72)

Comment 33

[Boris Zbarsky [:bzbarsky]]

Ok, all the performance tests on luna regressed as well (Tp, Tdhtml, Ts, Txul).
 Ts regressed on comet, and Txul went up on beast.

Chances are, this patch is the reason, since the other two patches really should
not have affected Tdhtml.

Comment 34

[Darin Fisher]

Yeah, it really can only be this patch that affected our perf numbers.  I'm not
sure how since libxpcom.so is not on any critical paths, or so I would have thought.

One possibility is that gklayout and friends are somehow using symbols from
libxpcom.so instead of directly using libxpcom_core.so.  It is true that I'm
linking all components against libxpcom.so in addition to libxpcom_core.so, so
it is possible that something odd is happening.

It's hard to imagine that the addition of a virtually unused libxpcom.so would
cause this kind of regression.  Hmm...

Comment 35

[Darin Fisher]

Hmm... previously all of the string symbols were being loaded from mozilla-bin,
whereas now they are all being loaded from libxpcom_core.so.  I wonder if that
could somehow be affecting performance.  Hmm...

Comment 36

[Darin Fisher]

i filed bug 266071 to track the perf regression issue.

Comment 37

[Hiro]

tinderbox is in flames by build of Camino.
I think that it is necessary to add libxpcom_core to a Xcode project file.
http://lxr.mozilla.org/mozilla/source/camino/Camino.xcode/

Comment 38

[[e-mail]]

Something in the last 10 days broke my mingw, and I'm blaming this.
c:/mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp: In static member function
   `static nsresult nsNativeAppSupportWin::GetCmdLineArgs(BYTE*,
   nsICmdLineService**)':
c:/mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:1916: `isspace' undeclared
   (first use this function)
c:/mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp:1916: (Each undeclared
   identifier is reported only once for each function it appears in.)

Comment 39

[Mike Kaply [:mkaply]]

neil: add a #include for <ctype.h> in nsNativeAppSupportWin.cpp

Comment 40

[David G King]

This bug may have also caused the 2nd problem in Bug #266087

Comment 41

[[e-mail]]

(In reply to comment #39)
>neil: add a #include for <ctype.h> in nsNativeAppSupportWin.cpp
Checked in.

Comment 42

[David G King]

(In reply to comment #39)
> neil: add a #include for <ctype.h> in nsNativeAppSupportWin.cpp

That didn't fix it for me, however Darin over in
http://bugzilla.mozilla.org/show_bug.cgi?id=266087#c5
has checked in a fix for nsWindowCreator.cpp

Comment 43

[Bill Mason]

Bug 266214 comment 4 suggests that the fix here broke the FF trunk installer in
bug 266149, and caused the smoketest blocker Seamonkey bug 266214 (browser will
not launch after install).

Comment 44

[knut st. osmundsen]

Darin or Mike: xpcom/build/nsOS2VACLegacy.cpp is linked into XPCOMCOR.DLL
instead of XPCOM.DLL. This causes a SYS2070 in npoji6.dll (IBM Java 1.3 plugin)
on termination. Please move the source to XPCOM.DLL.

Comment 45

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

the nsStringSupport changes here caused bug 266563 / bug 267313