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)
Tracking
(thunderbird28 fixed, thunderbird_esr2428+ fixed)
RESOLVED
FIXED
Thunderbird 29.0
People
(Reporter: stefan_gies, Assigned: aceman)
References
Details
(Keywords: regression)
Attachments
(4 files, 1 obsolete file)
114.90 KB,
image/png
|
Details | |
2.22 MB,
image/jpeg
|
Details | |
4.38 MB,
image/jpeg
|
Details | |
2.20 KB,
patch
|
aceman
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-esr24+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Reporter | ||
Comment 3•10 years ago
|
||
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
Reporter | ||
Comment 5•10 years ago
|
||
The error is valid on both platforms, 32bit and 64bit. Same as seamonkey.
Reporter | ||
Comment 6•10 years ago
|
||
I tested Thunderbird v17.0.11 ESR and Thunderbird v24.1.1. Thunderbird v24.1.1 has the problem I reported.
Reporter | ||
Comment 8•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8345469 -
Flags: review?(iann_bugzilla)
Reporter | ||
Comment 12•10 years ago
|
||
The error is also valid in Thunderbird v27.00 Beta 1 Build 2
Assignee | ||
Comment 13•10 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)
Comment 15•10 years ago
|
||
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•10 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•10 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•10 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•10 years ago
|
||
Thanks, fixed all nits.
Attachment #8345469 -
Attachment is obsolete: true
Attachment #8362654 -
Flags: review+
Keywords: checkin-needed
Comment 20•10 years ago
|
||
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
Assignee | ||
Comment 21•10 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 22•10 years ago
|
||
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+
Updated•10 years ago
|
tracking-thunderbird_esr24:
--- → 28+
Comment 23•10 years ago
|
||
https://hg.mozilla.org/releases/comm-aurora/rev/c5f88c5a9ef1
status-thunderbird28:
--- → fixed
Updated•10 years ago
|
Attachment #8362654 -
Flags: approval-comm-esr24? → approval-comm-esr24+
Comment 24•10 years ago
|
||
https://hg.mozilla.org/releases/comm-esr24/rev/93ad0f9b2190
status-thunderbird_esr24:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•