1806408 - Crash in [@ nsTreeBodyFrame::SetFocused]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

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

156 crashes from 114 installations of Firefox 108.0 and 108.0.1

Emilio: Any idea what changed? There hadd been isolated crash reports for previous versions.

Crash report: https://crash-stats.mozilla.org/report/index/c1c32539-034c-4502-8ee0-167620221219

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsTreeBodyFrame::SetFocused  layout/xul/tree/nsTreeBodyFrame.cpp:447
1  xul.dll  mozilla::dom::XULTreeElement_Binding::set_focused  dom/bindings/XULTreeElementBinding.cpp:338
2  xul.dll  mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>  dom/bindings/BindingUtils.cpp:3235
3  xul.dll  CallJSNative  js/src/vm/Interpreter.cpp:459
3  xul.dll  js::InternalCallOrConstruct  js/src/vm/Interpreter.cpp:547
3  xul.dll  InternalCall  js/src/vm/Interpreter.cpp:614
3  xul.dll  js::Call  js/src/vm/Interpreter.cpp:646
3  xul.dll  js::CallSetter  js/src/vm/Interpreter.cpp:790
4  xul.dll  SetExistingProperty  js/src/vm/NativeObject.cpp:2549
4  xul.dll  js::NativeSetProperty<1>  js/src/vm/NativeObject.cpp:2625

Comment 1

[Emilio Cobos Álvarez (:emilio)]

User comments seem helpful here:

Tried to clear history -regular blink was grayed out, he highlighted all history in Manage History when Firefox crashed.

when i want to delete history using the 'DEL' key mozilla crashes

etc. So I assume this is in the history window which is where there's a XUL tree.

However I don't quite understand how this line can crash on its own: https://hg.mozilla.org/releases/mozilla-release/annotate/eebe28618896b353a76ac9c220fd4b96334cd73d/layout/xul/tree/nsTreeBodyFrame.cpp#l447

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I couldn't repro either on release or nightly, on Linux...

opend-up hitory. selected all and hit delete. The browser crashaed like a house of cards.

I can't repro any of this, and we haven't touched trees nor that window in particular (that still uses legacy xul layout), so hard to say which change is responsible...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Olli / Nika, do you by any chance see anything suspicious in the line linked in comment 1?

GetSelection can run script, in theory, and destroy this (that really sucks). But IIUC that should be calling this which seems fairly harmless... Maybe some microtask or something could run in between and mess up the state?

Comment 4

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

Microtasks shouldn't run there, unless something triggers something which either spins event loop or calls to webidl callbacks. GC/CC could trigger some notification elsewhere which runs those or runs something else.
At least mView should assigned to a local variable.

We've had some similar looking crashes quite some time
https://crash-stats.mozilla.org/report/index/bc4b710b-92f0-45d5-a2a0-00a990221117
The relevant code is old. From 2001, bug 71139?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1806408 - Try to make nsTreeBodyFrame code more robust. r=smaug

Not a complete fix (there are some callers from reflow which are not
particularly great), but this should make the code more robust.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f56c9bfc8df2
Try to make nsTreeBodyFrame code more robust. r=smaug

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Interesting that these crashes only appear to have started after v108 went to late beta. Suggests at least that we might not be able to see any effect from this patch until 109.0b7 ships next week (assuming we uplift in the mean time).

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Seems like :smaug and :emilio have already figured it out, so clearing the ni?

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f56c9bfc8df2

Comment 10

[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.
• If no, please set status-firefox109 to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9308962 [details]
Bug 1806408 - Try to make nsTreeBodyFrame code more robust. r=smaug

Beta/Release Uplift Approval Request

• User impact if declined: Tentative fix for high-volume crash.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Relatively simple fix to ancient code.
• String changes made/needed: none
• Is Android affected?: No

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9308962 [details]
Bug 1806408 - Try to make nsTreeBodyFrame code more robust. r=smaug

Approved for 109.0b5. As noted in comment 7, however, we may not be able to see if this patch is helping until after b7 ships next week.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/95fcce0e09e9

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9308962 [details]
Bug 1806408 - Try to make nsTreeBodyFrame code more robust. r=smaug

Beta/Release Uplift Approval Request

• User impact if declined: Low risk tentative crash fix.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): Rather trivial patch to avoid a set of crashes in ancient code.
• String changes made/needed: none
• Is Android affected?: No

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9308962 [details]
Bug 1806408 - Try to make nsTreeBodyFrame code more robust. r=smaug

Approved for 108.0.2

Comment 16

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-release/rev/c3ca1d6a5519