"trust junk mail headers set by" dropdown is accumulating items when switching between junk settings of different accounts

RESOLVED FIXED in Thunderbird 29.0

Status

MailNews Core
Account Manager
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Stefan Gies, Assigned: aceman)

Tracking

({regression})

Thunderbird 29.0
regression

Thunderbird Tracking Flags

(thunderbird28 fixed, thunderbird_esr2428+ fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

114.90 KB, image/png
Details
2.22 MB, image/jpeg
Details
4.38 MB, image/jpeg
Details
2.20 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 8343022 [details]
error.png

User Agent: Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 SeaMonkey/2.23 (Beta/Release)
Build ID: 20131203184303

Steps to reproduce:

Add three or more mailaccounts to thunderbird/seamonkey. Then change to mail & newsgroup settings. Then jump from the first mailaccount to the junkmail-settings. Using the dropdwonmenu changes the content to eg. bogofilter. Jump to the next junkmail-settings of the second account and so on. Without touching any other settings than the dropdownmenu, the menu adds the previous spamfilter. 


Actual results:

At the end of four changes there will be more than ten entries in the dropdownmenue. 


Expected results:

The default quantity of entries do not change.
(Reporter)

Comment 1

5 years ago
Created attachment 8343136 [details]
the-beginning.JPG
(Reporter)

Comment 2

5 years ago
Created attachment 8343137 [details]
the_end.JPG
(Reporter)

Comment 3

5 years ago
This error is also valid in Seamonkey v2.22.1 and v2.23 Beta 1 Build 1
(Assignee)

Comment 4

5 years ago
Thanks for spotting this.
Confirming and taking.
Assignee: nobody → acelists
Status: UNCONFIRMED → NEW
Component: Untriaged → Account Manager
Ever confirmed: true
OS: Linux → All
Product: Thunderbird → MailNews Core
Hardware: x86 → All
Summary: Buffer error at junk-mail settings → "trust junk mail headers set by" dropdown is accumulating items when switching between junk settings of different accounts
(Reporter)

Comment 5

5 years ago
The error is valid on both platforms, 32bit and 64bit. Same as seamonkey.
(Reporter)

Comment 6

5 years ago
I tested Thunderbird v17.0.11 ESR and Thunderbird v24.1.1. Thunderbird v24.1.1 has the problem I reported.
(Assignee)

Comment 7

5 years ago
TB 17 does not have it?
(Reporter)

Comment 8

5 years ago
TB 17 have not this problem.
(Assignee)

Comment 9

5 years ago
Looks like fallout from bug 730290:
-      if (!aISPHeaderList.querySelector('[value="'+ fileName + '"]'))
-        aISPHeaderList.appendItem(fileName, fileName);
+      // If we've already added an item with this name, then don't add it again.
+      if (aExistingEntries.indexOf(fileName) == -1)
+        newEntries.push(fileName);

Previously, we checked if entries are already in the real dropdown widget before adding new. So if nothing new was found, no change was done. That seems to have had a rare edge case bug that if files were actually removed from filesystem, the entry was not removed from the widget.

The new code constructs the list of entries in a new array and only then adds them to the widget. It should instead replace the whole list with the new one.
Blocks: 730290
Keywords: regression
No longer blocks: 730290
(Assignee)

Comment 10

5 years ago
Sorry, I meant bug 847172.
Blocks: 847172
(Assignee)

Comment 11

5 years ago
Created attachment 8345469 [details] [diff] [review]
patch
Attachment #8345469 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Version: 26 → 22
(Reporter)

Comment 12

5 years ago
The error is also valid in Thunderbird v27.00 Beta 1 Build 2
(Assignee)

Comment 13

4 years ago
Comment on attachment 8345469 [details] [diff] [review]
patch

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

Not sure if Ian gets to this any time soon.
Attachment #8345469 - Flags: review?(neil)
Attachment #8345469 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 960292
Comment on attachment 8345469 [details] [diff] [review]
patch

The code looks lonely there, a better place would be just after initialising ispHeaderList, thus avoiding splitting up the code that builds the entries.

I see earlier in that file lastChild is used in another if test although in other places hasChildNodes() is used instead.
Attachment #8345469 - Flags: review?(neil) → review+

Comment 16

4 years ago
Comment on attachment 8345469 [details] [diff] [review]
patch

>+  // Ensure the menulist is empty
Nit: should end with . and if you move the code as suggested you could also add a . to the comment there.
>+  while (ispHeaderList.lastChild)
>+    ispHeaderList.removeChild(ispHeaderList.lastChild);
>+
Attachment #8345469 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 17

4 years ago
(In reply to neil@parkwaycc.co.uk from comment #15)
> The code looks lonely there, a better place would be just after initialising
> ispHeaderList, thus avoiding splitting up the code that builds the entries.
> 
> I see earlier in that file lastChild is used in another if test although in
> other places hasChildNodes() is used instead.
So which one is better?

Comment 18

4 years ago
Comment on attachment 8345469 [details] [diff] [review]
patch

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

LGTM

::: mailnews/base/prefs/content/am-junk.js
@@ +260,5 @@
>    }
>  
> +  // Ensure the menulist is empty
> +  while (ispHeaderList.lastChild)
> +    ispHeaderList.removeChild(ispHeaderList.lastChild);

I think whiles should always have braces
Attachment #8345469 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 19

4 years ago
Created attachment 8362654 [details] [diff] [review]
patch v2

Thanks, fixed all nits.
Attachment #8345469 - Attachment is obsolete: true
Attachment #8362654 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f28b79f5b46f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
(Assignee)

Comment 21

4 years ago
Comment on attachment 8362654 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 847172
User impact if declined: user wondering if the feature is actually working and which menuitem (spamfilter) to select
Testing completed (on c-c, etc.): trunk
Risk to taking this patch (and alternatives if risky): probably tiny
Attachment #8362654 - Flags: approval-comm-esr24?
Comment on attachment 8362654 [details] [diff] [review]
patch v2

[Triage Comment]
Lets get this onto aurora so we get beta testing next cycle
Attachment #8362654 - Flags: approval-comm-aurora+
tracking-thunderbird_esr24: --- → 28+
Attachment #8362654 - Flags: approval-comm-esr24? → approval-comm-esr24+
You need to log in before you can comment on or make changes to this bug.