Closed
Bug 611405
Opened 14 years ago
Closed 14 years ago
about:memory crash in Firefox-on-xulrunner setups
Categories
(Toolkit Graveyard :: XULRunner, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla2.0b8
People
(Reporter: glandium, Assigned: glandium)
References
Details
(Keywords: crash)
Attachments
(2 files)
4.02 KB,
patch
|
benjamin
:
review+
christian
:
approval2.0+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
benjamin
:
review+
christian
:
approval2.0+
|
Details | Diff | Splinter Review |
(Setting Linux as OS, but that may affect other OSes ; this also affects any xulrunner application using the xulrunner-stub and wants to use jemalloc_stats) The memory reporter manager uses jemalloc_stats to get the memory statistics. libxul.so thus has a weak reference on this symbol, which is normally exported from firefox-bin. It happens to be exported from firefox-bin exactly for this reason: because libxul uses it, and firefox-bin links against libxul. But in xulrunner-stub case, libxul is dlopen()ed, which means the compile-time linker doesn't know it needs to export the jemalloc_stats symbol from the binary. There are IMHO two problems here. The first is the way xulrunner-stub is linked, which could be fixed to export jemalloc_stats using several methods: "-Wl,-E", "-rdynamic", or a version script to export jemalloc_stats alone. In any case, this might need various rules for various OSes. The second problem is in what can be read in xpcom/base/nsMemoryReporterManager.cpp: // NB: we don't null-check this symbol at runtime because we expect it // to have been resolved. If it hasn't, the crash jumping to NULL // will indicate the bug. Considering this comes in libxul.so, and considering an application linking against libxul.so is not guaranteed to be linked against jemalloc, it seems wrong to not gracefully fail here.
Instead of just failing if jemalloc_stats isn't resolved or if --disable-jemalloc, it would be better to fall back on the libc mallinfo(). This was discussed when the original code was written but it wasn't a high priority at the time.
Assignee | ||
Comment 2•14 years ago
|
||
Note that jemalloc_stats not being resolved, in this case, doesn't mean the libc malloc is being used...
so this probably explains bug 609257
Severity: normal → critical
Keywords: crash
Comment 4•14 years ago
|
||
It explains it indeed. That explains the difference we see with the mentioned versions there.
Assignee | ||
Comment 5•14 years ago
|
||
The rationale here is that programs are normally linked such that they don't export dynamic symbols. In the firefox-bin case, symbols end up being exported because libxul.so links back to these symbols. With this patch, programs still don't export dynamic symbols, except if they link to libjemalloc, in which case only jemalloc symbols will be exported: -rdynamic makes gcc call ld with -Wl,--export-dynamic, and the version script limits the exported symbols. I made it so that all programs linking against libjemalloc are treated equally, which will make it safer for firefox-bin, since symbol exporting won't be due to a side effect but to deliberate action.
Assignee: nobody → mh+mozilla
Attachment #490067 -
Flags: review?(benjamin)
Assignee | ||
Comment 6•14 years ago
|
||
Second part: avoid the crash when jemalloc_stats is not available.
Attachment #490069 -
Flags: review?(benjamin)
Updated•14 years ago
|
Attachment #490069 -
Flags: review?(benjamin) → review+
Updated•14 years ago
|
Attachment #490067 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #490067 -
Flags: approval2.0?
Assignee | ||
Updated•14 years ago
|
Attachment #490069 -
Flags: approval2.0?
Attachment #490067 -
Flags: approval2.0? → approval2.0+
Attachment #490069 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 9•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/46a51d427820 http://hg.mozilla.org/mozilla-central/rev/a7f3ef6ac23a
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Assignee | ||
Comment 10•14 years ago
|
||
Orange fix-up: http://hg.mozilla.org/mozilla-central/rev/ad227939db82
Updated•8 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•