Closed Bug 1399627 Opened 2 years ago Closed 2 years ago

nsTextFormatter breakage coming with bug 1388789

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: tromey, Assigned: jorgk-bmo)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1388789 is going to change nsTextFormatter.  This will break
thunderbird, because vssprintf is no longer going to be available.

I found the uses here:

https://dxr.mozilla.org/comm-central/search?q=nsTextFormatter%3A%3Avs&redirect=false

The only ones are in nsExplainErrorDetails.

Examining the callers of that, most of the calls are made with one
or two string arguments.  A couple of calls are made with integers.

So one workaround would be to convert those ints to strings and
then change nsExplainErrorDetails to accept strings, like nsStringBundle:

https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/intl/strres/nsStringBundle.cpp#292

There may be other options as well.
Thanks for the heads-up. So we only have two calls we need to fix, right?

mailnews/compose/src/nsSmtpProtocol.cpp
107 nsTextFormatter::vssprintf(msg, eMsg.get(), args);
121 nsTextFormatter::vssprintf(msg, eMsg.get(), args);
Summary: nsTextFormatter breakage coming with bug 13388789 → nsTextFormatter breakage coming with bug 1388789
(In reply to Jorg K (GMT+2) from comment #1)
> Thanks for the heads-up. So we only have two calls we need to fix, right?
> 
> mailnews/compose/src/nsSmtpProtocol.cpp
> 107 nsTextFormatter::vssprintf(msg, eMsg.get(), args);
> 121 nsTextFormatter::vssprintf(msg, eMsg.get(), args);

Yes, though my proposed fix would push some changes out to a couple of callers of nsExplainErrorDetails.
Tom, when are you going to land bug 1388789? You have all the reviews. You're not waiting for me, are you?
Flags: needinfo?(ttromey)
Attached patch 1399627-vssprintf.patch (v1) (obsolete) — Splinter Review
Hi, it's me again. I'm not so familiar with those nsTextFormatter functions. How does this look?
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8909037 - Flags: feedback?(ttromey)
(In reply to Jorg K (GMT+2) from comment #3)
> Tom, when are you going to land bug 1388789? You have all the reviews.
> You're not waiting for me, are you?

Not waiting for you -- rather stuck in template hell trying to get it to compile everywhere.
Flags: needinfo?(ttromey)
Comment on attachment 8909037 [details] [diff] [review]
1399627-vssprintf.patch (v1)

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

Looks reasonable to me.  Thank you for doing this.
Attachment #8909037 - Flags: feedback?(ttromey) → feedback+
Comment on attachment 8909037 [details] [diff] [review]
1399627-vssprintf.patch (v1)

Review now or get it landed as bustage fix ;-)
Attachment #8909037 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e579f2ee9cac
Port bug 1388789 to mailnews: Remove use of nsTextFormatter::vssprintf(). f=tromey rs=bustage-fix
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
Sadly that gave bustage:
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsTextFormatter.h(65): error C2440: '<function-style-cast>': cannot convert from 'nsresult' to 'nsTextFormatter::BoxedValue' [log…]
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsTextFormatter.h(65): error C2466: cannot allocate an array of constant size 0 [log…]
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsTextFormatter.h(65): error C2512: 'nsTextFormatter::BoxedValue': no appropriate default constructor available [log…] 
It compiled fine without the changes from bug 1388789, so has nsTextFormatter::ssprintf() also changed now?
Flags: needinfo?(ttromey)
One thing that's wrong I can see already is that NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_1/smtpPermSizeExceeded1 is:
smtpPermSizeExceeded1=The size of the message you are trying to send exceeds the global size limit (%d bytes) of the server. The message was not sent; reduce the message size and try again.
And that has a %d which was changed to a string. I'd have to change the string ID to get that retranslated.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b9989a1c89ec
Backed out changeset e579f2ee9cac for causing bustage. a=backout
https://hg.mozilla.org/comm-central/rev/a6f8c809488f
temporarily remove calls to nsTextFormatter::vssprintf(). rs=bustage-fix
I got it now, the error is here:
nsTextFormatter::ssprintf(msg, eMsg.get(), aCode);
where aCode is an nsresult.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5bfcd3d8d9f0
Backed out changeset a6f8c809488f (bug 1399627, temporary fix). a=backout
https://hg.mozilla.org/comm-central/rev/3357fc962557
Port bug 1388789 to mailnews: Remove use of nsTextFormatter::vssprintf() - take 2. f=tromey rs=bustage-fix
OK, this compiles now. A little hacky to convert the string back to int, but still better than changing the string ID for smtpPermSizeExceeded1.
Attachment #8909037 - Attachment is obsolete: true
Attachment #8909037 - Flags: review?(acelists)
Flags: needinfo?(ttromey)
Comment on attachment 8910541 [details] [diff] [review]
1399627-vssprintf.patch (v2) [landed comment #13]

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

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +116,5 @@
>      case NS_ERROR_SMTP_GREETING:
>        exitString = errorStringNameForErrorCode(aCode);
>        bundle->GetStringFromName(exitString, eMsg);
> +      if (aCode == NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_1)
> +        nsTextFormatter::ssprintf(msg, eMsg.get(), atoi(arg1), arg2);

So if now all arguments are to be converted to string, I would have preferred there be no exception for a specific string. Somebody will wonder later why NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_1 can use %d but it won't work in another string.

@@ +1903,5 @@
>    if((m_responseCode/10 != 25))
>    {
>      mozilla::DebugOnly<nsresult> rv = nsExplainErrorDetails(m_runningURL,
>                                                              NS_ERROR_SENDING_MESSAGE,
> +                                                            m_responseText.get(), nullptr);

Are these second arguments needed? In the other bug we didn't need to specify optional arguments and it still worked.
(In reply to :aceman from comment #15)
> So if now all arguments are to be converted to string, I would have
> preferred there be no exception for a specific string. Somebody will wonder
> later why NS_ERROR_SMTP_PERM_SIZE_EXCEEDED_1 can use %d but it won't work in
> another string.
Sure, but as explained in comment #10, the %d is in an error string and I'd have to change the string ID to change that to %S to get it retranslated. That's a high price to pay for this mainly dormant code.

> > +        m_responseText.get(), nullptr);
> Are these second arguments needed? In the other bug we didn't need to
> specify optional arguments and it still worked.
Of course, the function has two fixed arguments now, 2x char*. The va_args stuff is gone. Which other bug?
I get it, make those parameters optional in the header file. That would be possible. I'm not a great friend of that really.
(In reply to Jorg K (GMT+2) from comment #16)
> Sure, but as explained in comment #10, the %d is in an error string and I'd
> have to change the string ID to change that to %S to get it retranslated.
> That's a high price to pay for this mainly dormant code.

So maybe add some comments to the code and the string that when it gets changed, the %d should be changed to %S and the specialcasing removed :)

(In reply to Jorg K (GMT+2) from comment #17)
> I get it, make those parameters optional in the header file. That would be
> possible. I'm not a great friend of that really.

Why? How does the declaration look in c++?
Because it hides errors. The nsIInputStreamPump::Init changes were hidden because other call arguments just shuffled forward.

It's like JS, you can go |char* xx = nullptr| in the header file and then that parameter is optional.
Comment on attachment 8910541 [details] [diff] [review]
1399627-vssprintf.patch (v2) [landed comment #13]

OK, I'll add a comment:

// Convert the error argument back to integer since the error
// message contains a %d.
Attachment #8910541 - Attachment description: 1399627-vssprintf.patch (v2) → 1399627-vssprintf.patch (v2) [landed comment #13]
Like this? I even mentioned the name of the string so there is hoping that if someone changes that, it will be found be DXR.
Attachment #8914637 - Flags: feedback?(acelists)
Yes, I saw the trailing spaces now, I'll remove them.
NO, I meant the other way round. That // XXX the %d placeholder in the string should be changed to %S at the next string change and this special casing of the string ID removed.
I know. But the comment explains what is done here, so the logical consequence is to remove the spacial casing should the string ever change. How about:

// Convert the error message argument back to integer since the error
// message string smtpPermSizeExceeded1 contains a %d.
// (Special case can be removed if that string ever changes, then
// %d should be changed to %S.)
Thanks, better :)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cf664b8070cf
Follow-up: add comment. rs=comment-only DONTBUILD
Comment on attachment 8914637 [details] [diff] [review]
1399627-follow-up-comment.patch

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

But with the comment fix as landed.
Attachment #8914637 - Flags: feedback?(acelists) → review+
Attachment #8910541 - Flags: review+
You need to log in before you can comment on or make changes to this bug.