Closed Bug 946669 Opened 10 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
normal

Tracking

(thunderbird28 fixed, thunderbird_esr2428+ fixed)

RESOLVED FIXED
Thunderbird 29.0
Tracking Status
thunderbird28 --- fixed
thunderbird_esr24 28+ fixed

People

(Reporter: stefan_gies, Assigned: aceman)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Attached image 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.
Attached image the-beginning.JPG
Attached image the_end.JPG
This error is also valid in Seamonkey v2.22.1 and v2.23 Beta 1 Build 1
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
The error is valid on both platforms, 32bit and 64bit. Same as seamonkey.
I tested Thunderbird v17.0.11 ESR and Thunderbird v24.1.1. Thunderbird v24.1.1 has the problem I reported.
TB 17 does not have it?
TB 17 have not this problem.
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
Sorry, I meant bug 847172.
Blocks: 847172
Attached patch patch (obsolete) — Splinter Review
Attachment #8345469 - Flags: review?(iann_bugzilla)
Status: NEW → ASSIGNED
Version: 26 → 22
The error is also valid in Thunderbird v27.00 Beta 1 Build 2
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)
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 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+
(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 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+
Attached patch patch v2Splinter Review
Thanks, fixed all nits.
Attachment #8345469 - Attachment is obsolete: true
Attachment #8362654 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f28b79f5b46f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
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+
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.