1878862 - about support modified values not readable

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[Edo Karabas]

Attachment: values.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:123.0) Gecko/20100101 Firefox/123.0

Steps to reproduce:

1. Create brand new Firefox profile (123 beta)
2. Create New Firefox Sync account
3. Sync
4. Check about:support important modified values

Actual results:

Values are not readable
Column with names are just numbers
Column with values are with different letters - from top to down it is saying: Tro at offsett 44 data proovider faiuled: modifbiedPreferelnces TypeEerror: malfsormed UTF-h8 characteor sequence
(note: every letter is in separate row, so I put it together)

Expected results:

Values should be readable when Sync account is connected.

Bug does not apear when Sync account is not connected and also disappear as soon as Sync is disconnected.
To be exact, after my investigation the bug disappear as soon as I remove key "services.sync.lastSync" in about:config, however after next sync the key is created and bug appears again.

Comment 1

[Edo Karabas]

Seems that last good build was from Sat Jan 13 09:19:22 2024 +0000
First bad from Sat Jan 13 21:58:47 2024 +0000
If I managed to pinpoint it correctly
Suspected bugs: Bug 1874376 and Bug 1869726 by Tooru Fujisawa [:arai]

Comment 2

[Tooru Fujisawa [:arai]]

Thank you for reporting!

The issue is reproducible with non-ASCII character being in the Date string.
I'll investigate the issue shortly.

moving to Core - JavaScript for now given both are related to external string.
I'll move to appropriate component (either xpconnect or DOM) once I figured out the details.

most likely the offending patches will be backed out.

Comment 3

[Tooru Fujisawa [:arai]]

So far, I've confirmed that the non-ASCII string hadn't been handled properly in sync or the "important modified values" table even before the change.
I see garbled string in the table, as "Wed Feb 07 2024 08:39:49 GMT+0900 (japonsk<?> atandardn<?> as)" in current release.

and apparently the change from those bugs made the situation worse.
If it's caused by the pref string handling, bug 1875678 patch should be the right fix.

I'll continue investigating.

Comment 4

[Edo Karabas]

Yes, in my case in the stable Firefox 122 I see:

Wed Feb 07 2024 00:53:45 GMT+0100 (stredoeurpsky atandardn as)

Not really sure why this string has to be so complicated, but whatever, words in the ( ) should be "stredoeurópsky štandardný čas" (european standard time) in Slovak language.

Comment 5

[Edo Karabas]

Attachment: cet.png

String in Firefox 122.
Garbled, but not causing issues.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tooru Fujisawa [:arai] from comment #3)

If it's caused by the pref string handling, bug 1875678 patch should be the right fix.

That's ready to land, let me know if I should pull the trigger.

Comment 7

[Tooru Fujisawa [:arai]]

The issue comes from bug 1874376 patch, XPCConvert::NativeData2JS change.
It had been accepting malformed data and performing lossy conversion (mozilla::LossyConvertUtf8toLatin1 and mozilla::ConvertUtf8toUtf16),
but XPCStringConvert::UTF8ToJSVal from the patch doesn't accept them and throws error.

So:

• bug 1875678 patch is correct fix for the consumer, given that passing malformed data is not good in general
• the change to XPCConvert::NativeData2JS needs to be backed out if the expected behavior is to accept malformed data

Comment 8

[Tooru Fujisawa [:arai]]

After more investigation, apparently XPCConvert::NativeData2JS had been accepting malformed data only when it's not at the end of the string.
And the pref/sync case haven't been hitting the issue because the date string always has closing ")" at the end of the string, and non-ASCII string appears before that.

So, the importance of the backout gets reduced, given the previous behavior was not "accept malformed data", but "accept malformed data only in some case".
Anyway, I'll prepare backout patches just in case.

Comment 9

[Tooru Fujisawa [:arai]]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #6)

(In reply to Tooru Fujisawa [:arai] from comment #3)

If it's caused by the pref string handling, bug 1875678 patch should be the right fix.

That's ready to land, let me know if I should pull the trigger.

Just to clarify, please land the bug 1875678 patch.
It should fix the issue (both the pre-existing issue about garbled characters and the current issue about error) regardless of whether bug 1874376 patch gets backed out or not.

Comment 10

[Tooru Fujisawa [:arai]]

actually, the above comment about XPCConvert::NativeData2JS behavior around end of string was wrong. please ignore it.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1878862 - Part 1: Revert bug 1874376 change to accept malformed UTF-8 string in XPCConvert. r=nika!

Before bug 1874376, XPCConvert::NativeData2JS had been allowing malformed UTF-8
sequence.

Bug 1874376 made it not accept any malformed UTF-8 string, and it caused
compatibility issue for the case where malformed input had been passed into.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1878862 - Part 2: Add tests to verify malformed UTF-8 is accepted in prefs. r?nika!

Comment 13

[Pascal Chevrel (relman team) -> :pascalc]

This is P1/S2, we will want to land and uplift the patch to beta this week.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/577ff7120699
Part 1: Revert bug 1874376 change to accept malformed UTF-8 string in XPCConvert. r=nika
https://hg.mozilla.org/integration/autoland/rev/bb7ec83aef1c
Part 2: Add tests to verify malformed UTF-8 is accepted in prefs. r=nika

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1878862 - Part 1: Revert bug 1874376 change to accept malformed UTF-8 string in XPCConvert. r=nika!

Before bug 1874376, XPCConvert::NativeData2JS had been allowing malformed UTF-8
sequence.

Bug 1874376 made it not accept any malformed UTF-8 string, and it caused
compatibility issue for the case where malformed input had been passed into.

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

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1878862 - Part 2: Add tests to verify malformed UTF-8 is accepted in prefs. r?nika!

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

Comment 17

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: None
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Is Android affected?: yes
• User impact if declined: about:support doesn't show important modified preference values, which makes it hard to investigate issues
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: Low
• Needs manual QE test: no
• Explanation of risk level: This is backout for bug 1874376 patch, which is landed to 123

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f21e7fc0c1b

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/577ff7120699
https://hg.mozilla.org/mozilla-central/rev/bb7ec83aef1c

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/0dad10cb8843

Comment 21

[Pascal Chevrel (relman team) -> :pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/0dad10cb8843

Comment 22

[Edo Karabas]

Thanks for quick fix. Fixed in Nightly 124 as well as Beta 123 beta 9.