1951433 - Crash in [@ mozilla::dom::Document::GetServoRestyleRoot]

Status: NEW

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

Description

[BugBot [:suhaib / :marco/ :calixte]]

Crash report: https://crash-stats.mozilla.org/report/index/cad9be38-f908-4afb-b6cc-ab5e30240926

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsCOMPtr<nsINode>::get const  xpcom/base/nsCOMPtr.h:751
0  xul.dll  nsCOMPtr<nsINode>::operator nsINode* const  xpcom/base/nsCOMPtr.h:759
0  xul.dll  mozilla::dom::Document::GetServoRestyleRoot const  dom/base/Document.h:4030
0  xul.dll  mozilla::dom::NoteDirtyElement  dom/base/Element.cpp:5050
0  xul.dll  mozilla::dom::Element::NoteDirtyForServo  dom/base/Element.cpp:5191
0  xul.dll  nsCSSFrameConstructor::LazilyStyleNewChildRange  layout/base/nsCSSFrameConstructor.cpp:6287
0  xul.dll  nsCSSFrameConstructor::ContentAppended  layout/base/nsCSSFrameConstructor.cpp:6411
1  xul.dll  mozilla::PresShell::ContentAppended  layout/base/PresShell.cpp:4547
2  xul.dll  mozilla::dom::MutationObservers::NotifyContentAppended::<lambda_6>::operator const  dom/base/MutationObservers.cpp:166
2  xul.dll  Notify  dom/base/MutationObservers.cpp:99

By querying Nightly crashes reported within the last 2 months, here are some insights about the signature:

• First crash report: 2025-01-24
• Process type: Multiple distinct types
• Is startup crash: No
• Has user comments: No
• Is null crash: Yes - 2 out of 3 crashes happened on null or near null memory address

Comment 1

[Andrew McCreight [:mccr8]]

This looks like a very generic signature that could likely use some crash stats signature work to improve.

Comment 2

[Andrew McCreight [:mccr8]]

Looking at the last months at the crashes that are on null, there are a total of 9.

6 of them have a proto signature like the crash in comment 0, with dom::NoteDirtyElement:

  Document* doc = aElement->GetComposedDoc();
  nsINode* existingRoot = doc->GetServoRestyleRoot();

It looks like doc is missing a null check.

Another 4 have ClearServoData() on the stack (eg: bp-85c20be7-7941-41f3-b2f2-970c90250301), which looks like this:

ClearServoData() { ClearServoData(GetComposedDoc()); }

Maybe ClearServoData should add a null check.

Either way, the crash volume is rather low.

Emilio, should this kind of null deref crash in DOM support code for Servo live in DOM or CSS? I was thinking DOM but maybe CSS makes sense instead? Thanks.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So I think the null deref should be pretty impossible. Those codepaths should only deal with connected elements.

We could change those to OwnerDoc() without affecting correctness or speed I guess, maybe adding a MOZ_ASSERT(IsInComposedDoc()), but same, something seems busted before that if we get to that point without being connected to the document...