Last Comment Bug 723472 - reftest graphite-03a causes spurious "out of memory" crash due to zero-sized allocation request
: reftest graphite-03a causes spurious "out of memory" crash due to zero-sized ...
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla13
Assigned To: Jonathan Kew (:jfkthame)
:
: Milan Sreckovic [:milan]
Mentors:
: 732316 (view as bug list)
Depends on:
Blocks: 680556 700022
  Show dependency treegraph
 
Reported: 2012-02-02 04:55 PST by John Daggett (:jtd)
Modified: 2012-03-05 16:39 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
fixed


Attachments
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized (1.27 KB, patch)
2012-02-02 06:54 PST, Jonathan Kew (:jfkthame)
cjones.bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description John Daggett (:jtd) 2012-02-02 04:55:52 PST
graphite-03a stacktrace on winxp:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jdaggett@mozilla.com-ea2710e4d8f8/try-win32-debug/try_xp-debug_test-reftest-build2820.txt.gz

 0  mozalloc.dll!mozalloc_abort(char const * const) [mozalloc_abort.cpp:ea2710e4d8f8 : 79 + 0x0]
 1  mozalloc.dll!mozalloc_handle_oom(unsigned int) [mozalloc_oom.cpp:ea2710e4d8f8 : 60 + 0x9]
 2  mozalloc.dll!moz_xrealloc [mozalloc.cpp:ea2710e4d8f8 : 137 + 0x7]
 3  xul.dll!graphite2::vm::Machine::Code::Code(bool,unsigned char const *,unsigned char const * const,unsigned char,unsigned short,graphite2::Silf const &,graphite2::Face const &) [Code.cpp:ea2710e4d8f8 : 210 + 0x9]
 4  xul.dll!graphite2::Pass::readRules(unsigned char const *,unsigned int,unsigned char const *,unsigned short const *,unsigned short const *,unsigned char const *,unsigned short const *,unsigned char const *,graphite2::Face const &) [Pass.cpp:ea2710e4d8f8 : 192 + 0x28]
 5  xul.dll!graphite2::Pass::readPass(void *,unsigned int,unsigned int,graphite2::Face const &) [Pass.cpp:ea2710e4d8f8 : 152 + 0x1f]
 6  xul.dll!graphite2::Silf::readGraphite(void const *,unsigned int,graphite2::Face const &,unsigned int) [Silf.cpp:ea2710e4d8f8 : 177 + 0x1a]
 7  xul.dll!graphite2::Face::readGraphite() [Face.cpp:ea2710e4d8f8 : 107 + 0x17]
 8  xul.dll!gr_make_face [gr_face.cpp:ea2710e4d8f8 : 60 + 0x8]
 9  xul.dll!gfxGraphiteShaper::ShapeWord(gfxContext *,gfxShapedWord *,wchar_t const *) [gfxGraphiteShaper.cpp:ea2710e4d8f8 : 174 + 0xe]
10  xul.dll!gfxGDIFont::ShapeWord(gfxContext *,gfxShapedWord *,wchar_t const *,bool) [gfxGDIFont.cpp:ea2710e4d8f8 : 166 + 0x1a]
11  xul.dll!gfxFont::GetShapedWord<wchar_t>(gfxContext *,wchar_t const *,unsigned int,unsigned int,int,int,unsigned int) [gfxFont.cpp:ea2710e4d8f8 : 1910 + 0xd]
12  xul.dll!gfxFont::SplitAndInitTextRun<wchar_t>(gfxContext *,gfxTextRun *,wchar_t const *,unsigned int,unsigned int,int) [gfxFont.cpp:ea2710e4d8f8 : 2103 + 0x19]
13  xul.dll!gfxFontGroup::InitScriptRun<wchar_t>(gfxContext *,gfxTextRun *,wchar_t const *,unsigned int,unsigned int,int) [gfxFont.cpp:ea2710e4d8f8 : 3205 + 0x14]
14  xul.dll!gfxFontGroup::InitTextRun<wchar_t>(gfxContext *,gfxTextRun *,wchar_t const *,unsigned int) [gfxFont.cpp:ea2710e4d8f8 : 3151 + 0x2a]

Not sure why an OOM would occur here...
Comment 1 Jonathan Kew (:jfkthame) 2012-02-02 05:09:35 PST
Strange, those tests all passed tryserver successfully when they were originally written. I wonder what's broken in the meantime.... will try to investigate.
Comment 2 Jonathan Kew (:jfkthame) 2012-02-02 06:54:29 PST
Created attachment 593826 [details] [diff] [review]
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized

So this is actually caused by a problem in the moz_x*alloc wrappers: they trigger an OOM abort when the pointer returned by the underlying *alloc function is NULL, but in the case of a zero-sized allocation request, NULL is in fact a legitimate return value.

The particular case here occurs because during loading of the Burmese font, graphite tries to resize a block to 0 bytes, and on Windows and Linux (but not OS X, apparently) that results in a NULL return from realloc(). The same issue applies to the malloc and calloc wrappers, however.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-02 10:16:15 PST
Comment on attachment 593826 [details] [diff] [review]
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized

Nice fix.

Is it possible to add a crashtest that uses this font?
Comment 4 Jonathan Kew (:jfkthame) 2012-02-02 11:02:41 PST
The reftest we're intending to add in bug 700022 uses it, so it will in effect be serving as a crashtest as well - is that good enough, or do you want a separate copy labelled as a crashtest as well?
Comment 6 Ed Morley [:emorley] 2012-02-03 10:54:40 PST
https://hg.mozilla.org/mozilla-central/rev/d0110db9290d
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-01 20:23:59 PST
*** Bug 732316 has been marked as a duplicate of this bug. ***
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-01 20:28:18 PST
Bug 680556 exposed this bug to external XPCOM components, so we either need to back that out Gecko 11 or take this fix there.
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-01 20:30:06 PST
Comment on attachment 593826 [details] [diff] [review]
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized

[Approval Request Comment]
Regression caused by (bug #): 680556
User impact if declined: Addons may crash
Testing completed (on m-c, etc.): It's been on m-c for some time, and is very simple.
Risk to taking this patch (and alternatives if risky): Backing out Bug 680556
String changes made by this patch:

This is simple enough to take even at this point in the beta cycle, IMO, but we could always back out Bug 680556.
Comment 10 Alex Keybl [:akeybl] 2012-03-02 09:07:43 PST
Comment on attachment 593826 [details] [diff] [review]
patch, don't treat a NULL malloc result as OOM when the requested allocation is zero-sized

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> This is simple enough to take even at this point in the beta cycle, IMO, but
> we could always back out Bug 680556.

Bug 680556 is also causing the OOM crasher in bug 716594. Would it be possible to prepare a backout for Beta 11 instead?

Approving the patch for Aurora 12, but I'd be more comfortable taking a backout of bug 680556 at this point.
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-02 09:51:23 PST
http://hg.mozilla.org/releases/mozilla-aurora/rev/67ee19b77fe0
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-02 09:57:50 PST
http://hg.mozilla.org/releases/mozilla-beta/rev/b091fc697c22

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