298047 - Drop the dependencies of XPCOM glue on NSPR

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Benjamin Smedberg]

In order to prevent the need for every XPCOM glue client to ship NSPR with
itself, I need to drop the standalone XPCOM glue dependency on NSPR. I think I
did this once before but never finished the last few steps (THREADSAFE_ISUPPORTS
can't be supported in the standalone glue without dynamically loading NSPR (I
think for the moment I'll just skip those, unless I find a glue internal that
needs them), and the debug threading checks (nsAutoOwningThread) cannot be used.

Comment 1

[Benjamin Smedberg]

OK, there's a problem in that the nsGenericFactory implementation needs the
THREADSAFE_ISUPPORTS, which uses PR_Atomic(Increment|Decrement). I'm thinking
about "glue"ing those functions along with the XPCOM functions.

Comment 2

[Doug Turner (:dougt)]

is this the only blocker?  as I remember (2+ years ago) there were more.

Comment 3

[Benjamin Smedberg]

It's the only one I don't see an easy fix for: I'm using platform-specific
linking code for dynamic loading, and the PR_GetEnv calls are just replaced with
getenv. I removed (#ifdef XPCOM_GLUE) the threadsafety checks of nsAutoOwningThread.

Comment 4

[Darin Fisher]

Perhaps NSPR can expose its implementation of PR_AtomicIncrement and
PR_AtomicDecrement as inlined functions that can be used by the Glue library. 
The exported symbols can still exist in NSPR as wrappers around the inlined
implementations.

Comment 5

[Wan-Teh Chang]

PR_AtomicIncrement and PR_AtomicDecrement can't always
be implemented in a few lines.  On some platforms they
are implemented with locks, which require most of NSPR.
So we can't inline PR_AtomicIncrement and PR_AtomicDecrement
on all platforms.

If XPCOM glue is a simple library that doesn't need to
have the best multithreaded performance, you can replace
PR_AtomicIncrement and PR_AtomicDecrement with your
own implementation that uses a lock.  This assumes XPCOM
glue has itw own implementation of locks.

Comment 6

[Darin Fisher]

Perhaps we should step back and reconsider this approach.  For extension
authors, the NSPR dependency is fine.  In fact, it is perfectly reasonable.  I'd
argue that this is also the case for any components created by an embedder.  It
is not the case for code that needs to run prior to XPCOM being initialized, and
that case only applies to a very limited subset of consumers: namely embedders.

Maybe this bug should simply be about solving the problem faced by embedders of
not having a NSPR until they locate a GRE to use.

There's no reason to not leverage NSPR when developing XPCOM component libraries.

Comment 7

[Benjamin Smedberg]

Correct. I don't think that components ought to be using the standalone glue.

Comment 8

[Darin Fisher]

I guess that means that we don't need to worry about nsGenericFactory then, right?

Comment 9

[Benjamin Smedberg]

Attachment: Remove the XPCOM glue dependency on NSPR, rev. 1

This patch removes almost all the NSPR dependencies except for the PRLinking
dependencies in nsXPCOMGlue.cpp (which are already removed on mac, so Mac will
be done when this patch lands).

The changes to nsStringAPI are due to linking errors of nsAString symbols in
the *not* standalone case (dependent glue). This moves all the symbols that you
would expect on nsAString to nsStringContainer to avoid naming conflicts. I
believe that this change is backwards compatible with both code and linking.

Comment 10

[Darin Fisher]

> The changes to nsStringAPI are due to linking errors of nsAString symbols in
> the *not* standalone case (dependent glue). This moves all the symbols that you
> would expect on nsAString to nsStringContainer to avoid naming conflicts. I
> believe that this change is backwards compatible with both code and linking.

I'm not sure I understand the need for this change.  I will have to think about
it more, but off-hand there should be no problem here since the glue library
should only be used by code that does not define MOZILLA_INTERNAL_API.

At the very least, it is sort of odd to change the NS_String* methods to take a
nsStringContainer type since those methods are designed to work with an
arbitrary nsAString.

Can you explain further why this change is necessary?

Comment 11

[Darin Fisher]

Comment on attachment 187530 [details] [diff] [review]
Remove the XPCOM glue dependency on NSPR, rev. 1

-NS_CStringCloneData(const nsACString &aStr)
+NS_CStringCloneData(const nsCStringContainer &aStr)

no, in fact this is very wrong.  this will break plenty of code.  consider code
like this:

NS_IMETHODIMP nsFoo::SetBar(const nsAString &bar) {
  PRUnichar *s = NS_StringCloneData(bar);
  ...
}

Now, this code won't compile.  or have you typedef'd nsAString to
nsStringContainer somewhere that I missed?

Comment 12

[Benjamin Smedberg]

class nsACString : public nsCStringContainer

Comment 13

[Benjamin Smedberg]

The string changes were made necessary by the mac builds I was trying to use
linking the nsTestSample.dylib component using the dependent glue (not the
standalone glue), because "the symbol nsACString::~nsACString [referenced in
nsTestSample.o] defined in indirectly referenced library libxul.dylib" (now
XUL). This is the same problem we've had with mixing the frozen and non-frozen
string APIs for a while, and I believe this fixes all those issues.

Comment 14

[Darin Fisher]

I think it would be better to fix bug 243109 instead, which calls for renaming
the symbols (either the internal ones or the external ones -- I'd change the
internal names to avoid conflicts with externally defined ones, which we may not
be able to change due to existing embeddings).

Making nsAString inherit from nsStringContainer is very awkward.  Moreover, the
API makes the promise that a nsStringContainer uses null-terminated storage, but
this is not true of an arbitrary nsAString.  See the documentation for
NS_StringSetData for example.

People may write code like this:

  {
    nsCStringContainer s;
    NS_CStringContainerInit(s);

    NS_CStringSetData(s, "hello world");

    const char *data;
    NS_CStringGetData(s, &data);
    printf("%s\n", data);

    NS_CStringContainerFinish(s);
  }

Now, this is not true of an arbitrary nsAString:

  NS_IMETHODIMP nsFoo::GetBar(nsACString &bar) {
    NS_CStringSetData(bar, "hello world");

    const char *data;
    NS_CStringGetData(s, &data);
    printf("%s\n", data);  // OOPS -- may not be null terminated !!!
  }

But, it is hard to explain to users that this latter case is still true if
nsACString "is a" nsCStringContainer in the C++ sense.

So, please reconsider this patch.  I think it is abusing the string API.

Comment 15

[Darin Fisher]

Another thing that would help the situation here would be to create a version of
libxpcom.dylib that has no dependency on libxul.dylib.  That special version
would be included in the SDK.  People would link to it, but use the version of
libxpcom.dylib included with a Firefox or XULRunner release.  (It's important
that the DSOs and import libs included in the SDK only export frozen symbols, so
including libxul.dylib as is would be a bad idea.)

Comment 16

[Benjamin Smedberg]

Attachment: Remove the XPCOM glue dependency on NSPR, rev. 2 [backed out]

This is so much simpler that it scares me. But my Firefox build seems to work
perfectly with the simple #define nsACString nsACString_internal in nscore.h

Comment 17

[Darin Fisher]

Comment on attachment 187670 [details] [diff] [review]
Remove the XPCOM glue dependency on NSPR, rev. 2 [backed out]

>Index: xpcom/base/nscore.h

> #ifdef MOZILLA_INTERNAL_API
> #define NS_COM_GLUE NS_COM
>+#define nsAString nsAString_internal
>+#define nsACString nsACString_internal
> #else
> #define NS_COM_GLUE
> #endif

That other bug is not entirely fixed by this change FWIW as we'll
need to do something similar for nsString and friends.


>Index: xpcom/glue/nsGREGlue.cpp

> #include <string.h>
> 
>-#include "prenv.h"
>-#include "prio.h"
>-#include "plstr.h"
>+# include <dirent.h>

Is dirent.h needed on all platforms?  Isn't it for XP_UNIX only?
At least, I think the extra whitespace between # and include is
not needed.


>+static PRBool isconffile(const char *filename)
>+{
>+  static const char kExt[] = ".conf";
>+
>+  // Slightly inefficient, but I don't care.
>+  int len = strlen(filename);
>+
>+  if (len < sizeof(kExt) - 1)
>+    return PR_FALSE;
>+
>+  return (strcmp(filename + len - sizeof(kExt) + 1, kExt) == 0);
>+}

How about this:

  {
    const char *dot = strrchr(filename, '.');
    return dot && (strcmp(dot, ".conf") == 0);
  }


>+    if (!isconffile(entry->d_name))

nit: maybe use a consistent naming convention for functions


>Index: xpcom/glue/nsGenericFactory.cpp

>-#ifndef XPCOM_GLUE
>-#ifdef DEBUG 
>-    char* cs = aClass.ToString();
>-    fprintf(stderr, "+++ nsGenericModule %s: unable to create factory for %s\n", mModuleName, cs);

This is a good fix, but it reminds me that we ought to have
NS_StringToID and NS_IDToString functions exported for people
to use.


>Index: xpcom/glue/standalone/nsXPCOMGlue.cpp

>-void
>-GRE_AddGREToEnvironment()

How do you avoid the need for this function?


>Index: xpcom/sample/Makefile.in

You might want to work with justdave to move the ,v file on the CVS
server to avoid picking up CVS blame for nsTestSample.cpp.


Finally, I think it might make better sense to combine xpcomglue.lib
with embed_base_s.lib, and maybe just have one embedglue.lib.  that
way we can eliminate the confusion of having two "glue" libraries.
embedders should anyways call NS_InitEmbedding instead of NS_InitXPCOM2.

Comment 18

[Benjamin Smedberg]

> That other bug is not entirely fixed by this change FWIW as we'll
> need to do something similar for nsString and friends.

nsString is already fixed by the #defines at 
http://lxr.mozilla.org/mozilla/source/xpcom/string/public/nsStringAPI.h#1083

> This is a good fix, but it reminds me that we ought to have
> NS_StringToID and NS_IDToString functions exported for people
> to use.

bug 288954

> How do you avoid the need for this function?

This function never did anything useful except on windows (because the linux/mac
soloaders don't read LD_LIBRARY_PATH after the process is instantiated), and
windows already has the necessary code:

http://lxr.mozilla.org/mozilla/source/xpcom/glue/standalone/nsXPCOMGlue.cpp#169

> Finally, I think it might make better sense to combine xpcomglue.lib
> with embed_base_s.lib, and maybe just have one embedglue.lib.  that
> way we can eliminate the confusion of having two "glue" libraries.
> embedders should anyways call NS_InitEmbedding instead of NS_InitXPCOM2.

I don't want to do that now. In 1.9 I intend to expose the embedding symbols
(NS_InitEmbedding) as exports from libxul, as well as integrating
gtkmozembed/activex control/cocoa NSView/QGeckoEmbed into libxul.

Comment 19

[Darin Fisher]

> nsString is already fixed by the #defines at 
> http://lxr.mozilla.org/mozilla/source/xpcom/string/public/nsStringAPI.h#1083

That's right.  I forgot about that ;-)


> > How do you avoid the need for this function?
> 
> This function never did anything useful except on windows (because the linux/mac
> soloaders don't read LD_LIBRARY_PATH after the process is instantiated), and
> windows already has the necessary code:
> 
> http://lxr.mozilla.org/mozilla/source/xpcom/glue/standalone/nsXPCOMGlue.cpp#169

Hmm.. the function calls SetCurrentDirectory and it has a function explaining
that that is necessary.  Is the comment wrong?


> 
> > Finally, I think it might make better sense to combine xpcomglue.lib
> > with embed_base_s.lib, and maybe just have one embedglue.lib.  that
> > way we can eliminate the confusion of having two "glue" libraries.
> > embedders should anyways call NS_InitEmbedding instead of NS_InitXPCOM2.
> 
> I don't want to do that now. In 1.9 I intend to expose the embedding symbols
> (NS_InitEmbedding) as exports from libxul, as well as integrating
> gtkmozembed/activex control/cocoa NSView/QGeckoEmbed into libxul.

I hope you are proposing that we have people link to a library (libxul) that
exports both frozen and non-frozen symbols.  That has only ever led to disaster
in the past.  Let's _not_ do that again.

Comment 20

[Darin Fisher]

"... and it has a [comment] explaining"

Comment 21

[Benjamin Smedberg]

> Hmm.. the function calls SetCurrentDirectory and it has a function explaining
> that that is necessary.  Is the comment wrong?

Sorta. Firstly, that code breaks lots of command-line handling stuff by setting
the current directory, and secondly the new DLL search path implemented in win2k
or xp (I forget which) means that local DLLs are calculated differently, so I
believe it is not an issue. As it stands that code certainly breaks win32
xulrunner apps that wish to handle file paths on the command line.

> I hope you are proposing that we have people link to a library (libxul) that
> exports both frozen and non-frozen symbols.  That has only ever led to disaster

I assume you mean "aren't", and I fully intend to stop exporting all the
non-frozen symbols from libxul by the end of 1.9.

Comment 22

[Benjamin Smedberg]

Attachment: cvs moves file

Comment 23

[Darin Fisher]

Comment on attachment 187686 [details] [diff] [review]
cvs moves file

r=darin

Comment 24

[Dave Miller [:justdave]]

Comment on attachment 187686 [details] [diff] [review]
cvs moves file

move completed.

Comment 25

[Mark Mentovai]

This may have caused Thunderbird/boxset to burn.

Comment 26

[Mark Mentovai]

Confirming.  The libtool in Xcode 1.1 can't handle -executable_path.  This was
expected to break 10.2, but it will break some 10.3 builds as well.  That's
fine.  Xcode 1.5 is apparently OK (and it's a free update).

It would be nice to figure out when -executable_path was added to libtool (1.2
or 1.5) so that the minimum build requirements could be updated.

Comment 27

[Benjamin Smedberg]

Comment on attachment 187670 [details] [diff] [review]
Remove the XPCOM glue dependency on NSPR, rev. 2 [backed out]

I backed this out because of tinderbox red with older versions of xcode (mac);
I thought this was a 10.2/10.3 thing, but it's not. Bug 299214 is tracking
officially upgrading the build requirements and getting tinderboxen upgraded.

Comment 28

[Benjamin Smedberg]

Attachment: Remove the XPCOM glue dependency on NSPR, rev. 2.1

OK, this is the same patch as above but it avoids using XPCOM_FROZEN_LDOPTS in
xpcom/samples/Makefile.in, which means that I can continue glue and xulrunner
development without breaking the standard tinderbox builds trees.

Comment 29

[Benjamin Smedberg]

For reference, we have hidden-visibility problems on mac too:
ld: warning multiple definitions of symbol vtable for nsGetInterface
../../dist/lib/libxpcomglue_s.a(nsIInterfaceRequestorUtils.o) definition of
vtable for nsGetInterface in section (__DATA,__const)
../../dist/bin/XUL(nsIInterfaceRequestorUtils.o) definition of vtable for
nsGetInterface
and several hundred more

Comment 30

[Benjamin Smedberg]

Those warnings occur while linking libxpcomsample.so (dependent glue).

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 187828 [details] [diff] [review]
Remove the XPCOM glue dependency on NSPR, rev. 2.1

Can we get at least review of the interdiff, and someone's assessment of what's
really resulting from those symbol-duplication warnings?

Comment 32

[Benjamin Smedberg]

Attachment: rev 2.1 -u21 like the original

Comment 33

[Darin Fisher]

Comment on attachment 187828 [details] [diff] [review]
Remove the XPCOM glue dependency on NSPR, rev. 2.1

interdiff looks good, r=darin

Comment 34

[Benjamin Smedberg]

The symbol-duplication warnings will cause problems in the future, and they
aren't anything new or really related to this patch at all: I just wanted to
make a note for future reference to examine the possibility of hidden-visibility
on mac.

Comment 35

[Benjamin Smedberg]

Revision 2.1 checked in for 1.8b3. I will mark this bug fixed, and the remaining
NSPR dependencies (all about linking) will be dealt with in bug 298044, which
will have a patch imminently ;-)

Comment 36

[Benjamin Smedberg]

*** Bug 191049 has been marked as a duplicate of this bug. ***