1496673 - Crash in mozilla::dom::CustomElementData::SizeOfIncludingThis

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-bda06159-5cd1-448e-9b33-4f3ff0181002.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::CustomElementData::SizeOfIncludingThis dom/base/CustomElementRegistry.cpp:269
1 xul.dll mozilla::dom::FragmentOrElement::nsExtendedDOMSlots::SizeOfExcludingThis dom/base/FragmentOrElement.cpp:853
2 xul.dll mozilla::dom::FragmentOrElement::AddSizeOfExcludingThis dom/base/FragmentOrElement.cpp:2374
3 xul.dll mozilla::dom::Element::AddSizeOfExcludingThis dom/base/Element.cpp:4385
4 xul.dll nsIDocument::AddSizeOfNodeTree dom/base/nsDocument.cpp:11645
5 xul.dll xpc::OrphanReporter::sizeOfIncludingThis js/xpconnect/src/XPCJSRuntime.cpp:2162
6 xul.dll static void StatsCellCallback<FineGrained> js/src/vm/MemoryMetrics.cpp:487
7 xul.dll static void IterateRealmsArenasCellsUnbarriered js/src/gc/PublicIterators.cpp:42
8 xul.dll js::IterateHeapUnbarriered js/src/gc/PublicIterators.cpp:58
9 xul.dll static bool CollectRuntimeStatsHelper js/src/vm/MemoryMetrics.cpp:767

=============================================================

this browser crash signature is newly showing up during the 63 beta cycle in low volume and looking related to the changes in bug 1486480.
most of the reports come from 32bit installations and for many of them the "System Memory Use Percentage" field indicates that the system is already under memory pressure.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like a null deref on:

    n += reaction->SizeOfIncludingThis(aMallocSizeOf);

which is inside this loop:

  for (auto& reaction : mReactionQueue) {

Presumably "reaction" is null.  

Based on the stack, looks like we're under CustomElementReactionsStack::InvokeReactions.  So we've done this bit:

        auto reaction(std::move(reactions.ElementAt(j)));

but are still in the  loop, so haven't cleared out mReactionQueue.  Looks like we just need to add a null-check to the memory reporting.

(On a side note, we are putting up a slow script dialog under custom element reactions here, which presumably means it's a slow-script custom element reaction in the chrome process???)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1496673. Fix crash in custom element data memory reporting code. r=jdai

While iterating the list in CustomElementReactionsStack::InvokeReactions we can have null pointers in mReactionQueue for reactions that have already been invoked.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82e3da06b72f
Fix crash in custom element data memory reporting code.  r=jdai

Comment 4

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/82e3da06b72f

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9014922 [details]
Bug 1496673.  Fix crash in custom element data memory reporting code.  r=jdai

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1486480

User impact if declined: Crashes.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just adds a null check to avoid a crash.

String changes made/needed: None.

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9014922 [details]
Bug 1496673.  Fix crash in custom element data memory reporting code.  r=jdai

Low risk patch fixing a crash, approved for our last 63 beta, thanks.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c79baa59f78f