1427118 - Crash in OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | mozilla::gfx::RecordedFontData::RecordedFontData<T>

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-01943250-25b0-4f59-9d4a-7cf120171130.
=============================================================

Top 10 frames of crashing thread:

0 mozglue.dll mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33
1 mozglue.dll mozalloc_handle_oom memory/mozalloc/mozalloc_oom.cpp:54
2 mozglue.dll moz_xmalloc memory/mozalloc/mozalloc.cpp:86
3 xul.dll mozilla::gfx::RecordedFontData::RecordedFontData<std::basic_istream<char, std::char_traits<char> > > gfx/2d/RecordedEventImpl.h:2747
4 xul.dll mozilla::gfx::RecordedEvent::LoadEvent<std::basic_istream<char, std::char_traits<char> > > gfx/2d/RecordedEventImpl.h:3172
5 xul.dll mozilla::layout::PrintTranslator::TranslateRecording layout/printing/PrintTranslator.cpp:51
6 xul.dll mozilla::layout::RemotePrintJobParent::PrintPage layout/printing/ipc/RemotePrintJobParent.cpp:128
7 xul.dll mozilla::layout::RemotePrintJobParent::RecvProcessPage layout/printing/ipc/RemotePrintJobParent.cpp:88
8 xul.dll mozilla::layout::PRemotePrintJobParent::OnMessageReceived ipc/ipdl/PRemotePrintJobParent.cpp:245
9 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:3180

=============================================================

crashes with this signature on windows are regressing since firefox 56 - over 70% come from users with chinese locale builds. perhaps related to bug 1374900?

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Anything that we can think of to clear these few crashes just based on the stacks?  A wallpaper solution?

Comment 2

[Lee Salzman [:lsalzman]]

(In reply to Milan Sreckovic [:milan] from comment #1)
> Anything that we can think of to clear these few crashes just based on the
> stacks?  A wallpaper solution?

This signature started with bug 1374900 in 56. But I don't think bug 1374900 caused a new problem here that wasn't existing before in another form.

The offending line of code (with the infallible allocation) existed before that bug.

Before that, I think we were for one reason or another just seeming to hit the OOM here instead https://bugzilla.mozilla.org/show_bug.cgi?id=1176837 like in the crash report https://crash-stats.mozilla.com/report/index/2d75102b-97f3-4d71-9a04-b72140180124

What exactly shifted the balance I am not sure. Given the restructuring of the event playback to support more fallibility in general, maybe we could play with making that allocation fallible. But it just might run afoul of some other assert downwind if event playback fails. I am not entirely sure yet.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: make RecordedFontData use fallible allocations

This is just an adaptation of the same fix we used for bug 1347632. Nothing imaginative going on here. Marks the allocations as fallible and ensures that the failure boolean is propagated up, while spitting out some warnings.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8951136 [details] [diff] [review]
make RecordedFontData use fallible allocations

Review of attachment 8951136 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/RecordedEventImpl.h
@@ +2805,5 @@
>  RecordedFontData::SetFontData(const uint8_t *aData, uint32_t aSize, uint32_t aIndex)
>  {
> +  mData = new (fallible) uint8_t[aSize];
> +  if (!mData) {
> +    gfxWarning() << "RecordedFontData failed to allocate data for recording";

Maybe << aSize with this warning so that we see the value?  Also, it could be useful to make this a gfxCriticalNote so that we see the message in crash reports (when we crash elsewhere)?

@@ +2839,5 @@
>    ReadElement(aStream, mFontDetails.fontDataKey);
>    ReadElement(aStream, mFontDetails.size);
> +  mData = new (fallible) uint8_t[mFontDetails.size];
> +  if (!mData) {
> +    gfxWarning() << "RecordedFontData failed to allocate data for playback";

Same note as above, show the size we're trying to allocate and maybe make it gfxCriticalNote

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: make RecordedFontData use fallible allocations

v2, using gfxCriticalNote and showing size information.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a35fe8a4a82d
make RecordedFontData use fallible allocations. r=milan

Comment 7

[Andrei Ciure[:aciure]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This looks like a reasonably low-risk crash fix and grafts cleanly to Beta. Should we consider nominating it for uplift?

Comment 9

[Lee Salzman [:lsalzman]]

Comment on attachment 8952033 [details] [diff] [review]
make RecordedFontData use fallible allocations

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1374900
[User impact if declined]: OOM crashes when printing or WebRender.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Replaces crash with a warning. Since this crash occurred under OOM conditions, this may just kick the can down the road to some other OOM condition outside of this code, though.
[String changes made/needed]: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8952033 [details] [diff] [review]
make RecordedFontData use fallible allocations

Fix for possible OOMs during printing. Taking for 59b13.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f11e93bf7bfc

Comment 12

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Lee Salzman [:lsalzman] from comment #9)
> [Is this code covered by automated tests?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no 

Since it's already covered by automation, marking as qe-verify-.