1941520 - Bug 1940278 makes fastmail composer hang on paste

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: Tab hang during common edit action on popular editor

STR:

• Go to app.fastmail.com, and compose an email.
• Insert some text that ends on an ascii whitespace like "foo ".
• Paste some other text.

ER:

• Pasting works.

AR:

• The tab hangs.

I debugged it and this loop doesn't terminate:

        if (/*isBlock( container ) && */ offset !== getLength(container)) {
            // Collect any inline contents of the block after the range point
            blockContentsAfterSplit = document.createDocumentFragment();
            while ((node = container.childNodes[offset])) {
                blockContentsAfterSplit.appendChild(node);
            }
        }

The issue is that the editor seems to be inserting a <br> every time the last <br> is removed (container is a <div> with two nodes at the point of the hang, a text node and a <br>).

Mozregression points to bug 1940278, Masayuki can you take a look?

Source for the editor seems here but unfortunately the bug doesn't seem to reproduce on https://fastmail.github.io/Squire/

Comment 1

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: Hmm, the regressor was uplifted to beta...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

Comment 3

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

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

Comment 4

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

Really thank you for the testcase. I'll take a look tomorrow.

Comment 5

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

I didn't understand why the patch caused the hang, but I got it now.

paste event and beforeinput events are fired before setting the toplevel edit sub-action. Therefore, we pass this guard here. However, the edit action has already been set to ePaste (or something). Thus, it passes into the new path. Then, replacing the last collapsible white-space with an NBSP causes setting the top level editing sub-action. Finally, OnEndHandlingTopLevelEditSubAction is called and that calls WhiteSpaceVisiblityKeeper::NormalizeVisibleWhiteSpacesAt and that causes replacing the trailing NBSP back to the collapsible white-space with inserting a <br> element which will be removed by JS.

Comment 6

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

Attachment: Bug 1941520 - Make `HTMLEditor::OnModifyDocument` set top level edit sub-action before maintaining the white-space visibility r=m_kato!

When pasting text, a paste event is fired before a beforeinput event.
Therefore, the editor still does not have top level edit sub-action. Therefore,
the DOM mutation caused by the web app will be handled by
HTMLEditor::OnModifyDocument immediately after removing the script blocker.
At this time, we may do:

• insert a padding <br> if something immediately after last input is removed
• replace a collapsible white-space if padding <br> is removed

Then, each handler sets the top level edit sub-action and the post-processor
will run immediately. Then, especially in the latter case,
WhiteSpaceVisibilityKeeper::NormalizeVisibleWhiteSpacesAt will insert a
padding <br> again and restores the replaced NBSP to a collapsible
white-space unexpectedly.

Therefore, if web apps trying to normalize the pasted content with removing
the pasted nodes temporarily, it may cause entering an infinite loop.

This patch makes HTMLEditor::OnModifyDocument set edit sub-action for the
hacks to avoid running the post-processor.

Additionally, this touches EditorBase::DoTransactionInternal to avoid the
assertion failure of the new test. The assertion failure indicates a logical
bug of our basic strategy. However, we should not touch the big design change
for now. (Anyway, the hack should be removed as soon as possible.)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac03643f4fa9
Make `HTMLEditor::OnModifyDocument` set top level edit sub-action before maintaining the white-space visibility r=m_kato

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/50091 for changes under testing/web-platform/tests

Comment 9

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

Attachment: Bug 1941520 - Make `HTMLEditor::OnModifyDocument` set top level edit sub-action before maintaining the white-space visibility r=m_kato!

When pasting text, a paste event is fired before a beforeinput event.
Therefore, the editor still does not have top level edit sub-action. Therefore,
the DOM mutation caused by the web app will be handled by
HTMLEditor::OnModifyDocument immediately after removing the script blocker.
At this time, we may do:

• insert a padding <br> if something immediately after last input is removed
• replace a collapsible white-space if padding <br> is removed

Then, each handler sets the top level edit sub-action and the post-processor
will run immediately. Then, especially in the latter case,
WhiteSpaceVisibilityKeeper::NormalizeVisibleWhiteSpacesAt will insert a
padding <br> again and restores the replaced NBSP to a collapsible
white-space unexpectedly.

Therefore, if web apps trying to normalize the pasted content with removing
the pasted nodes temporarily, it may cause entering an infinite loop.

This patch makes HTMLEditor::OnModifyDocument set edit sub-action for the
hacks to avoid running the post-processor.

Additionally, this touches EditorBase::DoTransactionInternal to avoid the
assertion failure of the new test. The assertion failure indicates a logical
bug of our basic strategy. However, we should not touch the big design change
for now. (Anyway, the hack should be removed as soon as possible.)

Original Revision: https://phabricator.services.mozilla.com/D234278

Comment 10

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: May see hungup
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: Paste something into the attached testcase (pasting text ending with/without white-space)
• Risk associated with taking this patch: Low
• Explanation of risk level: This patch touches only the path in the hack in the DOM mutation observer of HTMLEditor. I.e., this won't run in usual editable apps because they won't touch Gecko specific padding <br> elements.
• String changes made/needed: none
• Is Android affected?: yes

Comment 11

[pstanciu]

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

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/e46e3dc621b4

Comment 14

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 135.0b5 on Windows 10 x64. Pasting in the attached test case from comment 2 will hang the content.
The issue is verified fixed with Firefox 136.0a1 (2025-01-15) and 135.0b6 (20250116003737 - comment 13) on Windows 10x64, macOS 12 and Ubuntu 24. Pasting in the attached test case will no longer hang the page content.