Closed Bug 298047 Opened 19 years ago Closed 19 years ago

Drop the dependencies of XPCOM glue on NSPR

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8beta3

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
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.
is this the only blocker? as I remember (2+ years ago) there were more.
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.
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.
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.
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.
Correct. I don't think that components ought to be using the standalone glue.
I guess that means that we don't need to worry about nsGenericFactory then, right?
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.
Attachment #187530 - Flags: review?(darin)
> 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 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?
class nsACString : public nsCStringContainer
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.
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.
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.)
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
Attachment #187670 - Flags: review?(darin)
Attachment #187530 - Attachment is obsolete: true
Attachment #187530 - Flags: review?(darin)
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.
Attachment #187670 - Flags: review?(darin) → review+
> 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.
> 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.
"... and it has a [comment] explaining"
> 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.
Attachment #187670 - Flags: approval1.8b3?
Attachment #187670 - Flags: approval1.8b3? → approval1.8b3+
Attached patch cvs moves fileSplinter Review
Comment on attachment 187686 [details] [diff] [review] cvs moves file r=darin
Attachment #187686 - Flags: review+
Comment on attachment 187686 [details] [diff] [review] cvs moves file move completed.
Attachment #187670 - Attachment description: Remove the XPCOM glue dependency on NSPR, rev. 2 → Remove the XPCOM glue dependency on NSPR, rev. 2 [checked in]
This may have caused Thunderbird/boxset to burn.
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.
Depends on: 299214
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.
Attachment #187670 - Attachment description: Remove the XPCOM glue dependency on NSPR, rev. 2 [checked in] → Remove the XPCOM glue dependency on NSPR, rev. 2 [backed out]
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.
Attachment #187828 - Flags: approval1.8b3?
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
Those warnings occur while linking libxpcomsample.so (dependent glue).
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 on attachment 187828 [details] [diff] [review] Remove the XPCOM glue dependency on NSPR, rev. 2.1 interdiff looks good, r=darin
Attachment #187828 - Flags: review+
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.
Attachment #187828 - Flags: approval1.8b3? → approval1.8b3+
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 ;-)
Status: NEW → RESOLVED
Closed: 19 years ago
Depends on: 298044
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta3
Depends on: 327092
*** Bug 191049 has been marked as a duplicate of this bug. ***
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: