Last Comment Bug 509244 - gfx crash on memory-pressure notification
: gfx crash on memory-pressure notification
Status: RESOLVED FIXED
: [nv]
: fixed1.9.0.15
Product: Core
Classification: Components
Component: Graphics
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Vladimir Vukicevic (:vlad)
: thebes
:
:
: 419125
  Show dependency treegraph
 
Reported: 2009-08-08 13:32 PDT by Vladimir Vukicevic (:vlad)
Modified: 2009-11-01 21:08 PST (History)
11 users (show)
vladimir: blocking1.9.2+
dveditz: blocking1.9.0.15+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
karlt: in‑testsuite+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1-fixed
  .4+
  .4-fixed


Attachments
fix (549 bytes, patch)
2009-08-08 13:32 PDT, Vladimir Vukicevic (:vlad)
karlt: review+
pavlov: superreview+
dveditz: approval1.9.1.4+
dveditz: approval1.9.0.15+
Details | Diff | Splinter Review
mochitest crash test (2.19 KB, patch)
2009-09-16 16:25 PDT, Karl Tomlinson, offline (:karlt)
no flags Details | Diff | Splinter Review

Summon comment box

Description Vladimir Vukicevic (:vlad) 2009-08-08 13:32:44 PDT
Created attachment 393386 [details] [review]
fix

Went over this with karlt and stuart in irc..

13:22 < vlad> so there's this function and code: http://mxr.mozilla.org/mozilla-
central/source/gfx/src/thebes/nsThebesDeviceContext.cpp#258
13:23 < vlad> it calls NS_RELEASE and then maybe addrefs again
13:23 < vlad> but
13:23 < vlad> http://mxr.mozilla.org/mozilla-central/source/gfx/src/thebes/nsThebesFontMetrics.cpp#59
13:23 < vlad> Destroy() in nsThebesFontMetrics doesn't do anything
13:24 < vlad> and the destructor just deletes a member
13:24 < vlad> so it'll never get removed from that font cache array.. and we'll addref bogus memory, no?
13:25 < vlad> as best I can tell from mxr, nothing ever calls FontMetricsDeleted in our code
13:25 < vlad> (I'm hitting a crash in Compact, when I issue a memory pressure notification)
13:32 < karl> yeah, doesn't look right to me either
13:32 < vlad> I don't even know what that font cache does
13:32 < vlad> I trhink I have it fixed, at least not to crash
13:32 < vlad> I just call the FontMetricsDeleted from the destructor on mDeviceContext
13:33 < karl> i thought it did look right at one time in the past, but maybe i just believed the comments
13:36 < karl> this pattern appears quite a bit:
13:36 < karl> fm->Destroy();
13:36 < karl> NS_RELEASE(fm);
13:37 < vlad> fm->Destroy() should set its device context to null from I can tell
13:37 < vlad> and actually releasing it shouldn' call FontMetricsDeleted if the dc is null
13:37 < vlad> in the places where that pattern appears, fm is destroyed/released only if initialization fails
13:40 < karl> are you seeing the crash in AddRef()?
13:43 < vlad> in the release, actually
13:43 < vlad> the addrefs happen too quickly, the memory still looks semi-valid
13:43 < karl> looks like FontMetricsDeleted is meant to be called from FontMetricsDeleted
13:43 < vlad> but the second time around through a memory pressure notifier after that memory got reused
13:43 < karl> i mean
13:44 < karl> looks like FontMetricsDeleted is meant to be called from ~nsThebesFontMetrics
Comment 1 timeless 2009-08-08 14:45:38 PDT
this sounds like a bug romaxa filed
Comment 2 timeless 2009-08-13 00:07:29 PDT
the bug i had in mind is bug 503784.

note that not filing bugs with crash signatures as you did here means that it's hard for people to find duplicates. please cooperate with the other users of bugzilla by including crash signatures in your bug summaries.
Comment 3 timeless 2009-08-13 00:09:06 PDT
i'd like to further point out that i specifically asked you, vlad, for a review in that bug, and you bounced the review. i consider this inappropriate.
Comment 4 Vladimir Vukicevic (:vlad) 2009-08-14 13:56:16 PDT
*** Bug 503784 has been marked as a duplicate of this bug. ***
Comment 5 Vladimir Vukicevic (:vlad) 2009-08-14 13:56:26 PDT
http://hg.mozilla.org/mozilla-central/rev/11bd05bebf85

I am, as part of my job, doing fewer gfx reviews, which is why that review was bounced (as were most gfx-related reviews that would have good reviewers amongst the gfx team).
Comment 6 Brian Crowder 2009-08-22 01:15:21 PDT
I'm still seeing a bug that looks a lot like this when I use the faststart component's memory-pressure feature:

>	xul.dll!nsFontCache::Compact()  Line 272 + 0x9 bytes	C++
 	xul.dll!nsThebesDeviceContext::Observe(nsISupports * aSubject=0x00000000, const char * aTopic=0x0521e298, const wchar_t * aSomeData=0x0364cec8)  Line 358	C++
 	xul.dll!nsObserverList::NotifyObservers(nsISupports * aSubject=0x00000000, const char * aTopic=0x0521e298, const wchar_t * someData=0x0364cec8)  Line 129	C++
 	xul.dll!nsObserverService::NotifyObservers(nsISupports * aSubject=0x00000000, const char * aTopic=0x0521e298, const wchar_t * someData=0x0364cec8)  Line 185	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000005, unsigned int methodIndex=0x00000003, unsigned int paramCount=0x0012d8f0, nsXPTCVariant * params=0x00000001)  Line 102	C++
 	xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2710 + 0x20 bytes	C++
Comment 7 Stuart Parmenter 2009-08-27 17:17:06 PDT
we're still seeing this crash
Comment 8 Vladimir Vukicevic (:vlad) 2009-08-27 18:29:14 PDT
Hmm, my comment was lost earlier.  Need more information from the crash, like looking at data and all that.  karlt should look at/fix this, because we can probably just get rid of most of this code.
Comment 9 Stuart Parmenter 2009-08-27 22:58:10 PDT
just take a look at http://mxr.mozilla.org/mozilla-central/source/gfx/src/thebes/nsThebesDeviceContext.cpp#258 -- its use of oldfm is clearly horrible.
Comment 10 Stuart Parmenter 2009-08-27 23:02:04 PDT
or maybe the code is just gross, and not wrong.  can get a stack -- it crashes all the time on maemo.
Comment 11 Karl Tomlinson, offline (:karlt) 2009-08-28 11:55:55 PDT
A bug was fixed here.  Let's fix the next bug in bug 503784.
Comment 13 Samuel Sidler (old account; do not CC) 2009-09-02 23:14:00 PDT
Comment on attachment 393386 [details] [review]
fix

Pushing out approval requests...
Comment 14 Henrik Skupin (:whimboo) 2009-09-04 03:07:38 PDT
Tony, due to the lack of a device I'm not able to verify this fix. Could you or Marcia check if the crash went away? Thanks.
Comment 15 Karl Tomlinson, offline (:karlt) 2009-09-04 03:19:43 PDT
The bugs that depend on bug 503784 seem to confirm that this is now fixed but we should be able to write a test using nsIMemory::heapMinimize().
Comment 16 Daniel Veditz 2009-09-04 10:15:34 PDT
Comment on attachment 393386 [details] [review]
fix

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers
Comment 18 Karl Tomlinson, offline (:karlt) 2009-09-16 16:25:12 PDT
Created attachment 401127 [details] [review]
mochitest crash test

This uses the observer service to send a memory pressure notification.
It actually doesn't crash 1.9.0 and 1.9.1 because of another bug that meant the observer was not registered (bug 419125 comment 6).
http://hg.mozilla.org/mozilla-central/rev/c9b6282f0db7

The observer was registered before 1.9.2a1, so we should wait till those users are offered 1.9.2b1 before landing this test.

The fix for this bug may still be worthwhile on 1.9.0 and 1.9.1 because nsFontCache::Compact() can still be called in some unusual situations if fm->Init() should fail:
http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/b7dd9891657f/gfx/src/nsDeviceContext.cpp#l519
Comment 19 Al Billings [:abillings] 2009-09-16 16:53:35 PDT
So, reading comment 18, there is no crash on 1.9.0 because of a blocking bug (which may or may not be addressed at some point) and the only way to crash it is via the mochitest anyway? Is this mochitest going to be checked into 1.9.0?
Comment 20 Karl Tomlinson, offline (:karlt) 2009-09-16 17:18:00 PDT
(In reply to comment #19)
> So, reading comment 18, there is no crash on 1.9.0 because of a blocking bug
> (which may or may not be addressed at some point)

The mochitest doesn't crash 1.9.0 because of bug 419125 comment 6, but there could still be a crash on 1.9.0.

> and the only way to crash it is via the mochitest anyway?

Mochitest enables UniversalXPConnect, which makes it easy to produce the crash on 1.9.2.  This is definitely not the only way to crash on 1.9.2.  I expect there are ways to crash on 1.9.0 also, but I don't know what they are.

The file in this patch does not need to be run under mochitest.  To produce the crash, it is enough just to open the file and manually accept the privilege request.

> Is this mochitest going to be checked into 1.9.0?

It could be.  It wouldn't test this bug (unless other things change) but it would test other things.
Comment 21 Al Billings [:abillings] 2009-09-17 09:58:56 PDT
I'm not concerned with 1.9.2. I work on the 1.9.0 and 1.9.1 security releases. This is fixed in both of those and I'm trying to verify the fix. Since this bug lacks repro steps and the mochitest only works on 1.9.2, it is difficult to reproduce the issue on the branches with which I am concerned.

Note You need to log in before you can comment on or make changes to this bug.