1935728 - Assertion failure: this != presContext->GetViewportScrollStylesOverrideElement() (Leaving behind a raw pointer to this element (as having propagated scrollbar styles) - that's dangerous...), at /builds/worker/checkouts/gecko/dom/base/Element.cpp:2363

Status: VERIFIED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20241129-ed73389dc144 (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework --upgrade
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

Assertion failure: this != presContext->GetViewportScrollStylesOverrideElement() (Leaving behind a raw pointer to this element (as having propagated scrollbar styles) - that's dangerous...), at /builds/worker/checkouts/gecko/dom/base/Element.cpp:2363

#0 0x774f0fdaa37e in mozilla::dom::Element::UnbindFromTree(mozilla::dom::UnbindContext&) /builds/worker/checkouts/gecko/dom/base/Element.cpp:2361:7
#1 0x774f11cc4d10 in nsGenericHTMLElement::UnbindFromTree(mozilla::dom::UnbindContext&) /builds/worker/checkouts/gecko/dom/html/nsGenericHTMLElement.cpp:563:20
#2 0x774f0fd0e054 in nsIContent::UnbindFromTree() /builds/worker/checkouts/gecko/dom/base/FragmentOrElement.cpp:157:3
#3 0x774f0fffadbb in nsINode::RemoveChildNode(nsIContent*, bool) /builds/worker/checkouts/gecko/dom/base/nsINode.cpp:2339:9
#4 0x774f0fff0818 in nsINode::RemoveChild(nsINode&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/nsINode.cpp:955:3
#5 0x774f1043e65c in mozilla::dom::Node_Binding::removeChild(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/./NodeBinding.cpp:1086:60
#6 0x774f110e98dd in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3290:13
#7 0x774f148ef8fa in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:532:13
#8 0x774f148ef0d3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:628:12
#9 0x774f14905c18 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:700:10
#10 0x774f14905c18 in js::Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3338:16
#11 0x774f148ee56a in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:502:13
#12 0x774f148eefbd in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:660:13
#13 0x774f148f0728 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:727:8
#14 0x774f149d70fb in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/CallAndConstruct.cpp:119:10
#15 0x774f10b6fa79 in mozilla::dom::IdleRequestCallback::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::IdleDeadline&, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/./WindowBinding.cpp:456:8
#16 0x774f0fc687d4 in mozilla::dom::IdleRequestCallback::Call(mozilla::dom::IdleDeadline&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/WindowBinding.h:390:12
#17 0x774f0fe33e76 in Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/WindowBinding.h:403:12
#18 0x774f0fe33e76 in mozilla::dom::IdleRequest::IdleRun(nsPIDOMWindowInner*, double, bool) /builds/worker/checkouts/gecko/dom/base/IdleRequest.cpp:57:13
#19 0x774f0fb4bed1 in nsGlobalWindowInner::RunIdleRequest(mozilla::dom::IdleRequest*, double, bool) /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:739:12
#20 0x774f0fb4aeb3 in nsGlobalWindowInner::ExecuteIdleRequest(mozilla::TimeStamp) /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:767:3
#21 0x774f0fb4ac50 in IdleRequestExecutor::Run() /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:608:13
#22 0x774f0de52ba7 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:688:16
#23 0x774f0de4908d in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:1015:20
#24 0x774f0de47e6e in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:880:15
#25 0x774f0de48185 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:624:36
#26 0x774f0de5b396 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:336:37
#27 0x774f0de5b396 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
#28 0x774f0de6ebd4 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1159:16
#29 0x774f0de7589f in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#30 0x774f0ea14997 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
#31 0x774f0e966dd1 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:362:3
#32 0x774f0e966dd1 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:344:3
#33 0x774f1378e9d8 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#34 0x774f13842018 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:469:33
#35 0x774f147402db in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:646:20
#36 0x774f0ea15844 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:235:9
#37 0x774f0e966dd1 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:362:3
#38 0x774f0e966dd1 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:344:3
#39 0x774f1473f70a in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:584:34
#40 0x5941257cd7de in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:397:22

Comment 1

[Andrew McCreight [:mccr8]]

This is in DOM code, but it feels a bit layout-y. Daniel, maybe you could figure out what we should do with this?

Comment 2

[Andrew McCreight [:mccr8]]

Does this do anything interesting in a (non-debug) ASan build? This code won't run then but maybe that will give us a more concrete idea of how "dangerous" it actually is. Thanks.

Comment 3

[Daniel Holbert [:dholbert]]

I'm on PTO today, but fwiw, doing searchfox poking from my phone, it looks like I was the one who added this assertion many years back, here:
https://hg.mozilla.org/mozilla-central/rev/276e3d5e9085476bf13f8c481133fb4d02dc3aa7#l1.12

I can take a look on Monday.

Comment 4

[Tyson Smith [:tsmith]]

(In reply to Andrew McCreight [:mccr8] from comment #2)

Does this do anything interesting in a (non-debug) ASan build?

No results from an ASan build.

Comment 5

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20241206214935-3254d5c12d08.
The bug appears to have been introduced in the following build range:

Start: 337d4c8f793e8f4ef5a8efbca1f9d02edb56b409 (20241126083942)
End: 0c2bb4ccb656d87e2cd8e2eaf6ab80bfcf4746bd (20241126124230)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=337d4c8f793e8f4ef5a8efbca1f9d02edb56b409&tochange=0c2bb4ccb656d87e2cd8e2eaf6ab80bfcf4746bd

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Comment 6

[Bugmon [:jkratzer for issues]]

A pernosco session for this bug can be found here.

Comment 7

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

Set release status flags based on info from the regressing bug 1931301

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Bugmon [:jkratzer for issues] from comment #6)

A pernosco session for this bug can be found here.

This pernosco session seems to be lacking sources; I emailed jkratzer to see if that's a known issue (don't want to start a side conversation here). That makes the pernosco session a bit trickier to debug but not impossible.

Stepping back a bit, we've got some notes on why this assertion is important (and why we expect it should hold) here:
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsPresContext.h#1268-1275

// This is a non-owning pointer. May be null. If non-null, it's guaranteed to
// be pointing to an element that's still alive, because we'll reset it in
// UpdateViewportScrollStylesOverride() as part of the cleanup code when
// this element is removed from the document. (For <body> and the root
// element, this call happens in nsCSSFrameConstructor::ContentRemoved(). For
// fullscreen elements, it happens in the fullscreen-specific cleanup invoked
// by Element::UnbindFromTree().)
mozilla::dom::Element* MOZ_NON_OWNING_REF mViewportScrollOverrideElement;

In this case, the element does seem to be the body element -- the innermost UnbindFromTree call (the one that makes the assertion) has this being of type HTMLBodyElement. The code-comment quoted above says that I should look at nsCSSFrameConstructor::ContentRemoved to see where mViewportScrollOverrideElement should have been cleared in this case... but it seems nsCSSFrameConstructor::ContentRemoved was recently removed (or renamed-and-morphed) in bug 1931301, the regressing bug that bugmon + RyanVM identified in comment 5 and comment 7. It was replaced with a function called nsCSSFrameConstructor::ContentWillBeRemoved.

Comment 9

[Daniel Holbert [:dholbert]]

So in that pernosco session, we get two important calls to nsCSSFrameConstructor::ContentWillBeRemoved, just before the assertion failure:

(1) We get a call to nsCSSFrameConstructor::ContentWillBeRemoved where aChild is our HTMLBodyElement. At this point nsPresContext::mViewportScrollOverrideElement is pointing to that body element, and we successfully null out that pointer in UpdateViewportScrollStylesOverride, because its call to GetPropagatedScrollStylesForViewport explicitly passes down the removed-child-pointer when calling GetBodyElement, and so GetBodyElement is smart enough to return null in that case:
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsPresContext.cpp#1608,1639-1642

static Element* GetPropagatedScrollStylesForViewport(
...
  Element* bodyElement = document->GetBodyElement(aRemovedChild);
  if (!bodyElement) {
    return nullptr;
  }

This is all good and it's doing the cleanup that the comment that I quoted in comment 8 was alluding to.

(2) Then we get a call to nsCSSFrameConstructor::ContentWillBeRemoved where aChild is an <html> element (in C++ it's of type mozilla::dom::HTMLSharedElement but its aChild->NodeInfo()->mLocalName->mData is the string "html"). This time, this ContentWillBeRemoved call has flags aFlags=REMOVE_FOR_RECONSTRUCTION (not REMOVE_CONTENT) which means removingElement ends up nullptr here:
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsCSSFrameConstructor.cpp#7334-7337

const Element* removingElement =
    aFlags == REMOVE_CONTENT ? aChild->AsElement() : nullptr;
Element* newOverrideElement =
    presContext->UpdateViewportScrollStylesOverride(removingElement);

So, inside of UpdateViewportScrollStylesOverride, we don't have any prohibitions on matched elements, and we restore mViewportScrollOverrideElement to be pointing at the body element that we're in the process of removing.

Comment 10

[Daniel Holbert [:dholbert]]

So call (2) in comment 9 is the relevant one. Prior to bug 1931301, we did not discover the body (and did not it as UpdateViewportScrollStylesOverride) there. I'm not sure if that's because bug 1931301 made us lazier about actually-removing the body, vs. if it's something to do with the removingElement / aRemovedChild params (which were added in bug 1931301).

emilio, could you take a look here as followup work for bug 1931301? It might be that things end up OK here (because probably we end up calling UpdateViewportScrollStylesOverride again after the body element etc. get rebuild), but it's pretty scary that we end up restoring a dangling raw pointer to a soon-to-be-deleted-element in the meantime.

We should adjust the mechanics from bug 1931301 to ensure that we can maintain the invariant that this element stays nulled out and doesn't get restored to an element that's on-its-way-to-being-destroyed. (And we should update the comment that I quoted in comment 8 to mention ContentWillBeRemoved instead of ContentRemoved and to possibly include any extra nuance that we have to add here to keep this working.)

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1935728 - Fix nsCSSFrameConstructor::ContentWillBeRemoved reentrancy guard. r=dholbert,#layout

If we end up reframing the root for other reasons during the
ContentWillBeRemoved call, then we didn't set
mReframingForViewportStyles = true, which prevented us from re-entering
the viewport style update.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01780493fe01
Fix nsCSSFrameConstructor::ContentWillBeRemoved reentrancy guard. r=dholbert

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/01780493fe01

Comment 14

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20241210093052-b16c09f16ef5.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 15

[Tyson Smith [:tsmith]]

sec-moderate was suggested by emilio on matrix.