Closed
Bug 1452467
Opened 6 years ago
Closed 6 years ago
[@ OOM | large | ...] crashes in mozilla::gfx::DWriteFontFileStream::DWriteFontFileStream
Categories
(Core :: Graphics, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: philipp, Assigned: aosmond)
Details
(Keywords: crash, Whiteboard: [gfx-noted])
Crash Data
Attachments
(1 file)
5.95 KB,
patch
|
lsalzman
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-7b912f82-abc2-4faa-bbc6-1c78a0180408. ============================================================= 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:72 3 xul.dll std::_Allocate vs2017_15.4.2/VC/include/xmemory0:78 4 xul.dll std::vector<unsigned char, std::allocator<unsigned char> >::_Resize<<lambda_1c24340e0c23a1361b49dbc3231bff51> > vs2017_15.4.2/VC/include/vector:1491 5 xul.dll mozilla::gfx::DWriteFontFileStream::DWriteFontFileStream gfx/2d/NativeFontResourceDWrite.cpp:174 6 xul.dll mozilla::gfx::NativeFontResourceDWrite::Create gfx/2d/NativeFontResourceDWrite.cpp:236 7 xul.dll mozilla::gfx::Factory::CreateNativeFontResource gfx/2d/Factory.cpp:599 8 xul.dll mozilla::gfx::RecordedFontData::PlayEvent gfx/2d/RecordedEventImpl.h:2776 9 xul.dll mozilla::layout::PrintTranslator::TranslateRecording layout/printing/PrintTranslator.cpp:60 ============================================================= these crashes on windows have been around for a while. it appears to predominantly affect builds with asian locales (75% zh-cn, 5% ja). the user comment to the report in question says: "Crashing while printing document with pdf", so perhaps url correlations could find some sites where this is reproducible.
Comment 1•6 years ago
|
||
Here are several URLs - there seem to be many sites where users are logged in so those aren't useful, and many about:printpreview: *https://tcpc.work/worker/plate_printing_processes/300108/completion_vote.html *http://codex-themes.com/thegem/pages/about-us/about-us/ *https://cdn.flashtalking.com/72777/IVAN_RL_LOWES_SPRING_BLACK_FRIDAY-300x600/index.html *https://www.google.fr/maps/@46.1495819,6.7247,14.58z
Assignee | ||
Comment 2•6 years ago
|
||
I'm not sure if there is much else to be done besides gracefully failing the alloc, unless there is some font specific trick to avoiding an OOM? They can be quite large, 20M+, in some reports.
Assignee: nobody → aosmond
Attachment #8967441 -
Flags: review?(lsalzman)
Assignee | ||
Updated•6 years ago
|
Priority: -- → P3
Whiteboard: [gfx-noted]
Updated•6 years ago
|
Attachment #8967441 -
Flags: review?(lsalzman) → review+
Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5336e51b77f Make native font resource memory allocations fallible. r=lsalzman
Backout by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/19e60113f96d Backed out changeset e5336e51b77f for Windows static analysis build bustage on a CLOSED TREE. r=aosmond
Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f19a98e3d4c Make native font resource memory allocations fallible. r=lsalzman
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6f19a98e3d4c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6f19a98e3d4c
Reporter | ||
Comment 8•6 years ago
|
||
could you request uplift to beta in case you deem fit to do so? thank you
Flags: needinfo?(aosmond)
Updated•6 years ago
|
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 8967441 [details] [diff] [review] 0001-Bug-1452467-Make-native-font-resource-memory-allocat.patch, v1 Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: May OOM crash when using very large fonts, e.g. asian locales. [Is this code covered by automated tests?]: Yes, for the success condition. I don't think we ever hit the OOM in testing however. [Has the fix been verified in Nightly?]: Yes, in so far that it hasn't caused any new problems. Rate of reproduction is too low on nightly. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: It is just a change from an vector to an nsTArray (for fallible allocations) and handling the alloc failure. The original methods could fail before, so minimal extra plumbing was required. [String changes made/needed]: None.
Flags: needinfo?(aosmond)
Attachment #8967441 -
Flags: approval-mozilla-beta?
Comment 10•6 years ago
|
||
Comment on attachment 8967441 [details] [diff] [review] 0001-Bug-1452467-Make-native-font-resource-memory-allocat.patch, v1 Fix for a long standing high volume crash, let's take this for beta 15.
Attachment #8967441 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 11•6 years ago
|
||
We should look back at crash-stats mid next week to verify the fix in beta.
Flags: qe-verify+
Reporter | ||
Comment 12•6 years ago
|
||
the signature itself is a fairly generic one and the crashes described here make up only a subset of it. i'd expect the volume of the signature to drop by half with a fix in beta & this query should stay without results: https://crash-stats.mozilla.com/search/?signature=%3DOOM%20%7C%20large%20%7C%20mozalloc_abort%20%7C%20mozalloc_handle_oom%20%7C%20moz_xmalloc%20%7C%20std%3A%3A_Allocate%20%7C%20std%3A%3Avector%3CT%3E%3A%3A_Resize%3CT%3E&proto_signature=~DWriteFontFileStream&build_id=%3E20180416175245&release_channel=beta
Comment 13•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c408410b77e1
Comment 14•6 years ago
|
||
I was not able to reproduce the crash with the steps provided. Per Mozilla Crash reports, the number of crashes is significantly lower in the last week, 60.0b15 with 13 crashes reported and 60.0b16 with 4 crashes: https://goo.gl/jGSjh9. Andrew, can you please take a look at the crash stats and let me know if I can close this bug?
Flags: needinfo?(aosmond)
Comment 15•6 years ago
|
||
According to the stats reports no crashes were reported on the latest builds: https://goo.gl/PNteZP Marking bug as Verified.
Updated•6 years ago
|
Updated•6 years ago
|
Flags: qe-verify+
Assignee | ||
Updated•4 years ago
|
Flags: needinfo?(aosmond)
You need to log in
before you can comment on or make changes to this bug.
Description
•