Closed Bug 1170045 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: XPCOM, defect)

38 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: froydnj)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files, 2 obsolete files)

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&)
...
Looking at the OOMAllocationSize values it appears to be legit OOM crashes.
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.
Crash Signature: [@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xrealloc | mozilla::dom::DeferredFinalizerImpl<nsISupports>::AppendDeferredFinalizePointer(void*, void*)] → [@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xrealloc | mozilla::dom::DeferredFinalizerImpl<nsISupports>::AppendDeferredFinalizePointer(void*, void*)] [@ OOM | large | mozalloc_abort | mozalloc_handle_oom |…
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.
Attachment #8726804 - Flags: review?(erahm)
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.
Attachment #8726805 - Flags: review?(erahm)
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.
Attachment #8726806 - Flags: review?(continuation)
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?
Attachment #8726806 - Flags: review?(continuation) → review+
Here, let's submit a version where the comments aren't completely bogus.
Attachment #8726841 - Flags: review?(erahm)
Attachment #8726805 - Attachment is obsolete: true
Attachment #8726805 - Flags: review?(erahm)
Attachment #8726804 - Flags: review?(erahm) → review+
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
Attachment #8726841 - Flags: review?(erahm) → feedback+
Tests have been updated per review comments, and the PopLastN implementation
has been updated to pass said tests.  The extra tests were very helpful!
Attachment #8726896 - Flags: review?(erahm)
Attachment #8726841 - Attachment is obsolete: true
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
Attachment #8726896 - Flags: review?(erahm) → review+
Assignee: nobody → nfroyd
hi, is the new signature from bug 1272795 showing up in 47 for the first time related to this bug?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
You need to log in before you can comment on or make changes to this bug.