1989331 - Crash in [@ mozilla::RestyleManager::SnapshotFor]

Status: RESOLVED FIXED

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

Description

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

11 crashes from 11 installs of Firefox 144 betas which is more than for all older builds combined. Bug 769207 and bug 1987024 touched these files last. Could they be related?

Crash report: https://crash-stats.mozilla.org/report/index/31683dbe-ebd6-4db1-9ef5-a61010250918

MOZ_CRASH Reason:

MOZ_DIAGNOSTIC_ASSERT(!mInStyleRefresh)

Top 10 frames:

0  xul.dll  mozilla::RestyleManager::SnapshotFor(mozilla::dom::Element&)  layout/style/RestyleManager.cpp:3171
0  xul.dll  mozilla::RestyleManager::ElementStateChanged(mozilla::dom::Element*, mozilla:...  layout/style/RestyleManager.cpp:3470
1  xul.dll  mozilla::PresShell::ElementStateChanged(mozilla::dom::Document*, mozilla::dom...  layout/base/PresShell.cpp:4711
1  xul.dll  mozilla::dom::Document::ElementStateChanged(mozilla::dom::Element*, mozilla::...  dom/base/Document.cpp:8815
1  xul.dll  mozilla::dom::Element::NotifyStateChange(mozilla::dom::ElementState)  dom/base/Element.cpp:390
2  xul.dll  mozilla::dom::Element::State() const  dom/base/Element.h:312
2  xul.dll  mozilla::dom::HTMLTextAreaElement::IsValueEmpty() const  dom/html/HTMLTextAreaElement.h:374
2  xul.dll  mozilla::dom::HTMLTextAreaElement::UpdatePlaceholderShownState()  dom/html/HTMLTextAreaElement.cpp:1076
2  xul.dll  mozilla::dom::HTMLTextAreaElement::OnValueChanged(mozilla::TextControlElement...  dom/html/HTMLTextAreaElement.cpp:1089
3  xul.dll  mozilla::TextControlElement::OnValueChanged(mozilla::TextControlElement::Valu...  dom/html/TextControlElement.h:210

Comment 1

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

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on beta

For more information, please visit BugBot documentation.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I feel it's odd if this is caused by bug 769207 or bug 1987024. Bug 769207 removed the paths disabled by the pref. And bug 1987024 just changed the structure of some classes.

The problem is here:

mozilla::dom::HTMLTextAreaElement::UpdatePlaceholderShownState()
mozilla::dom::HTMLTextAreaElement::OnValueChanged(mozilla::TextControlElement::ValueChangeKind, bool, nsTSubstring<char16_t> const*)
mozilla::TextControlElement::OnValueChanged(mozilla::TextControlElement::ValueChangeKind, nsTSubstring<char16_t> const&)
mozilla::TextControlState::SetValue(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const*, mozilla::EnumSet<mozilla::TextControlState::ValueSetterOption, unsigned int> const&)
mozilla::TextControlState::SetValue(nsTSubstring<char16_t> const&, mozilla::EnumSet<mozilla::TextControlState::ValueSetterOption, unsigned int> const&)
mozilla::TextControlState::UnbindFromFrame(nsTextControlFrame*)

While destroying the text control frame, HTMLTextAreaElement tries to handle placeholder shown state...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, wait, this is impossible:

mozilla::dom::Element::NotifyStateChange(mozilla::dom::ElementState)	dom/base/Element.cpp:390
mozilla::dom::Element::State() const	dom/base/Element.h:312 

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(s2 since this is a topcrasher)

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

in this case, the web app has an input event listener which is kicked by a composition string change of IME and which retrieves a style value. Then, here requires a flush for some properties. Then, the frame of <input> is reframed and TextControlState::UnbindFromFrame needs to set the value to the latest one. Only the new value or the last value is empty and the other is not empty and needs to reset the empty value state to show/hide the placeholder text. So, to reproduce this bug, we need to change a style and retrieve a style value to flush the layout after setting the value but it's not yet applied to the element.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

To reproduce this bug, we need to make the situation that the element thinks the value is different state from the actual value when input event is fired.

The state is set only by OnValueChanged() overrides. They are called when the value is set or the value is modified by the editor before input event. So, basically, before the input event is fired, the element should have the latest empty value state. However, if the TextEditor has already been destroyed or the text control frame has already or is destroyed, TextEditor fails updating the state in the element.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And another necessary condition is, the text control frame should be dirty at dispatching the input event. After dispatching beforeinput nor textInput event, it seems that TextEditor does not flush the layout. So, I guessed that beforeinput or textInput listener can make the frame dirty. Then, TextEditor fails to update the state before dispatching input event. However, I cannot reproduce this bug with this simplest testcase. So, I'm still missing something...

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, I realized that why this is not reported from the nightly testers?? Some prefs enabled only in the nightly channel prevent this crash? Or has this bug already been fixed in the nightly channel?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

The crashes appear to have stopped with 144.0b7. Could have been the switch to late Beta. Otherwise, here's the pushlog for that:
https://hg-edge.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX-ANDROID_144_0b6_RELEASE&tochange=FIREFOX-ANDROID_144_0b7_RELEASE&branch=default

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The assertion is MOZ_DIAGNOSTIC_ASSERT, so I think switching to the late beta causes disabling the assertion.

On the other hand, there is no crash reports from the nightly channel...

Comment 11

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

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit BugBot documentation.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It seems that nobody reproduces this bug since 146. So, this must have already been fixed by another bug.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Still seeing these crashes on Beta 147.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks, Ryan.

Still just a guess, it could be caused by placeholder attribute or type of the <input> is changed.

void HTMLInputElement::UpdatePlaceholderShownState() {
  SetStates(ElementState::PLACEHOLDER_SHOWN,
            IsValueEmpty() && PlaceholderApplies() &&
                HasAttr(nsGkAtoms::placeholder));
}

This may be called by TextControlState::UnbindFromFrame. However, the value should not be changed because the new value is being set to the current value, but the storage is changed from TextEditor with the anonymous content to the TextControlState. Then, the above method is called but Element::SetStates may set the different state even if the value is not changed. That is, the result of PlaceholderApplies() has been changed by a change of type of <input> or HasAttr(nsGkAtoms::placeholder) has been changed.

Another possible case is, the value change has not been notified correctly in TextInputListener::OnEditActionHandled due to no frame. However, I guess this is less possible because the crash is caused by during a frame destruction.

Emilio, cannot set the placeholder state always? Does it affect to :placeholder pseudo element?

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Not sure what the question is? This is the web exposed :placeholder-shown pseudo class, so we'd have to check e.g. if it matches on other browsers on something like a checkbox, but my guess is not.

Both type and placeholder attribute changes should end up in UpdatePlaceholderShownState(), too...

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the unclear question, oh, I wrote odd things... However, thank you, that's clear for me.

Well, my current unsure thing is, why/how does this value is being changed during the TextControlState::UnbindFromFrame call... I need to read related code more, typically, whether a type change and a placeholder value change are handled synchronously. If they are, the TextEditor or the TextControlState missed to notify changing the value before the reframing.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I confirmed that input[type] and placeholder attribute changes are handled synchronously. So, looks like that TextEditor fails to notify the text control element of the value change so that the Element::SetStates updates the placeholder state during the call of TextControlState::UnbindFromFrame.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Some crash reports are set the URL to https://www.thaievisa.go.th/signup. However, there are some <input> elements in the page and I've not reproduced this bug on Windows with IME in the ASCII character composition mode.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The page basically, listen to input events and setting value to delete invalid characters for the field. And also listen to keydown but does not listen to composition events. I've not found the reframing cause.

The reduced stack of the report:

mozilla::RestyleManager::SnapshotFor(mozilla::dom::Element&) 	layout/style/RestyleManager.cpp:3158
mozilla::dom::HTMLInputElement::IsValueEmpty() const 	dom/html/HTMLInputElement.h:884
mozilla::dom::HTMLInputElement::UpdatePlaceholderShownState()	dom/html/HTMLInputElement.cpp:7102
mozilla::dom::HTMLInputElement::OnValueChanged(mozilla::TextControlElement::ValueChangeKind, bool, nsTSubstring<char16_t> const*)	dom/html/HTMLInputElement.cpp:7122
mozilla::TextControlElement::OnValueChanged(mozilla::TextControlElement::ValueChangeKind, nsTSubstring<char16_t> const&)	dom/html/TextControlElement.h:210
mozilla::TextControlState::SetValue(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const*, mozilla::EnumSet<mozilla::TextControlState::ValueSetterOption, unsigned int> const&)	dom/html/TextControlState.cpp:2670
mozilla::TextControlState::SetValue(nsTSubstring<char16_t> const&, mozilla::EnumSet<mozilla::TextControlState::ValueSetterOption, unsigned int> const&)	dom/html/TextControlState.h:291
mozilla::TextControlState::UnbindFromFrame(nsTextControlFrame*)	dom/html/TextControlState.cpp:2440 
mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)	layout/base/PresShell.cpp:4473
mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush)	layout/base/PresShell.h:1526
mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)	dom/base/Document.cpp:11678
mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType)	dom/base/Document.cpp:11610
nsIContent::GetPrimaryFrame(mozilla::FlushType)	dom/base/Element.cpp:275
mozilla::dom::Element::GetBoundingClientRect()	dom/base/Element.cpp:1131
mozilla::dom::Element_Binding::getBoundingClientRect(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)	dom/bindings/ElementBinding.cpp:4077 
mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)	layout/base/PresShell.cpp:4473
mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush)	layout/base/PresShell.h:1526
mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)	dom/base/Document.cpp:11678
mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType)	dom/base/Document.cpp:11610
nsIContent::GetPrimaryFrame(mozilla::FlushType)	dom/base/Element.cpp:275
mozilla::dom::Element::GetBoundingClientRect()	dom/base/Element.cpp:1131
mozilla::dom::Element_Binding::getBoundingClientRect(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)	dom/bindings/ElementBinding.cpp:4077
mozilla::MustConsumeMicroTask::RunAndConsumeJSMicroTask(JSContext*)	xpcom/base/CycleCollectedJSContext.h:252
mozilla::RunMicroTask(JSContext*, JS::MutableHandle<mozilla::MustConsumeMicroTask>)	xpcom/base/CycleCollectedJSContext.cpp:1102
mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool)	xpcom/base/CycleCollectedJSContext.cpp:1252
mozilla::CycleCollectedJSContext::LeaveMicroTask()	xpcom/base/CycleCollectedJSContext.h:445
mozilla::nsAutoMicroTask::~nsAutoMicroTask()	xpcom/base/CycleCollectedJSContext.h:636
mozilla::EventListenerManager::HandleEventSingleListener(mozilla::EventListenerManager::Listener*, nsAtom*, mozilla::WidgetEvent*, mozilla::dom::Event*, mozilla::dom::EventTarget*, bool)	dom/events/EventListenerManager.cpp:1275
mozilla::EventListenerManager::HandleEventWithListenerArray(mozilla::EventListenerManager::ListenerArray*, nsAtom*, mozilla::EventMessage, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, bool)	dom/events/EventListenerManager.cpp:1579
mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool)	dom/events/EventListenerManager.cpp:1484
mozilla::EventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool)	dom/events/EventListenerManager.h:465
mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&)	dom/events/EventDispatcher.cpp:364
mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)	dom/events/EventDispatcher.cpp:605
mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)	dom/events/EventDispatcher.cpp:686
mozilla::EventDispatcher::Dispatch(mozilla::dom::EventTarget*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)	dom/events/EventDispatcher.cpp:1260
mozilla::PresShell::EventHandler::DispatchEventToDOM(mozilla::WidgetEvent*, nsEventStatus*, nsPresShellEventCB*)	layout/base/PresShell.cpp:9635
mozilla::PresShell::EventHandler::DispatchEvent(mozilla::EventStateManager*, mozilla::WidgetEvent*, bool, nsEventStatus*, nsIContent*)	layout/base/PresShell.cpp:9170
mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*)	layout/base/PresShell.cpp:9063
mozilla::PresShell::EventHandler::HandleEventAtFocusedContent(mozilla::WidgetGUIEvent*, nsEventStatus*)	layout/base/PresShell.cpp:8812
mozilla::PresShell::EventHandler::HandleEvent(AutoWeakFrame&, mozilla::WidgetGUIEvent*, bool, nsEventStatus*)	layout/base/PresShell.cpp:7509
mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*)	layout/base/PresShell.cpp:7304
mozilla::PresShellWidgetListener::HandleEvent(mozilla::WidgetGUIEvent*)	layout/base/PresShellWidgetListener.cpp:246
mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&)	gfx/layers/apz/util/APZCCallbackHelper.cpp:537
mozilla::dom::BrowserChild::DispatchWidgetEventViaAPZ(mozilla::WidgetGUIEvent&)	dom/ipc/BrowserChild.cpp:1845
mozilla::dom::BrowserChild::RecvRealKeyEvent(mozilla::WidgetKeyboardEvent const&, nsID const&)	dom/ipc/BrowserChild.cpp:2448 

Comment 20

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

Since the crash volume is low (less than 15 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit BugBot documentation.

Comment 21

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

Is it known why the crash volume increases ~5x?

Comment 22

[Emilio Cobos Álvarez [:emilio]]

We've landed some editor changes recently for <input> and <textarea> (bug 2016280, bug 2020358, bug 2020681). Do any of those correlate with the recent spike?

But anyways, I think we can fix this properly now after those changes
.

Comment 23

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1989331 - Simplify editor teardown and init. r=masayuki,#dom-core

There's no need to go through SetValue() etc, the value isn't supposed
to change, we're just changing where it's stored. Handle this the same
way as we handle the selection state.

Comment 24

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

We've landed some editor changes recently for <input> and <textarea> (bug 2016280, bug 2020358, bug 2020681). Do any of those correlate with the recent spike?

The spike started with the new beta cycle which started last week. Nightly has one report from the last six months. Could the revert of bug 2016280 and related bugs need more changes? https://github.com/mozilla-firefox/firefox/commit/db672194f9d23a90e10cec5162a8c21147059417

Comment 25

[Emilio Cobos Álvarez [:emilio]]

Ah, no, then it's just because the assert is disabled on late beta (it's a DIAGNOSTIC_ASSERT), so it's normal it spikes when switching from late to early beta

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c6efbfab0d92
https://hg.mozilla.org/integration/autoland/rev/68ace92d6cbc
Simplify editor teardown and init. r=masayuki,dom-core

Comment 27

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

The bug is marked as tracked for firefox149 (beta). However, the bug still has low priority and has low severity.

:hsinyi, could you please increase the priority and increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 28

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/68ace92d6cbc

Comment 29

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox149 to wontfix.

For more information, please visit BugBot documentation.

Comment 30

[Emilio Cobos Álvarez [:emilio]]

Depends on a bunch of other stuff, early beta only.