723472 - reftest graphite-03a causes spurious "out of memory" crash due to zero-sized allocation request

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[John Daggett (:jtd)]

graphite-03a stacktrace on winxp:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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]]

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]]

Attachment: 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]

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]]

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 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0110db9290d

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d0110db9290d

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

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]]

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] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/67ee19b77fe0

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-beta/rev/b091fc697c22