8.71 KB, text/x-c++src
4.08 KB, patch
Benjamin Smedberg: review+
|Details | Diff | Splinter Review|
3.19 KB, patch
|Details | Diff | Splinter Review|
395 bytes, text/plain
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
./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
Created attachment 606913 [details] [diff] [review] Add mozglue to SDK
(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.
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)
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.
Created attachment 617812 [details] [diff] [review] Ship mozglue and jemalloc libraries in the SDK
Please take checkin-needed off the bug when a patch lands on inbound.
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?
libmemory.a is fine.
Created attachment 620713 [details] [diff] [review] Fixup for - Ship libmemory.a in the SDK instead of libjemalloc.a
Created attachment 621506 [details] New make.sh which work with latest m-c