1415567 - Remove {get,set}ComplexValue use of nsISupportsString in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Florian Quèze [:florian]]

Bug 1345294 removed the callers for m-c, bug 1414096 removed support from the platform.

Comment 1

[Florian Quèze [:florian]]

Attachment: script generated patch [landed with slight mods and backed out]

Comment 2

[Florian Quèze [:florian]]

Attachment: hand written fix [landed and backed out]

The script couldn't handle the following cases:
ignoring suite/modules/PlacesUIUtils.jsm:80 because the third instruction isn't the call
ignoring suite/common/utilityOverlay.js:1570 because the third instruction isn't the call
ignoring suite/common/src/nsSuiteGlue.js:691 because the second instruction isn't the expected assigment
ignoring suite/common/src/nsSuiteGlue.js:1526 because the third instruction isn't the call
ignoring suite/browser/nsBrowserContentHandler.js:163 because the third instruction isn't the call
ignoring suite/browser/nsBrowserContentHandler.js:215 because the third instruction isn't the call
ignoring mail/base/modules/distribution.js:45 because the second instruction isn't the expected assigment

I'm fixing the mail/ file by hand with this patch.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2bfef5c6f2a7
Port bug 1414096 to TB: Remove {get,set}ComplexValue use of nsISupportsString (script-generated patch). r=jorgk
https://hg.mozilla.org/comm-central/rev/48e041d4c81b
Port bug 1414096 to TB: Remove {get,set}ComplexValue use of nsISupportsString (hand-written patch). r=jorgk

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8926411 [details] [diff] [review]
script generated patch [landed with slight mods and backed out]

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

Thanks.

::: chat/components/src/imAccounts.js
@@ +232,5 @@
>  
>        if (this.canJoinChat &&
>            this.prefBranch.prefHasUserValue(kPrefAccountAutoJoin)) {
> +        let autojoin = this.prefBranch.getStringPref(
> +          kPrefAccountAutoJoin);

I took the liberty to join this to the previous line.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8926415 [details] [diff] [review]
hand written fix [landed and backed out]

Thanks again.

Comment 6

[Jorg K (CEST = GMT+2)]

FRG, Florian didn't cover the cases listed in comment #2. I landed this in a hurry, so I have to leave this to you. Shouldn't be hard since you have plenty of examples.

Florian, I had to s/è/e/ since landing this from Windows would have ended up in mojibake, I know since we have a contributor with a "ü".

Thanks for the patches. A couple of hours earlier would have been better since now we shipped a completely busted Daily build :-(

Comment 7

[Richard Marti (:Paenglab)]

Chat and newsgroups looking fixed. But mail and feed accounts are still busted f.ex. "nobody on Local Folders" and renaming doesn't work.

Comment 8

[Florian Quèze [:florian]]

(In reply to Richard Marti (:Paenglab) from comment #7)
> Chat and newsgroups looking fixed. But mail and feed accounts are still
> busted f.ex. "nobody on Local Folders" and renaming doesn't work.

There's likely also some C++ code that needs fixing. FWIW the platform patch has just been backed out due to a devtools (trivial) test failure.

Comment 9

[Jorg K (CEST = GMT+2)]

Looks like all these might need fixing. I'll do it in a minute:
https://dxr.mozilla.org/comm-central/search?q=NS_GET_IID(nsISupportsString&redirect=true

Comment 10

[Frank-Rainer Grahl (:frg)]

You sure?

> ignoring suite/modules/PlacesUIUtils.jsm:80 because the third instruction isn't the call

e.g. I think this is not affected. Only support for prefs was removed or do I miss something?

FRG

Comment 11

[Florian Quèze [:florian]]

(In reply to Frank-Rainer Grahl (:frg) from comment #10)
> You sure?
> 
> > ignoring suite/modules/PlacesUIUtils.jsm:80 because the third instruction isn't the call
> 
> e.g. I think this is not affected. Only support for prefs was removed or do
> I miss something?

Right, this specific instance isn't affected. The script just dumped to the terminal the places in the code that need a human review.

Comment 12

[Frank-Rainer Grahl (:frg)]

> Looks like all these might need fixing.

Ok they are all affected. Just need to check the js.

Comment 13

[Frank-Rainer Grahl (:frg)]

I think all of the suite source cases not handled by the script are fine. Thanks for fixing the others.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> There's likely also some C++ code that needs fixing. FWIW the platform patch
> has just been backed out due to a devtools (trivial) test failure.
ETA: One hour from now.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1415567-complex-value.patch (v1)

Missed the deadline by two minutes :-(

Richard, all working now?
Aceman, review now or PLR? I give you 60 minutes ;-)

Comment 16

[Richard Marti (:Paenglab)]

Comment on attachment 8926507 [details] [diff] [review]
1415567-complex-value.patch (v1)

Looks much better. Thanks.

Comment 17

[Jorg K (CEST = GMT+2)]

Notes for the reviewer:
There is also nsPrefBranch::GetStringPref()
https://dxr.mozilla.org/comm-central/source/mozilla/modules/libpref/Preferences.cpp#2413
which is geared around JS and setting a default value. It calls GetCharPref() anyway.

nsPrefBranch::SetStringPref()
https://dxr.mozilla.org/comm-central/source/mozilla/modules/libpref/Preferences.cpp#2434
calls SetCharPrefInternal() which in the end calls PREF_SetCStringPref() just like nsPrefBranch::SetCharPref() via nsPrefBranch::SetCharPrefInternal().

The [Get|Set]StringPref() functions are not used in M-C C++ code (since they are geared around JS), so I opted for the [Get|Set]CharPref() functions.

http://searchfox.org/mozilla-central/search?q=SetStringPref(&case=false&regexp=false&path=*.cpp
http://searchfox.org/mozilla-central/search?q=GetStringPref(&case=false&regexp=false&path=*.cpp

Comment 18

[Frank-Rainer Grahl (:frg)]

Attachment: 1415567-lonelysuitechange.patch

Jorg, found a lonely occurence in suite not covered. Tested and works. Could you give it your blessing and check it in with the other.

Lookd at the remaining ones in mail and mailnews too and they all seem to be ok.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8926514 [details] [diff] [review]
1415567-lonelysuitechange.patch

Thanks, I'll check it in with the C++ patch. I'll change the commit message.

Comment 20

[:aceman]

Comment on attachment 8926507 [details] [diff] [review]
1415567-complex-value.patch (v1)

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

Reading bug 1345294, it seems setCharPref works in Latin1. I don't think it is safe to push UTF-8 to it. Please use *StringPref() even if it may look the same today. It may still be touched in the future and we want those changes.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1415567-complex-value.patch (v2)

Done.

Comment 22

[:aceman]

Comment on attachment 8926532 [details] [diff] [review]
1415567-complex-value.patch (v2)

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

::: mailnews/base/src/nsMsgTagService.cpp
@@ +437,5 @@
>  
>  nsresult nsMsgTagService::GetUnicharPref(const char *prefName,
>                                nsAString &prefValue)
>  {
> +  nsCString valutf8;

Please call this utf8value. 'valutf8' makes me always think there is some character missing in the name ;)

::: mailnews/base/test/TestMsgStripRE.cpp
@@ +67,2 @@
>    // set localizedRe pref
> +  rv = prefBranch->SetStringrPref("mailnews.localizedRe", NS_ConvertUTF8toUTF16(utf8Prefixes));

Typo in SetStringrPref() . Also, utf8Prefixes is already in utf-8 so why convert to utf16? It just needs to be adopted into a nsCString to send to SetStringPref().

::: mailnews/base/util/nsMsgIdentity.cpp
@@ +438,5 @@
>  {
>    if (!mPrefBranch)
>      return NS_ERROR_NOT_INITIALIZED;
>  
> +  nsCString valutf8;

utf8value please.

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +521,5 @@
>  {
>    if (!mPrefBranch)
>      return NS_ERROR_NOT_INITIALIZED;
>  
> +  nsCString valutf8;

utf8value

::: mailnews/base/util/nsMsgUtils.cpp
@@ +1263,5 @@
>      pbr = do_GetService(NS_PREFSERVICE_CONTRACTID);
>      prefBranch = pbr;
>    }
>  
> +  nsCString valutf8;

utf8value

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1415567-complex-value.patch (v3)

Fixed variable name and typo. Good to go now?

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1415567-complex-value.patch (v3b)

Forgot to fix TestMsgStripRE.cpp.

Comment 25

[:aceman]

Comment on attachment 8926551 [details] [diff] [review]
1415567-complex-value.patch (v3b)

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

OK, builds for me, but the TestMsgStripRE.cpp does not seem to be compiled as part of normal build.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 1415567-lonelysuitechange.patch

With correct check-in message.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1415567-complex-value.patch (v3c)

This compiles for TestMsgStripRE.cpp and removes the horrible octal stuff.

Comment 28

[Jorg K (CEST = GMT+2)]

All too late now since bug 1414096 was backed out. So backout of the first two landed changesets:
https://hg.mozilla.org/comm-central/rev/3617ae4c65c86ddcc56f0b2b8651e7bbf73b0132
https://hg.mozilla.org/comm-central/rev/497ee9e16aff5daeb90d253773876185893f1e62

Comment 29

[Jorg K (CEST = GMT+2)]

So we'll need to reland all four patches when bug 1414096 relands.

Comment 30

[:aceman]

Comment on attachment 8926582 [details] [diff] [review]
1415567-complex-value.patch (v3c)

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

::: mailnews/base/test/TestMsgStripRE.cpp
@@ +61,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // set localizedRe pref, value "SV,ÆØÅ"
> +  rv = prefBranch->SetStringPref("mailnews.localizedRe",
> +                                 NS_LITERAL_CSTRING("SV,\xC3\x86\xC3\x98\xC3\x85"));

Probably OK, but please keep some comment that this is already utf8 encoded so we do not wonder in the future.

@@ +66,1 @@
>    NS_ENSURE_SUCCESS(rv, rv);

For me these didn't compile are the function ought to return int. So NS_ENSURE_SUCCESS(rv, 1); worked for me in both cases.

Comment 31

[Nicholas Nethercote [inactive]]

Apologies for the breakage. I was intending to fix comm-central but it slipped my mind at the end :(

Also, I don't think these patches needed to be backed out. They should be landable before bug 1414096 because they are switching from [gs]etComplexValue() to [gs]etStringPref(), which already works. I'm happy to wait for these patches to reland before relanding bug 1414096 if that's helpful?

Comment 32

[:aceman]

Yeah, we may only need bug 1345294 in to get *StringPref() and the we can already use it with these patches.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #30)
> Probably OK, but please keep some comment that this is already utf8 encoded
> so we do not wonder in the future.
That's actually quite obvious for someone working in that field.

> For me these didn't compile are the function ought to return int. So
> NS_ENSURE_SUCCESS(rv, 1); worked for me in both cases.
You go and fix it then in another bug. And add the comment you like.

(In reply to Nicholas Nethercote [:njn] from comment #31)
> Also, I don't think these patches needed to be backed out. They should be
> landable before bug 1414096 because they are switching from
> [gs]etComplexValue() to [gs]etStringPref(), which already works. I'm happy
> to wait for these patches to reland before relanding bug 1414096 if that's
> helpful?
Oops, how silly of me. I told Aceman that he didn't need to rebuild with the latest M-C to test this. Well, too late, they are backed out. You can reland bug 1414096 any time, I'm watching the M-C merges.

But while we're here, I'll f? you on the C++ patch.

Comment 34

[Jorg K (CEST = GMT+2)]

Comment on attachment 8926582 [details] [diff] [review]
1415567-complex-value.patch (v3c)

We were discussing whether to use [Get|Set]CharPref or [Get|Set]StringPref. Internally it's all the same, no? There's a superseded patch that uses [Get|Set]CharPref .

Comment 35

[Nicholas Nethercote [inactive]]

> We were discussing whether to use [Get|Set]CharPref or [Get|Set]StringPref.
> Internally it's all the same, no? There's a superseded patch that uses
> [Get|Set]CharPref .

They are definitely not the same! {Get,Set}StringPref and treat the strings as UTF8 (AUTF8String in the .idl), same as {Get,Set}ComplexValue(nsISupportsString). But {Get,Set}CharPref treat the strings as Latin1 (ACString in the .idl).

In practice it might not make a difference, depending on the exact combination of getters and setters used. (Problems occur if you pass non-UTF8 strings through XPIDL methods that expect UTF8; it will throw "invalid UTF8" errors.)

But in summary, you should use {Get,Set}StringPref, because that matches the existing code and will avoid any potential problems.

Comment 36

[Jorg K (CEST = GMT+2)]

(In reply to Nicholas Nethercote [:njn] from comment #35)
> They are definitely not the same!
I was talking about C++ code only. According to my comment #17, the C++ interfaces {Get,Set}StringPref and {Get,Set}CharPref do the same thing.

Comment 37

[Nicholas Nethercote [inactive]]

Oh, from C++ code:

- SetStringPref and SetCharPref are identical.

- GetStringPref and GetCharPref have slightly different interfaces. GetCharPref is probably easier to use.

Comment 38

[Jorg K (CEST = GMT+2)]

That's right. That's why I suggested attachment 8926507 [details] [diff] [review] using {Get,Set}CharPref, but Aceman spotted this hunk https://hg.mozilla.org/mozilla-central/rev/e843de356b7e#l6.19 and made me switch to the String versions.

Both do pretty much the same. Maybe you can advise which patch should go ahead.

Comment 39

[Nicholas Nethercote [inactive]]

When I wrote that hunk I didn't realize that GetCharPref was a reasonable alternative. I might change that hunk in my own patch before landing.

Comment 40

[Nicholas Nethercote [inactive]]

Comment on attachment 8926582 [details] [diff] [review]
1415567-complex-value.patch (v3c)

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

This is fine, though as discussed you could switch from the "String" to the "Char" funcs, which would let you remove the `EmptyCString(), 0` arguments.

Comment 41

[:aceman]

I still do not agree here.
If we use *CharPref() and *StringPref() still exists, we will lose semantics that the passed in values are utf8 and are a bit different to plain latin1. We may need this information in the future. Like today, we could easily grep which calls are the full utf8/utf16 strings by looking for the *ComplexValue+nsISupportsString.
If the C++ implementation is the same NOW, you call *CharPref() from *StringPref() internally. But callers should use the right API.
It is as if in HTML you used <H1> for headings and for the second level headings you ignored that <H2> exists, but used <H1 style="fonts-size: 80%">, because "hey, it looks the same as H2 now and I see H2 is defined the same way internally in Firefox's default style". Yes, it may look the same for some time, but the sematics is lost that those are second level headings. And the default style may change in Firefox any time in the future.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: 1415567-complex-value.patch (v3d)

Added comment and fixed NS_ENSURE_SUCCESS(rv, 1) as per comment #30. Aceman should be so happy now.

Comment 43

[Jorg K (CEST = GMT+2)]

Since the M-C got backed out, we have time for some try runs.

Try with three JS patches only:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8a18d18307536b20525b938f9b92009754d835e7

Try with JS patches and C++ patch:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3d0bc149d876885b60dc1b9cff2f5cb957cf1fc9

Try run with C++ patch only:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2169f202bf3e672d0c8eb05ce0098b43d1e9f14

I'm doing this since last night, with the M-C patch applied and all four C-C patches, I got a failure in
  calendar/test/unit/test_ltninvitationutils.js
which can also be see here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=296efe53253a743c9645942f3b84608145a68313&selectedJob=143027414
and here
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=48e041d4c81b126540eb255ad6b4a5ccce86feb1&selectedJob=143083995

So let's see who is the culprit here.

Comment 44

[Jorg K (CEST = GMT+2)]

Tries are green, well, one failure on Linux with the C++ patch only. I'll land this during the day when my latest push turn out OK.

Comment 45

[:aceman]

Comment on attachment 8926754 [details] [diff] [review]
1415567-complex-value.patch (v3d)

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

I hope we are both happy now ;)

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3a0c2f6597ef
Port bug 1414096 to TB: Remove {get,set}ComplexValue use of nsISupportsString (script-generated patch). r=jorgk
https://hg.mozilla.org/comm-central/rev/4b31b1b319fc
Port bug 1414096 to TB: Remove {get,set}ComplexValue use of nsISupportsString (hand-written patch). r=jorgk
https://hg.mozilla.org/comm-central/rev/90cf5e82cb3d
Port bug 1414096 to SM: Remove {get,set}ComplexValue use of nsISupportsString (hand-written patch). r=jorgk
https://hg.mozilla.org/comm-central/rev/6025ae6cc524
Port bug 1414096 to mailnews: Remove {get,set}ComplexValue use of nsISupportsString (C++ code). r=aceman