316098 - Need XPCOMGlue function for loading function pointers from libs

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[jhp (no longer active)]

From IRC:

bsmedberg: basically we need a function in the XPCOM glue that does "load a function pointer for a function named 'NS_DoSomething' from the XUL DLL"

This is necessary since XPCOMGlue doesn't use NSPR (or its PR_FindSymbol() function).

Comment 1

[Benjamin Smedberg]

Attachment: Add function for loading symbols from xul.dll

Comment 2

[Darin Fisher]

Comment on attachment 202818 [details] [diff] [review]
Add function for loading symbols from xul.dll

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

>         if (lastSlash) {
>             *lastSlash = '\0';
> 
>             XPCOMGlueLoadDependentLibs(xpcomDir, ReadDependentCB);
>+
>+            sprintf(lastSlash, "/" XUL_DLL);
>+
>+            sXULLibHandle = dlopen(xpcomDir, RTLD_GLOBAL | RTLD_LAZY);

So, it would seem that nulling out *lastSlash is unnecessary now.


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

>         if (lastSlash) {
>             *lastSlash = '\0';
> 
>             XPCOMGlueLoadDependentLibs(xpcomDir, ReadDependentCB);
>+
>+            sprintf(lastSlash, "\\" XUL_DLL);

ditto


>+nsresult
>+XPCOMGlueLoadXULFunctions(nsDynamicFunctionLoad *symbols)
>+{
>+    if (!sXULLibrary)
>+        return NS_ERROR_NOT_INITIALIZED;

It's interesting that the dlopen version does not null check
the DSO handle before trying to use it.  Are you doing that
explicitly because null is allowed with dlopen?  If so, shouldn't
you document that?


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


>+typedef void (*NSFuncPtr)();

Do we need to worry about stdcall vs cdecl here?


>+extern "C" NS_HIDDEN_(nsresult)
>+XPCOMGlueLoadXULFunctions(nsDynamicFunctionLoad *symbols);

I don't think we want XPCOM function names to use the term XUL
if we can help it.  What is this function really doing?  Is it
only loading symbols from libxul?  If so, why doesn't it also
support loading symbols from xpcom.dll?  Or, xpcom_core.dll?
If this is meant to expose "internal" APIs to glue consumers,
then perhaps it should be something like:

  XPCOMGlueLoadInternalFunctions

I'm not sure I understand the use case for this function at all.

Comment 3

[Benjamin Smedberg]

> So, it would seem that nulling out *lastSlash is unnecessary now.

It is still necessary because we pass "xpcomDir" to XPCOMGlueLoadDependentLibs before we write the XUL.dll particulars.
> the DSO handle before trying to use it.  Are you doing that
> explicitly because null is allowed with dlopen?  If so, shouldn't

Yes, I commented above but I shall comment again.

> >+typedef void (*NSFuncPtr)();
> 
> Do we need to worry about stdcall vs cdecl here?

Not really... all the callers are going to be casting these function pointers to particular types anyway.

> >+extern "C" NS_HIDDEN_(nsresult)
> >+XPCOMGlueLoadXULFunctions(nsDynamicFunctionLoad *symbols);
> 
> I don't think we want XPCOM function names to use the term XUL
> if we can help it.  What is this function really doing?  Is it
> only loading symbols from libxul?  If so, why doesn't it also

Yes, it is only loading (presumably frozen, because we're only exporting frozen functions) functions from libxul. We have no need to load functions from xpcom.dll because the glue does that for us directly.

The use case for this is to expose the frozen JavaXPCOM functions and the frozen gtkmozembed functions from libxul and then be able to import and use them dynamically through a function pointer. The typical code to bootstrap gtkmozembed would then be

static nsDynamicFunctionLoad funcs {
  { "gtk_moz_embed_get_type", &gtk_moz_embed_get_type } // need a cast here
  { "gtk_moz_embed_new",      &gtk_moz_embed_new }
  ... etc
};

int main() {
  GRE_FindGRE...();
  XPCOMGlueStartup();

  XPCOMGlueLoadXULFunctions(&funcs);

  // do stuff

  XPCOMGlueShutdown();
}

Comment 4

[Benjamin Smedberg]

Attachment: Add functions for loading symbols from xul.dll, rev. 2

And as I was reading my sample code I realized that the structure I proposed doesn't match it... so here we are.

Comment 5

[Darin Fisher]

Why is LibXUL providing gtkmozembed API functions?  That's really wacky.  Why isn't that a shim library built against frozen (or at least stable) APIs?  Why should FF or TB load the implementation of the gtkmozembed APIs?  The same thing goes for JavaXPCOM.

I'm really confused about the problem you are trying to solve.

Comment 6

[Benjamin Smedberg]

JavaXPCOM/gtkmozembed/activex control are all going to be builtin to libxul. There is not going to be a separate libgtkmozembed.so or activex control except perhaps for backwards compatibility that forwards to libxul.

The JavaXPCOM shim happens to have needed this first but it is not the primary target. The JavaXPCOM shim is a workaround for disabilities of the mac java system.load() call which cannot load libxul directly because even when it explicitly loads the dependent libs they don't get resolved properly.

Comment 7

[Darin Fisher]

> JavaXPCOM/gtkmozembed/activex control are all going to be builtin to libxul.

OK.  Where was this design discussed? ;-)


> The JavaXPCOM shim happens to have needed this first but it is not the 
> primary target. The JavaXPCOM shim is a workaround for disabilities of the 
> mac java system.load() call which cannot load libxul directly because even 
> when it explicitly loads the dependent libs they don't get resolved properly.

Why should we load code for the ActiveX shim and JavaXPCOM on Windows before they are needed?

I don't understand the OSX argument.  So, loading A.dylib, which depends on B.dylib, does not cause B.dylib to be loaded as well?

Comment 8

[Benjamin Smedberg]

> OK.  Where was this design discussed? ;-)

I discussed the JavaXPCOM design with Javier extensively via email, and I thought that you were aware of the "integrate gtkmozembed into libxul" bugs.

> Why should we load code for the ActiveX shim and JavaXPCOM on Windows before
> they are needed?

Because if we did not JavaXPCOM would have to duplicate the xptcall code and use the frozen string API etc, which is inefficient.

> I don't understand the OSX argument.  So, loading A.dylib, which depends on
> B.dylib, does not cause B.dylib to be loaded as well?

There are two related issues: on mac, dependencies are listed not merely with a filename but with an "installname", so libxul depends on @exectuable_path/libnspr.dylib. Since we're talking about embedders here, "exectuable_path" is going to be the path to the java runtime or whatnot, so it cannot automatically resolve the dependency (this is similar to the problem with LD_LIBRARY_PATH on linux). The XPCOM glue solves this problem by preloading the dependencies (using the dependentlibs.list file). However, this only works if you specify a special flag to the mac dynamic loader (see http://lxr.mozilla.org/mozilla/source/xpcom/glue/standalone/nsGlueLinkingOSX.cpp#71), which flag is not passed by the Java System.load() API. That is the only reason we need a java shim at all, instead of having Java just load libxul directly. The dependent bugs talk about this a bit more.

Comment 9

[Darin Fisher]

> I thought that you were aware of the "integrate gtkmozembed into libxul" 
> bugs.

OK, you're talking about bug 299988.  I thought the resolution there was that gtkmozembed just needed to call NS_InitXPCOM3 in the proper way.  What am I missing?  I don't see a justification for making FF on Linux load gtkmozembed.


> Because if we did not JavaXPCOM would have to duplicate the xptcall code and
> use the frozen string API etc, which is inefficient.

I don't understand why they would need to duplicate xptcall code.  Why don't you have some exports from XUL.dll for the xptcall API?  Freeze it when it is ready to be frozen.  Before that, just export it and call it non-frozen.  Require JavaXPCOM to #define something special or #include some special header file in order to opt-in to this unfrozen API.  Let JavaXPCOM be tightly bound 
to the particular release of LibXUL.  (Extension versioning!)  What is wrong with that solution?

Is the inefficiency of the frozen string API conjecture, or do we have figures that show how much it slows JavaXPCOM down?  If it slows it down by that much, why don't we open a performance bug against the frozen string API?


> There are two related issues: on mac, dependencies are listed not merely with 
> a filename but with an "installname", so libxul depends on
> @exectuable_path/libnspr.dylib. Since we're talking about embedders here,

Hrm... but if we installed LibXUL as a framework, then we could refer to its components using an absolute path, no?  Is that not a workable solution?

Sorry to be a pest, but it just seems unfortunate to need to fold code like this (that is going to be used by so few of our users) into the core Gecko DLL that will be used by all of our users.  How does this scale to other embeddings?

Comment 10

[jhp (no longer active)]

So what's the resolution here?  Which direction are we taking?

Comment 11

[Darin Fisher]

bsmedberg and I discussed this over e-mail, and we worked out many of the issues I had with this arrangement.  I'm still not very happy about coding "XUL" specific things into XPCOM.  We should try to maintain some semblence of modularity :-)

It seems to me that what you really want here is a XRE Glue library.  Since that is only used for embedders, it seems like you should just replace xpcomglue.lib with xreglue.lib.  (Keep xpcomglue_s.lib around for extension authors.)  If you go that route, then you could go to town in some toolkit/xre header file and freely use the XRE namespace and such.  How about it?

XRE_LoadFunctions! :-)

Comment 12

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

can one of you summarize that discussion somewhere public? :-) I also don't see why gtkmozembed should be merged into libXUL.

Comment 13

[Darin Fisher]

> can one of you summarize that discussion somewhere public? :-) I also don't see
> why gtkmozembed should be merged into libXUL.

I'll leave that to bsmedberg :-)

Comment 14

[Benjamin Smedberg]

> It seems to me that what you really want here is a XRE Glue library.  Since

How much different is it than the XPCOM glue? Unless we put this code in the nsGlueLinking sources I'm going to need to replicate large parts of this directory somewhere else. (Or we could just replace the XPCOM glue with toolkit/glue but I don't see what that buys us)... we're already inbreeding libxul and XPCOM in terms of framework names; I don't think that modularity to the point of a separate XUL glue is especially helpful and it makes thing more complicated.

Comment 15

[Darin Fisher]

Well, it has always been confusing to tell extension authors that they should link against one xpcom glue library and embedders another.  Moreover, embedders have had to also link against embed_base_s in the past.  It just seems to me that it would be cleaner to have a library named "embedglue" or "xreglue" for use by embedders, and "xpcomglue" for use by xpcom components authors.  Most of the stuff in the standalone xpcom glue is only needed by embedding, and I bet most of it could easily move out of XPCOM land.  It seems to me that embedding is something that doesn't belong at the XPCOM level.  But, hey... I guess having an embedding story that works is better than having a clean one that doesn't... so since you are doing the hacking, I'll leave it up to you.

Comment 16

[Benjamin Smedberg]

ok then, what I'd like to do is take this roughly as-is, so that I can put it on the 1.8 branch if needed. Then on the trunk I'll do a followup bug to split the componentglue (currently xpcomglue_s.lib) and the embedding glue (currently xpcomglue.lib) into better-named libs and perhaps build them from somewhere else.

Comment 17

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

it'd be good if 1.8 and trunk wouldn't differ in what people have to link to, imo...

Comment 18

[Darin Fisher]

(In reply to comment #17)
> it'd be good if 1.8 and trunk wouldn't differ in what people have to link to,
> imo...

But trunk is Firefox 3.0 (long ways off), and shouldn't we try to improve the way things work?  I too am fond of not changing things external developers need to think about, but XULRunner is new, LibXUL is new, so there is an opportunity to change the way things work for the better.  Let's take that opportunity! :-)

Comment 19

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

> But trunk is Firefox 3.0 (long ways off)

true, I keep forgetting that ;) Yeah, improving our embedding story is a good thing, especially if things are documented well ;)

Comment 20

[Benjamin Smedberg]

Mkaply, this is going to need OS/2 love at some point (the build won't break when I checkin, only when people start actually using this function, which could be a while yet on OS/2 since I don't really see any embedding on OS/2).

Comment 21

[Benjamin Smedberg]

Fixed on trunk.

Comment 22

[Mike Kaply [:mkaply]]

(In reply to comment #20)
> Mkaply, this is going to need OS/2 love at some point (the build won't break
> when I checkin, only when people start actually using this function, which
> could be a while yet on OS/2 since I don't really see any embedding on OS/2).
> 

Done

Comment 23

[jhp (no longer active)]

Attachment: const patch

Tried to use this code, but got a link error when building under MSVC6.  Fixed by adding "const" to function implementation.  Don't know if it's needed for the other files, but decided to add it anyway.  Also, the if check for the function pointer was testing the wrong thing.

Comment 24

[jhp (no longer active)]

'const patch' checked in to trunk.

Comment 25

[jhp (no longer active)]

Comment on attachment 203122 [details] [diff] [review]
Add functions for loading symbols from xul.dll, rev. 2

Requesting 1.8.0.1 approval.  This code is needed by Javaconnect to allow Java to load and call Mozilla functions exported by libXUL.

Comment 26

[Mike Schroepfer]

Comment on attachment 203122 [details] [diff] [review]
Add functions for loading symbols from xul.dll, rev. 2

Please consider for 1.8.1 - 1.8.0.1 is for major security and crash issues only.

Comment 27

[Mike Kaply [:mkaply]]

I think we are already having a major disconnect here. This is needed for the libxul release which will be on 1.8.0.1...

Comment 28

[Benjamin Smedberg]

Comment on attachment 203122 [details] [diff] [review]
Add functions for loading symbols from xul.dll, rev. 2

Re-requesting approval per drivers email, this does not affect Firefox or Thunderbird (which do not use GRE-finding facilities), and has baked on trunk.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 203122 [details] [diff] [review]
Add functions for loading symbols from xul.dll, rev. 2

a=dveditz for 1.8/1.8.0.1 branches. Please add the fixed1.8.1 and fixed1.8.0.1 keywords when checked in.

Comment 30

[Benjamin Smedberg]

Fixed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.