Closed Bug 503784 Opened 11 years ago Closed 11 years ago

nsFontCache::Compact crashes on heap-minimize notification [@ nsFontCache::Compact]

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- unaffected
fennec 1.0+ ---

People

(Reporter: romaxa, Assigned: timeless)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

#0  0x00000049 in ?? ()
#1  0xb4d38da4 in nsFontCache::Compact (this=0x9ba1530)
    at mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp:272
#2  0xb4d3976c in nsThebesDeviceContext::Observe (this=0x9b33968, aSubject=0x0, aTopic=0xb77db50a "memory-pressure", aSomeData=0xb77dbda0)
    at mozilla/gfx/src/thebes/nsThebesDeviceContext.cpp:356
#3  0xb7607b9a in nsObserverList::NotifyObservers (this=0x9c0fdb0, aSubject=0x0, aTopic=0xb77db50a "memory-pressure", someData=0xb77dbda0)
    at mozilla/xpcom/ds/nsObserverList.cpp:128
#4  0xb760802f in nsObserverService::NotifyObservers (this=0x9763e70, aSubject=0x0, aTopic=0xb77db50a "memory-pressure", someData=0xb77dbda0)
    at mozilla/xpcom/ds/nsObserverService.cpp:181
#5  0xb77bbc43 in g_mozilla_engine_observe (service_id=0x0, object=0x0, topic=0xb77db50a "memory-pressure", data=0xb77dbda0)
    at ../../../src/gecko/gmozillacppwrapper.cpp:630
#6  0xb77aabf5 in load_finished_cb (embed=0x96c8180, self=0x96e0010) at ../../src/gmozillaengine.c:2330
#7  0xb7ed641d in IA__g_cclosure_marshal_VOID__VOID (closure=0x9823f68, return_value=0x0, n_param_values=1, param_values=0x9998a80, 
    invocation_hint=0xbfba691c, marshal_data=0xb77aaaf0) at /home/bifh5/fremantle-i386-prereleased.cs2007q3/work/glib2.0-2.20.1/gobject/gmarshal.c:77
#8  0xb7ec94ec in IA__g_closure_invoke (closure=0x9823f68, return_value=0x0, n_param_values=1, param_values=0x9998a80, invocation_hint=0xbfba691c)
    at /home/bifh5/fremantle-i386-prereleased.cs2007q3/work/glib2.0-2.20.1/gobject/gclosure.c:767
#9  0xb7edd8e4 in signal_emit_unlocked_R (node=0x96df100, detail=0, instance=0x96c8180, emission_return=0x0, instance_and_params=0x9998a80)
    at /home/bifh5/fremantle-i386-prereleased.cs2007q3/work/glib2.0-2.20.1/gobject/gsignal.c:3247
#10 0xb7edf5ca in IA__g_signal_emit_valist (instance=0x96c8180, signal_id=155, detail=0,
Problem is next:
        nsIFontMetrics* fm = mFontMetrics[i];
        nsIFontMetrics* oldfm = fm;
>>>>        oldfm and fm - is not null
        // Destroy() isn't here because we want our device context to be
        // notified
        NS_RELEASE(fm); // this will reset fm to nsnull
>>>>fm - is null, and oldfm - pointer to destroyed fm data. not valid

        // if the font is really gone, it would have called back in
        // FontMetricsDeleted() and would have removed itself
        if (mFontMetrics.IndexOf(oldfm) >= 0) {
            // nope, the font is still there, so let's hold onto it too
>>>>            addrefing not valid pointer.
            NS_ADDREF(oldfm);
        }
the problem seems to be:
http://mxr.mozilla.org/mozilla-central/ident?i=FontMetricsDeleted

the comment expects that to be called

this code worked in 1.8 times and died with thebes.
Severity: normal → critical
Keywords: crash
Summary: nsFontCache::Compact crashes on heap-minimize notification → nsFontCache::Compact crashes on heap-minimize notification [@ nsFontCache::Compact]
Can we something about it?
Severity: critical → normal
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #388239 - Flags: review?(vladimir)
Attachment #388239 - Flags: review?(vladimir) → review?(jdaggett)
Severity: normal → critical
Flags: blocking1.9.2?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 509244
Attachment #388239 - Flags: review?(pavlov)
Attachment #388239 - Flags: review?(pavlov) → review+
reopening since this has a patch
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
When I discussed this patch with karlt, I recall him suggesting that it might not be a good thing, but I don't remember precisely why.
My concern was that fonts that are still in use get removed from the cache, so the next time the same font is needed a new (duplicate) font will be instantiated.

The bug is that mFontMetrics.IndexOf(oldfm) returns PRUint32,
which is always >= 0.

The comparison needs to be with mFontMetrics.NoIndex.
Blocks: 451670
-Wtype-limits with gcc-4.3 or -W(extra) on earlier versions would have found this:

/home/karl/moz/dev/gfx/src/thebes/nsThebesDeviceContext.cpp: In member function 'nsresult nsFontCache::Compact()':
/home/karl/moz/dev/gfx/src/thebes/nsThebesDeviceContext.cpp:270: warning: comparison of unsigned expression >= 0 is always true
Attached patch not always trueSplinter Review
Attachment #397379 - Flags: review?
Attachment #397379 - Flags: review? → review?(jdaggett)
Attachment #397379 - Flags: superreview+
Attachment #397379 - Flags: review?(jdaggett)
Attachment #397379 - Flags: review+
Attachment #397379 - Flags: approval1.9.2+
tracking-fennec: --- → 1.0+
Flags: blocking1.9.2? → blocking1.9.2+
http://hg.mozilla.org/mozilla-central/rev/52abed9ff3d3
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ee3bcae652e9
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: fixed1.9.2
Resolution: --- → FIXED
Blocks: 474369
Blocks: 513091
Blocks: 510955
Blocks: 509538
Blocks: 501704
Depends on: 514107
Should we land this on the older branches as well?
blocking1.9.1: --- → ?
status1.9.1: --- → ?
Flags: wanted1.9.0.x?
blocking1.9.1: ? → ---
Not needed on other branches.  This was a regression from
http://hg.mozilla.org/mozilla-central/rev/7366df357e91

nsVoidArray::IndexOf() returned PRInt32 (not PRUint32).
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
Attachment #388239 - Flags: review?(jdaggett)
Crash Signature: [@ nsFontCache::Compact]
You need to log in before you can comment on or make changes to this bug.