Closed Bug 1415567 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 58.0

People

(Reporter: florian, Assigned: florian)

References

Details

(Keywords: regression)

Attachments

(4 files, 6 obsolete files)

Bug 1345294 removed the callers for m-c, bug 1414096 removed support from the platform.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Keywords: regression
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.
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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.
Attachment #8926411 - Flags: review+
Comment on attachment 8926415 [details] [diff] [review]
hand written fix [landed and backed out]

Thanks again.
Attachment #8926415 - Flags: review+
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 :-(
Flags: needinfo?(frgrahl)
Target Milestone: --- → Thunderbird 58.0
Chat and newsgroups looking fixed. But mail and feed accounts are still busted f.ex. "nobody on Local Folders" and renaming doesn't work.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
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
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
Flags: needinfo?(frgrahl)
(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.
> Looks like all these might need fixing.

Ok they are all affected. Just need to check the js.
I think all of the suite source cases not handled by the script are fine. Thanks for fixing the others.
(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.
Attached patch 1415567-complex-value.patch (v1) (obsolete) — Splinter Review
Missed the deadline by two minutes :-(

Richard, all working now?
Aceman, review now or PLR? I give you 60 minutes ;-)
Attachment #8926507 - Flags: review?(acelists)
Attachment #8926507 - Flags: feedback?(richard.marti)
Comment on attachment 8926507 [details] [diff] [review]
1415567-complex-value.patch (v1)

Looks much better. Thanks.
Attachment #8926507 - Flags: feedback?(richard.marti) → feedback+
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
Attached patch 1415567-lonelysuitechange.patch (obsolete) — Splinter Review
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.
Attachment #8926514 - Flags: review?(jorgk)
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.
Attachment #8926514 - Flags: review?(jorgk) → review+
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.
Attached patch 1415567-complex-value.patch (v2) (obsolete) — Splinter Review
Done.
Attachment #8926507 - Attachment is obsolete: true
Attachment #8926507 - Flags: review?(acelists)
Attachment #8926532 - Flags: review?(acelists)
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
Attached patch 1415567-complex-value.patch (v3) (obsolete) — Splinter Review
Fixed variable name and typo. Good to go now?
Attachment #8926532 - Attachment is obsolete: true
Attachment #8926532 - Flags: review?(acelists)
Attachment #8926545 - Flags: review?(acelists)
Forgot to fix TestMsgStripRE.cpp.
Attachment #8926545 - Attachment is obsolete: true
Attachment #8926545 - Flags: review?(acelists)
Attachment #8926551 - Flags: review?(acelists)
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.
Attachment #8926551 - Flags: review?(acelists) → review+
With correct check-in message.
Attachment #8926514 - Attachment is obsolete: true
Attachment #8926581 - Flags: review+
This compiles for TestMsgStripRE.cpp and removes the horrible octal stuff.
Attachment #8926551 - Attachment is obsolete: true
Attachment #8926582 - Flags: review+
Attachment #8926411 - Attachment description: script generated patch → script generated patch [landed with slight mods and backed out]
Attachment #8926415 - Attachment description: hand written fix → hand written fix [landed and backed out]
So we'll need to reland all four patches when bug 1414096 relands.
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.
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?
Yeah, we may only need bug 1345294 in to get *StringPref() and the we can already use it with these patches.
(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 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 .
Attachment #8926582 - Flags: feedback?(n.nethercote)
> 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.
(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.
Oh, from C++ code:

- SetStringPref and SetCharPref are identical.

- GetStringPref and GetCharPref have slightly different interfaces. GetCharPref is probably easier to use.
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.
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 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.
Attachment #8926582 - Flags: feedback?(n.nethercote) → feedback+
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.
Added comment and fixed NS_ENSURE_SUCCESS(rv, 1) as per comment #30. Aceman should be so happy now.
Attachment #8926582 - Attachment is obsolete: true
Attachment #8926754 - Flags: review+
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 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 ;)
Attachment #8926754 - Flags: review+
Pushed by mozilla@jorgk.com:
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
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Blocks: 1629499
You need to log in before you can comment on or make changes to this bug.