Closed Bug 1427118 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Graphics, defect, P2, critical)

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: philipp, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [gfx-noted])

Crash Data

Attachments

(1 file, 1 obsolete file)

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?
Component: Graphics: WebRender → Graphics
OS: Windows → All
Hardware: x86 → All
Whiteboard: [gfx-noted]
Anything that we can think of to clear these few crashes just based on the stacks?  A wallpaper solution?
Flags: needinfo?(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.
Flags: needinfo?(lsalzman)
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.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8951136 - Flags: review?(jmuizelaar)
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
Attachment #8951136 - Flags: feedback+
v2, using gfxCriticalNote and showing size information.
Attachment #8951136 - Attachment is obsolete: true
Attachment #8951136 - Flags: review?(jmuizelaar)
Attachment #8952033 - Flags: review?(milan)
Attachment #8952033 - Flags: review?(milan) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a35fe8a4a82d
make RecordedFontData use fallible allocations. r=milan
https://hg.mozilla.org/mozilla-central/rev/a35fe8a4a82d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
This looks like a reasonably low-risk crash fix and grafts cleanly to Beta. Should we consider nominating it for uplift?
Flags: needinfo?(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
Flags: needinfo?(lsalzman)
Attachment #8952033 - Flags: approval-mozilla-beta?
Comment on attachment 8952033 [details] [diff] [review]
make RecordedFontData use fallible allocations

Fix for possible OOMs during printing. Taking for 59b13.
Attachment #8952033 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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-.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.