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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

#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,

Comment 1

[Oleg Romashin (:romaxa)]

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);
        }

Comment 2

[timeless]

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.

Comment 3

[Oleg Romashin (:romaxa)]

Can we something about it?

Comment 4

[timeless]

Attachment: we could probably do this

Comment 6

[Stuart Parmenter]

reopening since this has a patch

Comment 7

[Brian Crowder]

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.

Comment 8

[Karl Tomlinson (:karlt)]

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.

Comment 9

[Karl Tomlinson (:karlt)]

-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

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: not always true

Comment 11

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/52abed9ff3d3
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ee3bcae652e9

Comment 12

[Daniel Veditz [:dveditz]]

Should we land this on the older branches as well?

Comment 13

[Karl Tomlinson (:karlt)]

Not needed on other branches.  This was a regression from
http://hg.mozilla.org/mozilla-central/rev/7366df357e91

nsVoidArray::IndexOf() returned PRInt32 (not PRUint32).