1170045 - crash in OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xrealloc | mozilla::dom::DeferredFinalizerImpl<nsISupports>::AppendDeferredFinalizePointer(void*, void*)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mats Palmgren (inactive)]

This bug was filed from the Socorro interface and is 
report bp-3a2e1eff-3ead-42c5-b3b0-ab44b2150531.
=============================================================

Similar to bug 1162024 but it looks like a separate issue.
100% of the crashes are on Windows.
It appears to have started in v38.
Crashes for the past 28 days:
Product   Version       Percentage   Number Of Crashes
Firefox   38.0.1        55.71 %      1152
Firefox   38.0b99       13.69 %      283
Firefox   38.0.5b99     6.00 %       124
Firefox   38.0.5b2      5.17 %       107
Firefox   38.0.5b3      5.13 %       106
Firefox   38.0b9        3.67 %       76
Firefox   38.0          3.24 %       67
Firefox   38.0.5b1      2.71 %       56
Firefox   38.0b6        1.35 %       28
Firefox   38.0b8        1.11 %       23
Firefox   38.0b5        0.63 %       13
Firefox   38.0b2        0.43 %       9
Firefox   38.0b4        0.34 %       7
Firefox   38.0b1        0.34 %       7
Firefox   38.0b3        0.24 %       5
Firefox   41.0a1        0.14 %       3
Firefox   38.0b7        0.05 %       1
Firefox   40.0a1        0.05 %       1 


Stack:

mozalloc_abort(char const* const)
mozalloc_handle_oom(unsigned int)
moz_xrealloc
mozilla::dom::DeferredFinalizerImpl<nsISupports>::AppendDeferredFinalizePointer(void*, void*)
XPCWrappedNative::Release()
xul.dll@0x19fdb57
nsTArray_Impl<mozilla::DeferredFinalizeFunctionHolder, nsTArrayInfallibleAllocator>::Clear()
mozilla::dom::DeferredFinalizerImpl<nsISupports>::AppendDeferredFinalizePointer(void*, void*)
FinalizeArenas
@0x3bccdf
@0x8a4718f
js::gc::GCRuntime::endMarkingZoneGroup()
js::gc::GCRuntime::beginSweepPhase(bool)
js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason)
js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason)
js::gc::GCRuntime::gcSlice(JS::gcreason::Reason, __int64)
js::gc::CheckAllocatorState<1>
js::NewGCAccessorShape(js::ExclusiveContext*)
js::Shape::new_(js::ExclusiveContext*, js::StackShape&, unsigned int)
js::PropertyTree::getChild(js::ExclusiveContext*, js::Shape*, js::StackShape&)
js::NativeObject::getChildProperty(js::ExclusiveContext*, JS::Handle<js::NativeObject*>, JS::Handle<js::Shape*>, js::StackShape&)
...

Comment 1

[Mats Palmgren (inactive)]

Looking at the OOMAllocationSize values it appears to be legit OOM crashes.

Comment 2

[Andrew McCreight [:mccr8]]

Well, the old code also used an nsTArray, so I feel like this is probably mostly a signature change. But it shouldn't be too hard to change this code to a SegmentedVector, as all it does it push and pop.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 0 - modify TestSegmentedVectors to use explicitly-updated counts

The scheme in TestSegmentedVectors to use manually-annotated points and
magic numbers corresponding to those annotations works OK for small
numbers of operations.  But for testing bulk push and pop, we're going
to be doing many more operations, so let's move to recording explicitly
in code the operations we expect to see, and checking those instead.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add bulk pop support to SegmentedVector

Writing PopLastN in this way is probably a bit of a micro-optimization,
but at least it comes with tests and some comments for verifying the
goodness of the code.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 2 - use SegmentedVector in the DeferredFinalize implementation

This change ought to make the necessary allocations for many deferred
finalizations smaller, at the cost of some extra space usage and
slightly slower walking over the array.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8726806 [details] [diff] [review]
part 2 - use SegmentedVector in the DeferredFinalize implementation

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

Thanks for fixing this.

::: dom/bindings/BindingUtils.h
@@ +2897,1 @@
>      if (newLen == 0) {

Maybe change this test to if (oldLen == aSlice) and remove newLen?

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add bulk pop support to SegmentedVector

Here, let's submit a version where the comments aren't completely bogus.

Comment 8

[Eric Rahm [:erahm]]

Comment on attachment 8726841 [details] [diff] [review]
part 1 - add bulk pop support to SegmentedVector

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

::: mfbt/SegmentedVector.h
@@ +243,5 @@
> +      if (segmentLen > aNumElements) {
> +        break;
> +      }
> +
> +      // Destroying the segment destroys all elements contained therein.

Maybe I'm missing something, but don't you need to mSegments.popLast() here?

::: mfbt/tests/TestSegmentedVector.cpp
@@ +179,5 @@
> +      size_t length = v.Length();
> +      v.PopLastN(length);
> +      dtorCalls += length;
> +      MOZ_RELEASE_ASSERT(gNumDtors == dtorCalls);
> +    }

Lets add a test that actually crosses segment boundaries, ie:

v.Clear()
v.Append(x) * (segementSize + 1)
v.PopLastN(segmentSize)

Additionally can you add a test to |TestBasics| (or a separate test) that verifies the contents of the vector after calling |PopLastN|? Here we're just asserting that the right number of things were popped, but not necessarily the right things.

@@ +180,5 @@
> +      v.PopLastN(length);
> +      dtorCalls += length;
> +      MOZ_RELEASE_ASSERT(gNumDtors == dtorCalls);
> +    }
> +    

nit: trailing space

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add bulk pop support to SegmentedVector

Tests have been updated per review comments, and the PopLastN implementation
has been updated to pass said tests.  The extra tests were very helpful!

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8726896 [details] [diff] [review]
part 1 - add bulk pop support to SegmentedVector

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

::: mfbt/tests/TestSegmentedVector.cpp
@@ +232,5 @@
> +      dtorCalls += length;
> +      MOZ_RELEASE_ASSERT(v.IsEmpty());
> +      MOZ_RELEASE_ASSERT(gNumDtors == dtorCalls);
> +    }
> +    

nit: trailing space

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9609f981f97
https://hg.mozilla.org/integration/mozilla-inbound/rev/00d7f8217cc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfc843963851

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9609f981f97
https://hg.mozilla.org/mozilla-central/rev/00d7f8217cc2
https://hg.mozilla.org/mozilla-central/rev/dfc843963851

Comment 13

[[:philipp]]

hi, is the new signature from bug 1272795 showing up in 47 for the first time related to this bug?

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to philipp from comment #13)
> hi, is the new signature from bug 1272795 showing up in 47 for the first
> time related to this bug?

Probably, but we've had other crashes in this code before, so I think this is a signature change.