Closed Bug 1727840 Opened 5 months ago Closed 4 months ago

Crash in [@ mozilla::DeleteNodeTransaction::DeleteNodeTransaction]

Categories

(Core :: DOM: Editor, defect, P3)

defect

Tracking

()

RESOLVED FIXED
94 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox91 --- unaffected
firefox92 --- unaffected
firefox93 --- wontfix
firefox94 --- fixed

People

(Reporter: mccr8, Assigned: masayuki)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

Crash report: https://crash-stats.mozilla.org/report/index/da7e2adc-ac8a-4c45-9532-9fbc40210826

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(HTMLEditUtils::IsRemovableNode(aContentToDelete))

Top 10 frames of crashing thread:

0 XUL mozilla::DeleteNodeTransaction::DeleteNodeTransaction editor/libeditor/DeleteNodeTransaction.cpp:36
1 XUL mozilla::DeleteNodeTransaction::MaybeCreate editor/libeditor/DeleteNodeTransaction.cpp:24
2 XUL mozilla::EditorBase::DeleteNodeWithTransaction editor/libeditor/EditorBase.cpp:2212
3 XUL mozilla::EditorBase::InsertTextIntoTextNodeWithTransaction editor/libeditor/EditorBase.cpp:3077
4 XUL mozilla::EditorBase::InsertTextWithTransaction editor/libeditor/EditorBase.cpp:2914
5 XUL mozilla::HTMLEditor::HandleInsertText editor/libeditor/HTMLEditSubActionHandler.cpp:1072
6 XUL mozilla::EditorBase::InsertTextAsSubAction editor/libeditor/EditorBase.cpp:6007
7 XUL mozilla::EditorBase::OnCompositionChange editor/libeditor/EditorBase.cpp:3673
8 XUL mozilla::EditorEventListener::HandleChangeComposition editor/libeditor/EditorEventListener.cpp:1068
9 XUL mozilla::EventListenerManager::HandleEventInternal dom/events/EventListenerManager.cpp:1312

5 crashes from a single installation. It looks like this diagnostic assert was added in bug 1727185.

Severity: -- → S2

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

Masayuki, does this newly added assert crash ring any bell?

Flags: needinfo?(masayuki)

Yeah, this detects a bug of HTMLEditor here:
https://searchfox.org/mozilla-central/rev/5f81c5091d442d829120e19477ba869ae5219709/editor/libeditor/EditorBase.cpp#3071-3075

The stack means that composition string of IME becomes empty, and the text node is being deleted because of unnecessary, however, the node is already not editable. I'm not sure what's going on in the web apps because it's really tricky case that disabling editable state of composing element...

Can I know the URL of crashed web apps (via email or something)?

Flags: needinfo?(masayuki) → needinfo?(continuation)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

Can I know the URL of crashed web apps (via email or something)?

There's one URL that shows up twice in crash reports: https://webk.telegram.org/

Flags: needinfo?(continuation)

Thank you. That's helpful even though I cannot reproduce this bug.

Perhaps, this depends on IME. If IME does not consume Escape key or something which is handled by web apps to move focus or something, then, contenteditable may be set to false before we delete the text node from editor.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

It seems that this requires a run of a legacy mutation event listener, but there are no one at least in the chat at least...

Priority: -- → P3

Although we don't have a testcase, HTMLEditor may need to delete a text node
if it's created for composition. If it's not allowed, users may see staying
composition string in disabled editor.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/de32238c780f
Make `DeleteNodeTransaction` allow to create its instance for non-editable text node which was created or modified by an editor r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(masayuki)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #10)

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Hi Makoto-san,
Masayuki is away for a week. I don't think this is important enough to uplift to beta, as the patch mainly relaxed assertion criteria for non-release builds. Does that sound right?

Flags: needinfo?(masayuki) → needinfo?(m_kato)

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #11)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #10)

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Hi Makoto-san,
Masayuki is away for a week. I don't think this is important enough to uplift to beta, as the patch mainly relaxed assertion criteria for non-release builds. Does that sound right?

Yes, this issue is MOZ_DIAGNOSTIC_ASSERT_IF, and this assertion is enabled on nightly and dev edition only.

Flags: needinfo?(m_kato)
You need to log in before you can comment on or make changes to this bug.