1885462 - Crash in [@ mozilla::dom::Selection::SelectFrames]

Status: ASSIGNED

(Core :: DOM: Selection, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/d935db4c-8512-4e90-b9f8-9f19b0240314

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(aRange.IsPositioned())

Top 10 frames of crashing thread:

0  xul.dll  mozilla::dom::Selection::SelectFrames const  dom/base/Selection.cpp:1736
1  xul.dll  mozilla::dom::Selection::Clear  dom/base/Selection.cpp:1341
1  xul.dll  mozilla::dom::Selection::CollapseInternal  dom/base/Selection.cpp:2496
2  xul.dll  mozilla::dom::Selection::CollapseInLimiter  dom/base/Selection.cpp:2444
3  xul.dll  mozilla::dom::Selection::CollapseInLimiter  dom/base/Selection.h:221
3  xul.dll  mozilla::dom::Selection::CollapseInLimiter  dom/base/Selection.h:216
3  xul.dll  nsFrameSelection::TakeFocus  layout/generic/nsFrameSelection.cpp:1342
4  xul.dll  nsFrameSelection::HandleClick  layout/generic/nsFrameSelection.cpp:1170
5  xul.dll  nsIFrame::MoveCaretToEventPoint  layout/generic/nsIFrame.cpp:4873
6  xul.dll  nsIFrame::HandleEvent  layout/generic/nsIFrame.cpp:4407

Possible regression.

Comment 1

[Jan Jaeschke [:jjaschke]]

Masayuki, is the assert at this point the right thing to do? There's quite a few callers of SelectFrames(), which don't necessarily enforce the range to be positioned (e.g. the Clear() which is visible in the crash stack trace).

I could see this assert being meaningful if aSelect is true, but I would assume it'd be safer in general to just return early if the range is not positioned. Or is it rather a bug that there is a non-positioned range in Selection at all?

Comment 2

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

Yes, I think the assertion is reasonable. AbstractRange::mIsPositioned is set to true only when both boundaries and the range root are properly set to non-null and the given range should be previous selection range in the case. So, my question (and the root cause) is, how did the selection range become "not positioned"?

Note that I changed that the range will be removed from selections when the range becomes "not positioned" or is updated into different range root in 124. So the change is applied even in the beta channel. I'm not sure where is the remaining path causing this odd state.

Comment 3

[Mayank Bansal]

Bug 1890888 has a fuzz testcase that repros this crash

Comment 4

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

Thank you. However, it seems that it's caused by supporting selection across shadow DOM boundaries and it was landed recently. So, I guess that there is another cause. On the other hand, the testcase provides a good example how to create the odd situation.

Comment 5

[Mayank Bansal]

Bug 1896229 has a fuzz testcase that crashes with this crash signature.

Comment 6

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

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

• Top 10 content process crashes on beta

:avandolder, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Comment 7

[Adam Vandolder [:avandolder]]

Sean, should this crash also be fixed by the patch for bug 1890899?

Comment 8

[Frederik Braun [:freddy] (OOO until August 18th)]

Hey, this just happened for me on a recent Nightly. My build ID was 20240531093001, which I guesss hould be recent enough?

The crash is in MOZ_DIAGNOSTIC_ASSERT(aRange.IsPositioned()); from Selection.cpp#1771.

Comment 9

[Frederik Braun [:freddy] (OOO until August 18th)]

Happens somewhat reproducible on one of my local home network appliances. Not easy for me to build a test case but happy to re-test if someone needs this.

Comment 10

[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 11

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

I find a code looking odd to me.
https://searchfox.org/mozilla-central/rev/01aaa47e62a2015e7641f26ab0bc2bb00ab579b8/dom/base/Selection.cpp#704
When it's called, it does not clear the selection ranges, but unregister itself from the ranges. However, this path should not related to the stack in comment 0 because it's called during a press of the primary button of the pointing device...

Comment 12

[Sean Feng [:sefeng211]]

Attachment: Bug 1885462 - Add a new MOZ_DIAGNOSTIC_ASSERT_IF assert to Selection::SelectFrames

I wonder if this relates to shadow-crossing selection. I.e,
the default boundaries (nsRange.mStart, nsRange.mEnd) are collapsed,
but there's a CrossShadowBoundaryRange that makes this nsRange
still valid to many callers.

Comment 13

[Sean Feng [:sefeng211]]

The proposed patch just adds an assertion to see if we can isolate this to a shadow-crossing boundary selection issue.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/429c14f128c5
Add a new MOZ_DIAGNOSTIC_ASSERT_IF assert to Selection::SelectFrames r=masayuki

Comment 15

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/429c14f128c5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

A few new crash reports have come in since comment 15. All of them have been MOZ_DIAGNOSTIC_ASSERT(aRange.IsPositioned()) so far.
https://crash-stats.mozilla.org/report/index/a4a9866f-802a-409d-ba52-191e20240922

Comment 17

[Gabriele Svelto [:gsvelto]]

This is somewhat spiking on nightly, crash volume has doubled in recent days.

Comment 18

[Sean Feng [:sefeng211]]

Masayuki, I think this https://crash-stats.mozilla.org/report/index/22467122-bdcb-46c6-9646-500360241007 matches what you described in comment 11. Selection::Clear would do that right? Though I don't see how it can cause the crash.

thoughts?

Comment 19

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

Well, it occurs only when the cycle collector cleans Selection up. However, it seems that the crash is not after that.

AbstractRange::mIsPositioned is updated in nsRange::DoSetRange. When start and/or end boundary gets cleared, it's set to false. Then, it's removed from Selection, the assertion fails. However, I have no idea how to clear the boundaries...

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Have we seen any of these crashes in fuzzing land by chance, Jason?

Comment 21

[Jason Kratzer [:jkratzer]]

No, unfortunately not. I suspect it's because of the user interaction required based on the stack?

Comment 22

[Frederik Braun [:freddy] (OOO until August 18th)]

It just happened to me again when I was signing into a website using the password manager and clicking to submit a form. I am not sure but my selection was either the username in the field or actually something from the password managers injected popup. I am using the Firefox builtin password manager.

Hope this helps :)

Comment 23

[Mayank Bansal]

The fuzzing testcase in bug 1922153 crashes for me with this signature. Open the testcase and do a CTRL + A.
https://crash-stats.mozilla.org/report/index/7b4a3626-bf3e-43fc-88e6-b2b280250408#tab-bugzilla
https://crash-stats.mozilla.org/report/index/a8e3b900-994f-4279-99b0-1483c0250408#tab-bugzilla

Comment 24

[Sean Feng [:sefeng211]]

Thanks, I'll submit a patch to that bug, and needs an revisit to this bug once that one is fixed.