2048095 - Rewrapping of quotes if only the quote is selected fails

Status: RESOLVED DUPLICATE of bug 187997

(Core :: DOM: Editor, defect)

Description

[Francesco]

Attachment: rewrap.png

Rewrapping is mostly after manually editing plaintext replies, incl. newsgroup follow-ups in Thunderbird. It is also used to rewrap quotes, which might have been split to insert replies.

I understand that the algorithm was reworked in bug 230112. Unfortunately, rewrapping of quotes is now completely broken. I didn't test it on multi-level quotes, a single level is already broken after the first line.

In the attached picture you see a long quote, first rewrapped with TB 140 and then with TB 153. Disclaimer: The rewrapping code in TB 140 gave no deterministic result, sometimes is worked as shown, sometimes it produced a different result. Now the result is deterministic and consistently wrong.

Please test on a mix of quoted text and inserted new text.

Comment 1

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

Francesco: Could you clarify the steps to reproduce the bug?

Comment 2

[Francesco]

Sure. Answer an e-mail with plaintext or post a follow-up in a newsgroup which is always plaintext.

Delete almost everything and paste this text into the first quoted line so it looks like in the picture:

Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since 1966, when designers at Letraset and James Mosley, the librarian at St Bride Printing Library in London, took a 1914 Cicero translation and scrambled it to make dummy text for Letraset's Body Type sheets. It has survived not only many decades, but also the leap into electronic typesetting, remaining essentially unchanged. It was popularised thanks to these sheets and more recently with desktop publishing software including versions of Lorem Ipsum.

Then select that text and rewrap.

Comment 3

[Max Emig [:maxe]]

The rewrapping code in TB 140 gave no deterministic result, sometimes is worked as shown, sometimes it produced a different result.

It was very weird, but deterministic in the sense same input, same output. For example, a certain input caused OOM in 100% of cases.

I am unable to reproduce this case right now solely based on the InternetCiter.cpp implementation.

If you consider sth a regression, you need a case that worked before, otherwise it is just a normal bug. Between 140 and 152 there were no relevant changes made, you can just use either ESR or 151 or Beta 152 for comparison.

There may be one or more issues with HTMLEditorDataTransfer.cpp or nsPlainTextSerializer.cpp both of which I did not touch matching your described behaviour.

Very wild guess, this could be a duplicate of bug 1140894.

Cheers

Max

Comment 4

[Francesco]

Attachment: rewrap.eml

I don't think that the problem will be resolved by blaming it onto 15 y/o bug 1140894. If you need a reproducible case, here it is. Import this message by dragging it into a drafts folder, edit it, select the quote and rewrap it. I noticed that it woks better when nothing is selected, that would be bug 187997.

Works in 140, doesn't work now. So where in the algorithm does it add the > at the front of the line when creating a new line?

Comment 5

[Max Emig [:maxe]]

I don't think that the problem will be resolved by blaming it onto 15 y/o bug 1140894.

Francesco, I did a fuzzing run of over 1 million inputs comparing old and new function results. I am not blaming, just troubleshooting.

This is what I get out, both on Thunderbird 152 (before the patch) and Thunderbird 154 (latest Daily) when not selecting anything:

On 19 Feb 2025 21:23, user wrote:
> Lorem Ipsum is simply dummy text of the printing and typesetting
> industry. Lorem Ipsum has been the industry's standard dummy text
> ever since 1966, when designers at Letraset and James Mosley, the
> librarian at St Bride Printing Library in London, took a 1914 Cicero
> translation and scrambled it to make dummy text for Letraset's Body
> Type sheets. It has survived not only many decades, but also the
> leap into electronic typesetting, remaining essentially unchanged.
> It was popularised thanks to these sheets and more recently with
> desktop publishing software including versions of Lorem Ipsum.

And when selecting only the quote and then pressing rewrap, incorrect but identical on both:

On 19 Feb 2025 21:23, user wrote:
> Lorem Ipsum is simply dummy text of the printing and typesettingindustry. Lorem Ipsum has been the industry's standard dummy text ever
since 1966, when designers at Letraset and James Mosley, the librarian
at St Bride Printing Library in London, took a 1914 Cicero translation
and scrambled it to make dummy text for Letraset's Body Type sheets. It
has survived not only many decades, but also the leap into electronic
typesetting, remaining essentially unchanged. It was popularised thanks
to these sheets and more recently with desktop publishing software
including versions of Lorem Ipsum.

I even had the Thunderbird 152 (Stable) open and rewrap my bug230112-rewrap-oom-proof.eml, crashed immediately and the crash reporter opened, which is proof that the rewrite did not somehow end up in there.

Edit: Now it fully reflects how it looks on my screen.

Comment 6

[Francesco]

Please accept my apologies, your fix is not the culprit. I ran mozregression and that took me to:
https://hg-edge.mozilla.org/comm-central/pushloghtml?fromchange=ffb4b5ddf32f5714b4e9d57a678be8edaf4ba35c&tochange=854d9293825c4ff4ad360fd179664608df2378dc
during the 142 period.

That roughly translates to
https://hg-edge.mozilla.org/mozilla-central/pushloghtml?startdate=2025-06-27&enddate=2025-06-29
which makes bug 1972762 a likely regression candidate.

Apparently this isn't covered by tests :-(

Comment 7

[Max Emig [:maxe]]

(In reply to Francesco from comment #6)

Please accept my apologies, your fix is not the culprit. I ran mozregression and that took me to:
https://hg-edge.mozilla.org/comm-central/pushloghtml?fromchange=ffb4b5ddf32f5714b4e9d57a678be8edaf4ba35c&tochange=854d9293825c4ff4ad360fd179664608df2378dc
during the 142 period.

That roughly translates to
https://hg-edge.mozilla.org/mozilla-central/pushloghtml?startdate=2025-06-27&enddate=2025-06-29
which makes bug 1972762 a likely regression candidate.

Apparently this isn't covered by tests :-(

I didn't check as far back as that, also in a slightly different place than I suspected.

I can take care of that during the next week.

It's also a bit of the inherent issue that we are relying on Firefox-code whose only real consumer we are.

We would need a Thunderbird mochitest against a Firefox/Gecko code path or rebuilding even more than that in highly synthetic tests.

Maybe it's a good idea to fix more issues in this patch, would need more data what else is still legitimately broken.

Cheers

Max

Comment 8

[Francesco]

I can take care of that during the next week.

Thanks for the offer. Since we mentioned bug 1140894 and bug 187997, I took a look at what they are reporting. The former only contains "anecdotal evidence" documented with pictures, the latter contains a sample messages (attachment 114313 [details], attachment 157658 [details], attachment 458104 [details]). Maybe there are some usable text cases. In any case, one test case was presented here (attachment 9598222 [details]).

Comment 9

[Max Emig [:maxe]]

The situation may be even worse, we are dealing with a post-pin refactor in an adjacent area. I may just have to wait on the next pin/merge.

Comment 10

[Francesco]

Please clarify. "Post-pin", "pin/merge"?

Comment 11

[Max Emig [:maxe]]

I was wrong on this, I was in a hurry on the go.

Please clarify.

Post-pin

Newer code in Firefox/Gecko than we are using in Thunderbird, currently 5 days old.

pin/merge

Usually done in a no-bug commit.

Comment 12

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

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

Comment 13

[Magnus Melin [:mkmelin]]

(In reply to Max Emig from comment #11)
Not sure how you got into that state. If you're using latest tip there is no such thing. c-c tip is not pinned, but uses m-c tip

Comment 14

[Max Emig [:maxe]]

I disagree with the previous regression assumption.

Comment 15

[Francesco]

(In reply to Max Emig from comment #14)

I disagree with the previous regression assumption.

Can you motivate this?

So which bug do you suggest regressed it then. Based on
https://hg-edge.mozilla.org/comm-central/pushloghtml?fromchange=ffb4b5ddf32f5714b4e9d57a678be8edaf4ba35c&tochange=854d9293825c4ff4ad360fd179664608df2378dc
the more precise M-C range is:
https://hg-edge.mozilla.org/mozilla-central/pushloghtml?fromchange=262215ce9b5579f207c98d8554ee9cf02613f6d3&tochange=c3bc8b9da99f2c7f5d9728e8c40edfc3e7669f8f

There is only one possible candidate in this push.

Comment 16

[Max Emig [:maxe]]

I am still not fully convinced this even is a regression at all.

I have 140.11.0esr (64-bit) on Debian here, if I select just the text without the > and rewrap it works, but if I select everything starting with the 2nd line, I get the exact same bad result.

Is your 140 behaving differently from mine?

But either way, I devised a working ~10 LOC fix that only needs a little more scrutiny in an area untouched during your alleged regression window.

Comment 17

[Max Emig [:maxe]]

Regressed by: 1972762

Would you like to build the fix, if you know better?

I am now back at my desktop, I have hg annotate, various ways to abuse unit-tests, creative use of searchfox-cli as well as bespoke tooling to check the equivalence of refactored functions and wasted hours on this.

This claimed bug id does not hold up, but if it makes you feel better you can request a backout.

Comment 18

[Francesco]

(In reply to Max Emig from comment #16)

if I select just the text without the > and rewrap it works

It doesn't work here. Yes, it rewraps, but the lines aren't prepended with >.

Is your 140 behaving differently from mine?

I repeated the test now, and it equally fails in 140. However, it worked before as documented in the original screenshot. As I wrote initially:
The rewrapping code in TB 140 gave no deterministic result, sometimes is worked as shown, sometimes it produced a different result.

I have no further insights.

But either way, I devised a working ~10 LOC fix that only needs a little more scrutiny in an area untouched during your alleged regression window.

Thanks, it would be good to get this fixed so it behaves correctly consistently.

Would you like to build the fix, if you know better?

No, I'm only triaging and testing, not developing. I ran mozregression and that led me to the stated regression range and therefore the regressor. You can repeat mozregression. Of course, if rewrapping results are not deterministic, then mozregression doesn't help.

This claimed bug id does not hold up, but if it makes you feel better you can request a backout.

I don't understand this comment. Bug 1972762 was landed in the 142 cycle, about a year ago, it consisted of various changeset. Impossible to back out. Besides, Mozilla policy is not to back out, but to fix regressions. Also, some regression in a function (rewrap) not used by Firefox won't be a reason for a backout.

Comment 19

[Max Emig [:maxe]]

I got this out of Thunderbird 140, hence no regression on this part. We may be talking about more than one bug here. :(

On 19 Feb 2025 21:23, user wrote:
> Lorem Ipsum is simply dummy text of the printing and typesetting
industry. Lorem Ipsum has been the industry's standard dummy text ever
since 1966, when designers at Letraset and James Mosley, the librarian
at St Bride Printing Library in London, took a 1914 Cicero translation
and scrambled it to make dummy text for Letraset's Body Type sheets. It
has survived not only many decades, but also the leap into electronic
typesetting, remaining essentially unchanged. It was popularised thanks
to these sheets and more recently with desktop publishing software
including versions of Lorem Ipsum.

Comment 20

[Francesco]

I re-tested it in 140. Without spellchecking enabled, it re-wraps correctly, with spellchecking enabled and lots of red underlines, it fails. This seems to be reproducible. IIRC, the mozregression ran with an en-US dictionary only and no underlines. So there could be various conditions that trigger the bug or not.

Comment 21

[Max Emig [:maxe]]

It is no doubt very brittle, if you so firmly believe this is a regression can you use the same profile that worked for you on 140 on 154?

Comment 22

[Francesco]

Unfortunately, I cannot, since it would be irreversibly upgraded. Running 140 on the Daily profile fails all the time. Why don't we stop the discussion about regression/regressor and focus on a fix?

Comment 23

[Max Emig [:maxe]]

Unfortunately, I cannot, since it would be irreversibly upgraded.

Profiles can be copied/backed up.

Why don't we stop the discussion about regression/regressor and focus on a fix?

Because you re-added an incorrect regressed by and we still have a triage owner watching. And the fix discussion/review will live on Phabricator anyway.

Comment 24

[Max Emig [:maxe]]

I am out here, D307590 for bug 187997 is for this behaviour.

One last time: I did not observe any regression in testing or any codepath that makes a regression likely.

Comment 25

[Francesco]

I retested this. The result depends on what exactly is selected. If only the quote is selected, the rewrap fails. If the linebreak after the quote is also selected, the rewrap works. Likely when running mozregression I selected the quotes in a different way.

It will hopefully be fixed in bug 187997. Sorry about the noise.