Closed Bug 1404633 Opened 7 years ago Closed 6 years ago

stylo: Crash in OOM | unknown | alloc::oom::oom | style::properties::StyleStructRef<T>::mutate<T>

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Version:
57 Branch
Platform:
All
Windows
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1415151
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- affected

People

(Reporter: philipp, Unassigned)

Details

(Keywords: crash, regression)

Crash Data

This bug was filed from the Socorro interface and is 
report bp-77956347-fec6-42aa-b9c9-e5eb40170930.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	alloc::oom::oom 	src/liballoc/oom.rs:26
1 	xul.dll 	style::properties::StyleStructRef<style::gecko_bindings::structs::root::mozilla::GeckoPosition>::mutate<style::gecko_bindings::structs::root::mozilla::GeckoPosition> 	toolkit/library/x86_64-pc-windows-msvc/release/build/style-7d743b4d7af5be49/out/properties.rs:126739
2 	xul.dll 	style::properties::longhands::box_sizing::cascade_property 	toolkit/library/x86_64-pc-windows-msvc/release/build/style-7d743b4d7af5be49/out/properties.rs:41954
3 	xul.dll 	style::properties::cascade 	toolkit/library/x86_64-pc-windows-msvc/release/build/style-7d743b4d7af5be49/out/properties.rs:142737
4 	xul.dll 	style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::cascade_style<style::gecko::wrapper::GeckoElement> 	servo/components/style/style_resolver.rs:586
5 	xul.dll 	style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::cascade_style_and_visited<style::gecko::wrapper::GeckoElement> 	servo/components/style/style_resolver.rs:309
6 	xul.dll 	style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::resolve_style<style::gecko::wrapper::GeckoElement> 	servo/components/style/style_resolver.rs:237
7 	xul.dll 	geckoservo::glue::Servo_ResolveStyleLazily 	servo/ports/geckolib/glue.rs:3158
8 	xul.dll 	mozilla::ServoStyleSet::ResolveStyleLazilyInternal(mozilla::dom::Element*, mozilla::CSSPseudoElementType, nsIAtom*, mozilla::ServoStyleContext const*, mozilla::StyleRuleInclusion, bool) 	layout/style/ServoStyleSet.cpp:1354
9 	xul.dll 	mozilla::ServoStyleSet::ResolveStyleLazily(mozilla::dom::Element*, mozilla::CSSPseudoElementType, nsIAtom*, mozilla::StyleRuleInclusion) 	layout/style/ServoStyleSet.cpp:625
10 	xul.dll 	nsComputedDOMStyle::DoGetStyleContextNoFlush(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType, nsComputedDOMStyle::AnimationFlag) 	layout/style/nsComputedDOMStyle.cpp:679
11 	xul.dll 	nsComputedDOMStyle::GetStyleContextNoFlush(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) 	layout/style/nsComputedDOMStyle.h:104
12 	xul.dll 	nsComputedDOMStyle::GetStyleContext(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) 	layout/style/nsComputedDOMStyle.cpp:419
13 	xul.dll 	nsComputedDOMStyle::UpdateCurrentStyleSources(bool) 	layout/style/nsComputedDOMStyle.cpp:925
14 	xul.dll 	nsComputedDOMStyle::GetPropertyCSSValue(nsTSubstring<char16_t> const&, mozilla::ErrorResult&) 	layout/style/nsComputedDOMStyle.cpp:1029
15 	xul.dll 	nsComputedDOMStyle::GetPropertyValue(nsTSubstring<char16_t> const&, nsTSubstring<char16_t>&) 	layout/style/nsComputedDOMStyle.cpp:383
16 	xul.dll 	mozilla::dom::CSSStyleDeclarationBinding::getPropertyValue 	dom/bindings/CSSStyleDeclarationBinding.cpp:172
17 		@0x2dbd71cba92

this crash signature is spiking up during 57.0b on 32bit and 64bit versions of firefox on windows.
These appear to be OOMs when allocating a style struct. Not sure if there's too much we can do here.
Priority: -- → P3
(In reply to [:philipp] from comment #0)
> this crash signature is spiking up during 57.0b on 32bit and 64bit versions
> of firefox on windows.

Bug 1399513 added the "OOM | ..." signature for this type of stylo crash on September 13, so I wonder if it's really spiking or just a new signature?

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> These appear to be OOMs when allocating a style struct. Not sure if there's
> too much we can do here.

It would be really helpful to know whether these are large or small OOMs (bug 1396899 comment 1) since this determines the next steps for the bug (we ignore small OOMs, and large OOMs should become fallible or be split into <=1M chunks).

In the meantime I propose that we make do with this stopgap approach: assume that reports where all of {virtual,physical,pagefile} > 300M are likely to be large-OOMs, and prioritize based on this volume.

Applying this proposed filter, about one-fifth of this crash volume is likely-large-OOMs: https://crash-stats.mozilla.com/search/?signature=%3DOOM%20%7C%20unknown%20%7C%20alloc%3A%3Aoom%3A%3Aoom%20%7C%20style%3A%3Aproperties%3A%3AStyleStructRef%3CT%3E%3A%3Amutate%3CT%3E&available_virtual_memory=%3E300000000&available_page_file=%3E300000000&available_physical_memory=%3E300000000&product=Firefox&date=%3E%3D2017-09-02T15%3A16%3A20.000Z&date=%3C2017-10-02T15%3A16%3A20.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> These appear to be OOMs when allocating a style struct. Not sure if there's
> too much we can do here.

Oh, maybe I missed something here. This style struct is a fixed size and it's a single one, not an array? If that's so, then assume small OOM and disregard comment 2, I suppose.
(In reply to David Major [:dmajor] from comment #3)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> > These appear to be OOMs when allocating a style struct. Not sure if there's
> > too much we can do here.
> 
> Oh, maybe I missed something here. This style struct is a fixed size and
> it's a single one, not an array?

Yeah that's right. So it seems like this would be OOM|Small, though it does odd that some reports have system memory usage < 90%. Unless that's normal for some reason?
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> (In reply to David Major [:dmajor] from comment #3)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> > > These appear to be OOMs when allocating a style struct. Not sure if there's
> > > too much we can do here.
> > 
> > Oh, maybe I missed something here. This style struct is a fixed size and
> > it's a single one, not an array?
> 
> Yeah that's right. So it seems like this would be OOM|Small, though it does
> odd that some reports have system memory usage < 90%. Unless that's normal
> for some reason?

An address space exhaustion is easily possible even when overall system memory use is low. I'm more worried about the fact that we still see a few of these when all three available-memory-types are at healthy amounts.
Is it really OOM?

It seems that stack in style::properties::StyleStructRef<T>::mutate<T> points to
> StyleStructRef::Vacated => panic!("Accessed vacated style struct")
rather than the line with actual style struct allocation.
Most likely need a minidump analysis to say for sure.
Available Page File 	7,307,264 bytes (6.97 MB)

I'm gonna say yes.
Is the implementation of `panic!` nontrivial? Could it be itself OOMing?
It can, as it needs to allocate for the string [1], but if the OOM is caused solely from the panic, we should have seen the panic itself in other crash reports.

But in general, panic should go via std::panicking::begin_panic, and it shouldn't show OOM in its signature...

[1] https://github.com/rust-lang/rust/blob/0ade339411587887bf01bcfa2e9ae4414c8900d4/src/libstd/panicking.rs#L511
I disassembled one of these functions and I think it's safe to say that this is a vanilla OOM of `UniqueArc::new((**v).clone())`:

xul!style::properties::StyleStructRef<style::gecko_bindings::structs::root::mozilla::GeckoColor>::mutate<style::gecko_bindings::structs::root::mozilla::GeckoColor>+0x1c [z:\build\build\src\obj-firefox\toolkit\library\x86_64-pc-windows-msvc\release\build\style-7d743b4d7af5be49\out\properties.rs @ 126732]:
...
126732 00007ffa`9d665475 e872fdae00      call    xul!HeapAlloc (00007ffa`9e1551ec)
126732 00007ffa`9d66547a 4889c7          mov     rdi,rax
126732 00007ffa`9d66547d 4885ff          test    rdi,rdi
126732 00007ffa`9d665480 0f8495000000    je      xul!style::properties::StyleStructRef<style::gecko_bindings::structs::root::mozilla::GeckoColor>::mutate<style::gecko_bindings::structs::root::mozilla::GeckoColor>+0xeb (00007ffa`9d66551b)  Branch

If HeapAlloc returns null, we branch to:

xul!style::properties::StyleStructRef<style::gecko_bindings::structs::root::mozilla::GeckoColor>::mutate<style::gecko_bindings::structs::root::mozilla::GeckoColor>+0xeb [z:\build\build\src\obj-firefox\toolkit\library\x86_64-pc-windows-msvc\release\build\style-7d743b4d7af5be49\out\properties.rs @ 126739]:
126739 00007ffa`9d66551b e820752400      call    xul!alloc::oom::oom (00007ffa`9d8aca40)

That the debug symbols mark the above as line 126739 is just misleading.
While I'm in this disassembly, the size parameter to HeapAlloc is 16 bytes:
126732 00007ffa`9d66546c 41b810000000    mov     r8d,10h

Which lines up with the fact that these reports have horrendously low amounts of memory available. I don't think there's any point to worrying about this call site. If we fix it, the OOM will immediately jump somewhere else. (But if the crash rate is unacceptably high, we may want to work on memory usage more generally.)
> Which lines up with the fact that these reports have horrendously low
> amounts of memory available. I don't think there's any point to worrying
> about this call site. 

I agree, but it would be nice if Socorro could process this signature as "OOM | small" to make this clearer.
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: fix-optional → wontfix
The crash signature for these OOMs may have changed with the update to rust 1.20.0. See bug 1415151 comment 7.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.