1672407 - "browser.compose.setComposeDetails"-API when running on Windows: content of "body" will be mis-interpreted; doubling Windows-style linefeeds

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[Achim Seufert]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:82.0) Gecko/20100101 Firefox/82.0

Steps to reproduce:

When using this API-call ...

browser.compose.setComposeDetails(tab.id, { plainTextBody: body });

... the content of "body" will be mis-interpreted; doubling Windows-style linefeeds.

I explained/documented everything in detail here:
https://github.com/4ch1m/sandbox/tree/master/composer-api

A simple sample extension that can reproduce the bug is enclosed.
(Sorry for not putting it all in here.)

Actual results:

I explained/documented everything in detail here:
https://github.com/4ch1m/sandbox/tree/master/composer-api

A simple sample extension that can reproduce the bug is enclosed.
(Sorry for not putting it all in here.)

Expected results:

I explained/documented everything in detail here:
https://github.com/4ch1m/sandbox/tree/master/composer-api

A simple sample extension that can reproduce the bug is enclosed.
(Sorry for not putting it all in here.)

Comment 1

[C-E]

Bug confirmed using TB 78.5.0 (32 bit) and Windows 10 Pro 20H2 Build 19042.630

Comment 2

[John Bieling (:TbSync)]

It appears to happen during setting the content already. SetComposeDetails() eventually calls editor.insertText() . One can also execute the following from the console in a compose window:

window.GetCurrentEditor().insertText("123\r\n123\r\n");

On Windows 10: The content "looks" right, but if copy-pasted into an editor which is capable of showing line endings, one can see, that the extra \r\n are already there. But it appears the editor collapses \r\n\r\n into a single linebreak. This

window.GetCurrentEditor().insertText("123\n123\n");

works correct (and gives \r\n per line break).

@Magnus: The editor code is in M-C and they explicitly have this:

// Note that we don't need to replace native line breaks with XP line breaks
// here because Chrome does not do it.

https://searchfox.org/comm-central/source/mozilla/editor/libeditor/EditorBase.cpp#4985-4986

Instead of changing anything there (which I do not think is a good idea), I would simply sanitize the input for the WebExtension compose.setComposeDetails and replace any \r\n by \n? This is also done in other code paths by the composer directly (https://searchfox.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#2717) but not in this case. Ok with you?

Comment 3

[Magnus Melin [:mkmelin]]

I think so.

Comment 4

[John Bieling (:TbSync)]

Attachment: bug1672407_sanitize_new_line_characters_for_SetComposeDetails.patch

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=046752251f8338b8209c8b5e81dce3ba664886b3

Comment 5

[John Bieling (:TbSync)]

Attachment: bug1672407_sanitize_new_line_characters_for_SetComposeDetails_v2.patch

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9191909 [details] [diff] [review]
bug1672407_sanitize_new_line_characters_for_SetComposeDetails_v2.patch

Review of attachment 9191909 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4386,5 @@
>      gMsgCompose.bodyModified = true;
>    }
>    if (typeof newValues.plainTextBody == "string") {
>      editor.selectAll();
> +    // remove \r from line endings, which cause extra newlines (bug 1672407)

Nit: use real sentence so capitalize Remove and a period at the end.

::: mail/components/extensions/test/browser/browser_ext_compose_details.js
@@ +335,5 @@
> +          if (field in expected) {
> +            browser.test.assertEq(
> +              expected[field],
> +              state[field],
> +              `Check value for ${field}`

It's worth considering how these messages will look for both ok and not-ok cases. 

Would suggest `isPlaintext ${field} is ok `
Similar for the other ones. If you just have "check value x", it's harder to decipher what fails when something starts failing.

Comment 7

[John Bieling (:TbSync)]

Attachment: bug1672407_sanitize_new_line_characters_for_SetComposeDetails_v3.patch

${field} does contain the name of the field in question, so the message looks like so:

  FAIL Check value for plainTextBody - Expected: ["123","456","789"], Actual: ["123","","456","","789"] -

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/07818cec5c26
"browser.compose.setComposeDetails"-API: sanitize new line characters for SetComposeDetails. r=mkmelin

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dac080bf62e6
followup - fix linting. rs=eslint DONTBUILD

Comment 10

[John Bieling (:TbSync)]

Comment on attachment 9192129 [details] [diff] [review]
bug1672407_sanitize_new_line_characters_for_SetComposeDetails_v3.patch

[Approval Request Comment]
User impact if declined:
Add-on developers can not use the latest fixes and improvements of our WebExtension API.

Testing completed (on c-c, etc.):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=3cdff293d5d1bd77777f5da371caaa02c4b05de3

Risk to taking this patch (and alternatives if risky):
I hope none

Remark:
The provided try run includes all bugs I want to uplift for TB 78.7 and shows a working patch order.
The first one, which does not have a bug number in the commit messages is Bug 1680653.
This bug is the 9th one.

Comment 11

[Wayne Mery (:wsmwk)]

Comment on attachment 9192129 [details] [diff] [review]
bug1672407_sanitize_new_line_characters_for_SetComposeDetails_v3.patch

[Triage Comment]
Approved for esr78

Comment 12

[Dave Miller [:justdave]]

Thunderbird 78.7.0:
https://hg.mozilla.org/releases/comm-esr78/rev/a98fd8a250d4
https://hg.mozilla.org/releases/comm-esr78/rev/ee72377a3ecc