Closed Bug 1496673 Opened Last year Closed Last year

Crash in mozilla::dom::CustomElementData::SizeOfIncludingThis

Categories

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

63 Branch
All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: philipp, Assigned: bzbarsky)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.
Assignee: nobody → jdai
Priority: -- → P2
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???)
While iterating the list in CustomElementReactionsStack::InvokeReactions we can have null pointers in mReactionQueue for reactions that have already been invoked.
Assignee: jdai → bzbarsky
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82e3da06b72f
Fix crash in custom element data memory reporting code.  r=jdai
https://hg.mozilla.org/mozilla-central/rev/82e3da06b72f
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Please request Beta approval on this when you get a chance.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Attachment #9014922 - Flags: approval-mozilla-beta?
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.
Attachment #9014922 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.