Closed Bug 611405 Opened 9 years ago Closed 9 years ago

about:memory crash in Firefox-on-xulrunner setups

Categories

(Toolkit Graveyard :: XULRunner, defect, critical)

All
Linux
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Keywords: crash)

Attachments

(2 files)

(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.
Note that jemalloc_stats not being resolved, in this case, doesn't mean the libc malloc is being used...
Blocks: 610488
Blocks: 609257
No longer blocks: 610488
so this probably explains bug 609257
Severity: normal → critical
Keywords: crash
It explains it indeed. That explains the difference we see with the mentioned versions there.
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)
Second part: avoid the crash when jemalloc_stats is not available.
Attachment #490069 - Flags: review?(benjamin)
Duplicate of this bug: 609257
Attachment #490069 - Flags: review?(benjamin) → review+
Attachment #490067 - Flags: review?(benjamin) → review+
Attachment #490067 - Flags: approval2.0?
Attachment #490069 - Flags: approval2.0?
Attachment #490067 - Flags: approval2.0? → approval2.0+
Attachment #490069 - Flags: approval2.0? → approval2.0+
a=LegNeato for m-c.
http://hg.mozilla.org/mozilla-central/rev/46a51d427820
http://hg.mozilla.org/mozilla-central/rev/a7f3ef6ac23a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
No longer blocks: 609257
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.