151604 - XPCOM Glue standalone should dynamically load symbols

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

The glue library should stub the public frozen functions that XPCOM provides
(see xpcom/build/nsXPCOM.h). Then glue library should dynamically load these
symbols from the XPCOM library. This will allow two important things to happen. 

First, it will allow the component or application to not have to link with XPCOM
for any symbol.  In fact having to linking to any Gecko library after this
dynamic loading happens will be an immediate indication that something is wrong.  

The second important reason is targeted at the application embedder. If
the glue code does load XPCOM, this same glue code can unload XPCOM and thereby
taking a big step to allow unloading gecko when it is not used. This will allow
many applications to shutdown Gecko when it is not needed and restart Gecko at will.

Patch coming up...

Comment 1

[Doug Turner (:dougt)]

"component or application to not have to link with XPCOM"  - this applies mostly
to embedding application.  

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

This patch does a couple things:

1.  Adds XPCOM_GLUE to mean the same thing as _IMPL_NS_COM_OFF as far as dll
importing goes.  It is just a cleaner name that is easier to remember.

2.  You will see the addition of two new files to xpcom/glue/standalone.  These
two files do the dirty work of loading xpcom, finding the right symbols, and
proxing the call into the xpcom library.  

These two files expose two new public functions:

nsresult NS_COM XPCOMGlueStartup(const char* xpcomFile)
nsresult NS_COM XPCOMGlueShutdown()

In order to load xpcom, we need to know where to find it.  I am leaving this up
to the caller.	I imagine different users of XPCOM will want to point this at
different places.  The embedding API may want to use this and tie the location
it passes to the MRE.  In any case,  the location is what gets passed to
PR_LoadLibrary().

3.  I created a new private API exposed by XPCOM called NS_GetFrozenMethods. 
This is a function is suppose to be used only by the glue library.  The
function will fill out a structure of frozen functions which XPCOM implements. 
Instead of looking for each symbol separately, Rick and I thought that this
approach was simplier.	Notice that the structure is allocated via malloc and
free, not the xpcom allocator.	The embedder owns this memory.

4.  xpcom/glue/standalone now depends on strings since the public methods it
stubs needs these headers.  

5.  In the shutdown of the glue, I have some windows heap compation fu.  This
stuff was removed from window close during M9.	I find it to be extremely
helpful when shutting down.  Of course this doesn't mean jack since we don't
restart (yet.  I have a prototype of Gecko restarting without many problems.)  


6.  I stubbed out nsTraceRefcnt so that the standalone glue could be built
without looking for this class in xpcom.  I can #define DEBUG_dougt if this is
too ugly for real world consumption.

So, the next question is, "what does this effect?".  The simply answer is
nothing right now.  No one uses this stuff but very shortly, I will be checkin
in embedding code that does use this and supports Gecko restarting (embedders
choice, of course).

Comments?

Comment 3

[Alec Flett]

Comment on attachment 87552 [details] [diff] [review]
patch v.1

ok, I think the general approach looks good.. few questions though:

* why do we need to do resident set shrinkage here? I'm kinda thinking we let
the application do it (like the comment says)

* why don't we just do	implicit initialization - we're taking the time to
check if (!initialized) ... then we wouldn't need to export XPCOMGlueStartup

* with that, you could call XPCOMGlueShutdown from NS_XPCOMShutdown() or
something like that, and stop exporting that.

* I'm not sure of the advantage of putting all these reserved variables in the
middle of the struct - at the very least they should be at the end, after the
register/unregisterExitRoutine stuff.
However, the way I've seen microsoft handle this is to keep a "size" member
variable which should always equal sizeof(structname) - and then not have any
reserved space. depending on usage, this should allow you to pass around a
pointer to the object, and use the size if you're ever making a copy of it.
when extending the structure, you just extend off the end and existing slots
shouldn't be affected.

* typedefs for function types - I realize they're somewhat private to
nsXPCOMPrivate.h, but maybe should we maybe append "Func" or "Callback" to the
type name? I don't like having a typedef named "Shutdown"   "Init" etc...

Comment 4

[Doug Turner (:dougt)]

>* why do we need to do resident set shrinkage here? I'm kinda thinking we let
the application do it (like the comment says)

I will comment it out.  But wow - you should see the numbers.  without this, the
heap remains bloated by 10mb or so; the fix pulls us very close to pre gecko
footprint.

> * why don't we just do implicit initialization - we're taking the time to
check if (!initialized) ... then we wouldn't need to export XPCOMGlueStartup

We will have no idea where the xpcom library is.  Consider the case where there
is a MRE and the xpcom library is system wide.  In the same case, you can
suppose there is an application what wants to use this with their own private
"MRE".  Bottom line is that someone needs to tell us where the library is and
that policy should not live in xpcom/glue/standalone.

>* with that, you could call XPCOMGlueShutdown from NS_XPCOMShutdown() or
something like that, and stop exporting that.

I don't want to tie this to xpcom proper.  It is stub code.  However, I am going
to tie this to InitEmbedding.  I have patches which already convert all of the
(windows) test application over to this.  Stay tuned for these changes.

> I'm not sure of the advantage of putting all these reserved variables in the
middle of the struct - at the very least they should be at the end, after the
register/unregisterExitRoutine stuff. However, the way I've seen microsoft
handle this is to keep a "size" member variable which should always equal
sizeof(structname) - and then not have any reserved space. depending on usage,
this should allow you to pass around a pointer to the object, and use the size
if you're ever making a copy of it. when extending the structure, you just
extend off the end and existing slots shouldn't be affected.

removed the reserved fields.  added a PRUint32 which is the size of the structure.  

> * typedefs for function types - I realize they're somewhat private to
nsXPCOMPrivate.h, but maybe should we maybe append "Func" or "Callback" to the
type name? I don't like having a typedef named "Shutdown"   "Init" etc..

Sure.


New patch with these fixes coming up.

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.2

fixed alec's concerns. (thanks!)
renamed methods to functions
added static global flag to detect failure to unload.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.3

includes dp's code review:
- XPCOMGlueStartup can take a null argument which means the first xpcom library
found in system path. 
- Clean up of initalization to avoid crashes (eg. failed init followed by a
call to shutdown).
- reuse of xpcomFunctions variable to mean the glue has been initalized.

Comment 7

[Suresh Duddi (gone)]

Comment on attachment 88481 [details] [diff] [review]
patch v.3

r=dp

Comment 8

[Alec Flett]

Comment on attachment 88481 [details] [diff] [review]
patch v.3

sr=alecf - good idea on the global init stuff

I see what you mean about not tying this to xpcom proper too. 

we should definitely document the compressworkingset stuff for embeddors though
- pretty cool that you can get it that low.

Comment 9

[Doug Turner (:dougt)]

Checking in base/nscore.h;
/cvsroot/mozilla/xpcom/base/nscore.h,v  <--  nscore.h
new revision: 1.50; previous revision: 1.49
done
Checking in build/nsXPCOM.h;
/cvsroot/mozilla/xpcom/build/nsXPCOM.h,v  <--  nsXPCOM.h
new revision: 1.7; previous revision: 1.6
done
Checking in build/nsXPCOMPrivate.h;
/cvsroot/mozilla/xpcom/build/nsXPCOMPrivate.h,v  <--  nsXPCOMPrivate.h
new revision: 1.3; previous revision: 1.2
done
Checking in build/nsXPComInit.cpp;
/cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v  <--  nsXPComInit.cpp
new revision: 1.142; previous revision: 1.141
done
Checking in components/xcDll.cpp;
/cvsroot/mozilla/xpcom/components/xcDll.cpp,v  <--  xcDll.cpp
new revision: 1.60; previous revision: 1.59
done
Checking in glue/Makefile.in;
/cvsroot/mozilla/xpcom/glue/Makefile.in,v  <--  Makefile.in
new revision: 1.11; previous revision: 1.10
done
Checking in glue/standalone/Makefile.in;
/cvsroot/mozilla/xpcom/glue/standalone/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.cpp,v
done
Checking in glue/standalone/nsXPCOMGlue.cpp;
/cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.cpp,v  <--  nsXPCOMGlue.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.h,v
done
Checking in glue/standalone/nsXPCOMGlue.h;
/cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.h,v  <--  nsXPCOMGlue.h
initial revision: 1.1
done
Checking in sample/Makefile.in;
/cvsroot/mozilla/xpcom/sample/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done