Closed Bug 736564 Opened 12 years ago Closed 12 years ago

mozglue library should be included into xulrunner SDK

Categories

(Core :: mozglue, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: romaxa, Assigned: glandium)

References

Details

Attachments

(4 files, 2 obsolete files)

I'm building Mozilla Embedding (in process) and found that it stopped working recently after landing patch:
https://bug729940.bugzilla.mozilla.org/attachment.cgi?id=601861

Here is embedding I use: http://hg.mozilla.org/users/romaxa_gmail.com/qtmozembed
If I just revert whole Gecko hash patch it works fine.

here is the crash backtrace:
#0  0x00000000 in ?? ()
#1  0x476dd9b0 in PL_DHashTableOperate (table=0x1b01fb8, key=0x47eb2858, op=PL_DHASH_LOOKUP)
    at obj-build-linaro-qt-qws-dbg/xpcom/build/pldhash.cpp:612
#2  0x4770936e in GetEntry (aKey=..., this=0x1b01fb8) at ../../dist/include/nsTHashtable.h:170
#3  nsBaseHashtable<nsIDHashKey, nsFactoryEntry*, nsFactoryEntry*>::Get (this=0x1b01fb8, aKey=...) at ../../dist/include/nsBaseHashtable.h:148
#4  0x477094fa in nsComponentManagerImpl::RegisterCIDEntry (this=0x1b01f78, aEntry=0x480c0364, aModule=0x1b19b70)
    at xpcom/components/nsComponentManager.cpp:456
#5  0x4770b0be in nsComponentManagerImpl::RegisterModule (this=0x1b01f78, aModule=0x47fb37f4, aFile=<optimized out>)
    at xpcom/components/nsComponentManager.cpp:430
#6  0x4770bd02 in nsComponentManagerImpl::Init (this=0x1b01f78) at xpcom/components/nsComponentManager.cpp:380
#7  0x476e1446 in NS_InitXPCOM2_P (result=0x0, binDirectory=<optimized out>, appFileLocationProvider=<optimized out>)
    at xpcom/build/nsXPComInit.cpp:490
#8  0x46bfbe62 in XRE_InitEmbedding2 (aLibXULDirectory=0x1ad77d8, aAppDirectory=0x1ad7910, aAppDirProvider=<optimized out>)
    at toolkit/xre/nsEmbedFunctions.cpp:202
#9  0x0000dfbe in InitEmbedding (aProfilePath=<optimized out>) at gecko/EmbeddingSetup.cpp:289
#10 0x0000e986 in MozApp (aProfilePath=0x15524 "/home/romaxa/mozfoc", this=0x1aae9a8) at gecko/MozApp.cpp:104
#11 MozApp::CreateMozApp (aProfilePath=0x15524 "/home/romaxa/mozfoc") at gecko/MozApp.cpp:70
#12 0x00011200 in GeckoPreLoaded (aLoader=<optimized out>, aData=0xbec1d554) at microb.cpp:40
#13 0x00011124 in GeckoLoader::timerfired (this=0xbec1d530) at geckoloader.cpp:30
#14 0x4112ac78 in QMetaCallEvent::placeMetaCall (this=<optimized out>, object=<optimized out>) at kernel/qobject.cpp:525
#15 0x41138cac in QObject::event (this=0xbec1d530, e=<optimized out>) at kernel/qobject.cpp:1192
#16 0x404671b4 in notify_helper (e=0x1aae968, receiver=0xbec1d530, this=<optimized out>) at kernel/qapplication.cpp:4550
#17 QApplicationPrivate::notify_helper (this=<optimized out>, receiver=0xbec1d530, e=0x1aae968) at kernel/qapplication.cpp:4522
#18 0x4046e258 in QApplication::notify (this=0xbec1d540, receiver=0xbec1d530, e=0x1aae968) at kernel/qapplication.cpp:4411
#19 0x4110d1d0 in QCoreApplication::notifyInternal (this=0xbec1d540, receiver=<optimized out>, event=0x1aae968) at kernel/qcoreapplication.cpp:876
#20 0x4111248c in sendEvent (event=0x1aae968, receiver=0xbec1d530) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:231
#21 QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=<optimized out>, data=0x1a65a88) at kernel/qcoreapplication.cpp:1497
#22 0x4050806c in sendPostedEvents () at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:236
#23 QEventDispatcherQWS::processEvents (this=0x1a66488, flags=...) at kernel/qeventdispatcher_qws.cpp:92
#24 0x4110a1e4 in QEventLoop::processEvents (this=<optimized out>, flags=...) at kernel/qeventloop.cpp:149
#25 0x4110a468 in QEventLoop::exec (this=0xbec1d4fc, flags=...) at kernel/qeventloop.cpp:200
#26 0x41112858 in QCoreApplication::exec () at kernel/qcoreapplication.cpp:1148
#27 0x0000d5ec in main (argc=3, argv=<optimized out>) at microb.cpp:78
Attached file Simple embedding test
Attached file Make file (obsolete) —
./embeddingtest /path/to/firefox/folder
#4  0x00000000 in ?? ()
#5  0xb5bdef7c in HashKey (aKey=0xb6585658) at ../../dist/include/nsHashKeys.h:391
#6  nsTHashtable<nsBaseHashtableET<nsIDHashKey, nsFactoryEntry*> >::s_HashKey (table=0x9886e40, key=0xb6585658) at ../../dist/include/nsTHashtable.h:442
#7  0xb5ba7b29 in PL_DHashTableOperate (table=0x9886e40, key=0xb6585658, op=PL_DHASH_LOOKUP)
    at xpcom/build/pldhash.cpp:611
#8  0xb5bdf802 in GetEntry (aKey=..., this=0x9886e40) at ../../dist/include/nsTHashtable.h:170
#9  nsBaseHashtable<nsIDHashKey, nsFactoryEntry*, nsFactoryEntry*>::Get (this=0x9886e40, aKey=...) at ../../dist/include/nsBaseHashtable.h:148
#10 0xb5bdf9f1 in nsComponentManagerImpl::RegisterCIDEntry (this=0x9886e00, aEntry=0xb71205c8, aModule=0x989e960)
    at xpcom/components/nsComponentManager.cpp:456
#11 0xb5be2079 in nsComponentManagerImpl::RegisterModule (this=0x9886e00, aModule=0xb71424b0, aFile=0x0)

#5  0xb5bdef7c in HashKey (aKey=0xb6585658) at ../../dist/include/nsHashKeys.h:391
391	    return mozilla::HashBytes(aKey, sizeof(KeyType));
(gdb) p aKey
$1 = (nsIDHashKey::KeyTypePointer) 0xb6585658
(gdb) x/wa 0xb6585658
0xb6585658 <_ZL20kComponentManagerCID>:	0x91775d60
Sounds like embedding just don't see these functions and unable to call it.
Ok, seems adding:
-Wl,--whole-archive $OBJ_DIST_BIN/../lib/libmozglue.a -lpthread  -Wl,--no-whole-archive -rdynamic

Helps to solve this problem
and libmozglue.a not in SDK
by the same reason it fail to load jemalloc functions
Attached patch Add mozglue to SDK (obsolete) — Splinter Review
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #606913 - Flags: review?(benjamin)
(In reply to Oleg Romashin (:romaxa) from comment #7)
> by the same reason it fail to load jemalloc functions

Yeah, it's HashBytes, which is used to hash nsID's.
Summary: Hash tables gecko update seems broke Embedding XPCOM init → mozglue library should be included into xulrunner SDK
This patch seems to imply that we're going to require all embedders to link their binary against mozglue/jemalloc. That has previously not been the case, and will completely break some existing usages like python embeddings where you don't modify the python binary.

If we're going to do this, I think it should be proposed more explicitly and we should understand the tradeoffs. For now we should not require this linkage, and functions such as hashbytes should be moved to a different library (such as the XPCOM glue) that is required and doesn't have to be linked to the root binary.
(or we should separate mfbt from mozutils)
Component: XPCOM → mozglue
QA Contact: xpcom → mozglue
I understand not wanting to link against jemalloc, but what about MOZ_ASSERT, which currently lives out-of-line in mozglue, or other functions we might want to define not-inline in mfbt?

I can just make HashBytes inline, if that's satisfactory to everyone.  I don't want to move it into a Gecko-only library if I can help it.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> (or we should separate mfbt from mozutils)

And jemalloc?
> I can just make HashBytes inline, if that's satisfactory to everyone.  I
> don't want to move it into a Gecko-only library if I can help it.
yep, making HashBytes as inline would work too, that was a first thing I did. should I attach patch with that?
(In reply to Oleg Romashin (:romaxa) from comment #14)
> > I can just make HashBytes inline, if that's satisfactory to everyone.  I
> > don't want to move it into a Gecko-only library if I can help it.
> yep, making HashBytes as inline would work too, that was a first thing I
> did. should I attach patch with that?

That's fine with me, at least until we get the larger mess sorted out.
Maybe we should just undo Bug 677501, which combined them in the first place...
Essentially, this is a linux-only problem. Mac and windows don't have the problem because mozglue is a shared library there.

For embedding on linux, we should ship libmozglue.a as part of the SDK, but as bsmedberg says, that doesn't solve the problem for e.g. python. For that one or similar other cases, what could work is a separate libmozglue dynamic library, that would come with the SDK as well (i guess). The embedding app would dlopen it before loading libxul/libxpcom, and the dynamic linker would take the missing symbols from there.

Alternatively, we could have libmozglue.a split in a static and a dynamic part. The static part would be optional when linking (mostly, jemalloc), and the dynamic part would be loaded at runtime.

Moving mfbt to the XPCOM glue would make it impossible to use in the custom dynamic linker, which lives in libmozglue (and it currently uses MOZ_ASSERT and other stuff).
Why can't we just remove jemalloc from mozglue (on Linux) and ship it separately?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> Why can't we just remove jemalloc from mozglue (on Linux) and ship it
> separately?

that wouldn't solve the embedding problem.
Why not? In that case embedders can be required to link against mozglue and can choose whether to link against jemalloc, right?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> Why not? In that case embedders can be required to link against mozglue and
> can choose whether to link against jemalloc, right?

That doesn't solve the problem for python you mentioned in comment 10
Why not? pyxpcom would not link against jemalloc, would link against mozglue, and it would work correctly, no?
So what should be done here?
Let me take this.
Assignee: romaxa → mh+mozilla
Attachment #617812 - Flags: review?(ted.mielczarek)
Attachment #617812 - Flags: review?(benjamin)
Attachment #617812 - Flags: review?(ted.mielczarek) → review+
Attachment #606913 - Attachment is obsolete: true
Attachment #606913 - Flags: review?(benjamin)
Attachment #617812 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
OS: Linux → All
Hardware: ARM → All
Please take checkin-needed off the bug when a patch lands on inbound.
Flags: in-testsuite-
Keywords: checkin-needed
So, I landed this today, but I found out this creates a problem with upcoming jemalloc3. Basically, we also need to put the stuff from memory/build in the jemalloc library we ship in the SDK. Doing this the easy way (without modifying current LIBRARY_NAMEs and so on) would mean shipping a "libmemory.a" library instead of "libjemalloc.a". On one hand, libmemory is a pretty generic name, on the other hand, it has the advantage of allowing any malloc library to be used without having to rename the library shipped in the SDK, and thus avoiding third parties some headaches.

bsmedberg, would you be okay with shipping a libmemory.a in the SDK, or do you prefer libjemalloc.a, or some other name?
Assignee: mh+mozilla → nobody
Component: mozglue → Networking
OS: All → Linux
QA Contact: mozglue → networking
Hardware: All → ARM
Target Milestone: mozilla15 → mozilla14
Version: Trunk → Other Branch
Assignee: nobody → mh+mozilla
Component: Networking → mozglue
OS: Linux → All
QA Contact: networking → mozglue
Hardware: ARM → All
Target Milestone: mozilla14 → mozilla15
Version: Other Branch → Trunk
libmemory.a is fine.
Whiteboard: [leave open after inbound merge]
Attachment #620713 - Flags: review?(ted.mielczarek) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9138b968b9e8
Whiteboard: [leave open after inbound merge]
https://hg.mozilla.org/mozilla-central/rev/4b4f59ac01ac
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 751871
Attachment #606850 - Attachment is obsolete: true
Attachment #621506 - Attachment mime type: application/x-sh → text/plain
Depends on: 763152
You need to log in before you can comment on or make changes to this bug.