1675037 - Invalid argument list passed to the nsILDAPMessage.getValues method (in the AbLDAPAttributeMap.jsm module).

Status: RESOLVED FIXED

(Thunderbird :: Address Book, task)

Description

[Chris GAUDRY [:cmgaudry33]]

Attachment: thunderbird_78.4.0_AbLDAPAttributeMap.jsm.patch

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.111 Safari/537.36

Steps to reproduce:

Nothing other then performing a search for all method calls in the javascript source code.

Actual results:

Nothing special, but this is a defect created when changing the signature of the nsILDAPMessage.getValues ​​method.

Expected results:

Nothing special

Comment 1

[Magnus Melin [:mkmelin]]

From bug 1562157?
How does the bug show itself in Thunderbird? Which version are you using?

Comment 2

[Chris GAUDRY [:cmgaudry33]]

I'm working on Thunderbird 78.4.0 (and 78.4.1 for now).
I'm had written en module which deals with LDAP. The API has been changed so I had to change this module.
But I saw that a wrong call still exists in the code. So I've submitted this patch.

Comment 3

[Chris GAUDRY [:cmgaudry33]]

Attachment: bug_1675037_AbLDAPAttributeMap.jsm.diff

Comment 4

[Ben Campbell]

Comment on attachment 9186671 [details] [diff] [review]
bug_1675037_AbLDAPAttributeMap.jsm.diff

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

LGTM. Looks like an old-style call leftover from nsIArray-removal. Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4700ffb0e14e
Fix Invalid argument list passed to the nsILDAPMessage.getValues method (in the AbLDAPAttributeMap.jsm module). r=benc

Comment 6

[Chris GAUDRY [:cmgaudry33]]

Hi,

For your information, this bug is still there in Thunderbird 78.4.3. It has been fixed in the daily branch only.

Thanks in advance.

Chris

Comment 7

[Magnus Melin [:mkmelin]]

AH, this is from https://hg.mozilla.org/comm-central/rev/28e58c1b88e1 which was v 71 already.

We use the status + target milestone for trunk, then the flags for tracking uplifts to branches.

But this additional parameter is just ignored, right? So it's just a matter of code cleanness.

Comment 8

[Thomas D. (:thomas8)]

Comment on attachment 9186671 [details] [diff] [review]
bug_1675037_AbLDAPAttributeMap.jsm.diff

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

Review should have requested changing `var` to `let`.

::: mailnews/addrbook/src/AbLDAPAttributeMap.jsm
@@ +132,5 @@
>  
>          // find the first attr that exists in this message
>          if (msgAttrs.includes(attr)) {
>            try {
> +            var values = aMessage.getValues(attr);

Next time, can we please change `var` to `let` when touching old code? Leaking block-scope variables into function scope isn't nice and can be hazardous.

Comment 9

[Thomas D. (:thomas8)]

FYI.

(In reply to Thomas D. (:thomas8) from comment #8)

Review of attachment 9186671 [details] [diff] [review] ⧉[details] [diff] [review]:

Review should have requested changing var to let.

::: mailnews/addrbook/src/AbLDAPAttributeMap.jsm
@@ +132,5 @@

         // find the first attr that exists in this message
         if (msgAttrs.includes(attr)) {
           try {
+            var values = aMessage.getValues(attr);

Next time, can we please change var to let when touching old code?
Leaking block-scope variables into function scope isn't nice and can be
hazardous.

Comment 10

[Magnus Melin [:mkmelin]]

Not the end of the day...

Comment 11

[Thomas D. (:thomas8)]

[Let's change var to let where we touch it, to proactively avoid scope problems]

(In reply to Magnus Melin [:mkmelin] from comment #10)

Not the end of the day...

Assuming you mean "not the end of the world" - sure, I never said that.

But I think it's important to alert a new contributor to this fact, and ideally this should not escape a one-line review. It looks trivial now, and it's most likely just a trivial oversight, but in the future, this type of oversight may cause creepy and hard-to-find scope bugs if nearby spots get changed and happen to use the same variables. I have no easy way of telling the degree of awareness on this issue. So I think pro-actively pointing this out to the relevant parties in a team spirit is better than being burnt later when it happens again in more relevant spots.

So the needinfo is just to make sure that this gets noticed in the flood of bug mail, and does not carry any implications of severity nor any accusations. If everyone is fully aware, great - they can just cancel the needinfo saying "thanks, of course!". It's really just part of QA, if you so wish, and taking responsibility for code quality as a team. Details matter.

Comment 12

[Thomas D. (:thomas8)]

Hi Chris, thank you for improving our code. Code quality is crucial.

Let me flesh this out for you a bit.

(In reply to Magnus Melin [:mkmelin] from comment #7)

AH, this is from https://hg.mozilla.org/comm-central/rev/28e58c1b88e1 which was v 71 already.

That's where the patch for bug 1562157 was landed, which probably overlooked this detail per comment 1 and comment 4.
As this "error in code" does not affect the application behaviour like a regular "defect", we can consider it a mere code refactoring: I have changed the type of this bug from "defect" to "task".

We use the status + target milestone for trunk, then the flags for tracking uplifts to branches.

Thunderbird release channels:

• Daily -> Beta -> Release

Code changes first land on "Trunk" from which Daily release is created, well, daily... ;-)
They then ride the train according to the Rapid Release Calendar (that's Firefox, but TB is roughly following that).

You can see from the first row of Past Branch Dates table that as of today, for Thunderbird:

• Trunk/Daily is 84 (FF Central, for TB: comm-central)
• Beta is 83
• Release is 82. Thunderbird uses the ESR cycle so we stick to 78 and push point releases, currently on 78.4.3 (Firefox 78.4).

So your patch here was landed on Trunk and automatically picked up by Daily, hence Target Milestone: 84 Branch (first landing).
Then there are tracking flags on the bug like tracking?thunderbird83 to request tracking, and determine "affected" or "wontfix".
For important fixes, uplifts can be requested (after landing) on the patch itself (attachment 9186671 [details] [diff] [review]), via Details > Edit details. > approval-comm-beta? | approval-comm-release? (that's probably mailnews core) | approval-comm-esr78?.

But this additional parameter is just ignored, right? So it's just a matter of code cleanness.

This bug was set tracking?thunderbird83: wontfix so release drivers have decided uplifting to beta (and release) isn't needed.
So your patch will ride the trains as per above, see Future branch dates table in the release calender:

• now: Trunk/Daily 84 (as seen in: past branch dates).
• merge date 2020-11-16 (tentative): Beta 84
• merge date 2020-12-14 (tentative): Release 84, which will be branded as ESR TB 78.6 (as seen in that table, ESR: Firefox 78.6).

Hth, and hope that I got that right (any errors, please correct me). ;-)

Comment 13

[Chris GAUDRY [:cmgaudry33]]

Hi Thomas,

I felt that my fixes weren't meant to deeply modify the existing code, but only to flag the problem to make it easier to understand and fix.
For example, the patch on AddrBookMailingList, I was inspired by the code implemented in AddrBookCard, to be consistent. My goal was not to refactor.
Anyway, I'll try to do my best by doing what you asked me about using let instead of var.

Comment 14

[Chris GAUDRY [:cmgaudry33]]

I also understand that I must be less impatient ;-)

Comment 15

[Ben Campbell]

(In reply to Thomas D. (:thomas8) from comment #8)

Review should have requested changing var to let.

Yep - I should have recommended that. In general, that's the way we've been going, I just missed that one.

Comment 16

[Chris GAUDRY [:cmgaudry33]]

Missing from 78.5.1 but it should be fixed in 78 too.

Comment 17

[Magnus Melin [:mkmelin]]

For requesting uplifts, please set the approval-comm-esr78 flag (to ?)

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9186671 [details] [diff] [review]
bug_1675037_AbLDAPAttributeMap.jsm.diff

[Approval Request Comment]
Code correctness patch. No risk.

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9186671 [details] [diff] [review]
bug_1675037_AbLDAPAttributeMap.jsm.diff

[Triage Comment]
Approved for esr78

Comment 20

[Rob Lemley [:rjl]]

Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/6b41eee12f37