Closed Bug 1367933 Opened 4 years ago Closed 4 years ago

Adaptive junk mail controls, address books getting unselected

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
normal

Tracking

(thunderbird_esr5254+ fixed, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 54+ fixed
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: u277459, Assigned: aceman)

Details

Attachments

(1 file, 1 obsolete file)

5.17 KB, patch
aceman
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170518000419

Steps to reproduce:

Account Settings: Junk Settings: Enable adaptive junk mail controls for this account.
I select all my address books under "Do not automatically mark mail as junk if the sender is in".


Actual results:

Sometime later (days, weeks), some of the address books are no longer selected in this setting.  They will always be a contiguous set of address books at the beginning or end of the alphabetic listing of the address books.

No changes are made to the account.  This happens with multiple accounts, but not the at the same time, or the same set of address books being unselected.


Expected results:

Well, not this.  Keep the settings without losing them.
Nuts.  I forgot to mention:

- That this has been going on for at least the past two years, with all the release versions of Thunderbird throughout that time.

- This has continued even after a couple of times creating whole new Profiles and reinstalling Thunderbird.

- I don't use CCleaner or any other program remove anything related to Thunderbird.  I used to use CCleaner to clean the cache, then stopped.
Component: Untriaged → Account Manager
That's weird. Do you use any addons/extensions in Thunderbird that may touch addressbooks?
ThunderBirthDay accesses one of the 19 address books.

MyPhoneExplorer reads & writes from/to all address books.  This add-on works with its same-name program to synchronize the address books between Thunderbird and my Android smart phone.  I did a synch just now, and it did not cause this issue.

MoreFunctionsForAddressBook I have disabled all the time, except for the rare times I need to remind myself how it works, for instructing others.
"Intermittent" bugs like this one - "Sometime later (days, weeks)" - are almost impossible to fix unless someone makes this reproducible. I was once chasing a bug (bug 1216914, Drafts folder corruption) for more than half a year. In the end, I was running a specially compiled debug version for some weeks that was watching that particular condition. In the end I found the problem, and once found, I could reproduce it predictably in seconds and we fixed it (one-line fix).

So in this case we'd need to compile a version that watches the deselection of those address books (after finding out how to do that). Then we need to compile a special debug version that does a "planned crash" when the condition occurs. At that point we need to attach the debugger and get a C++ stack and JS stack to identify the caller.

So unless someone in the community steps up to do this effort, or the reporter can somehow find the condition that causes the problem, there's little hope of fixing that. Read bug 1216914 for some light reading. I documented all my hassle well over there.
I appreciate the immediate attention that this bug has elicited.  I fear it will languish or die, being labelled as "un-reproduceable" or "unsolved" unless someone else says they also see this.

Over the time I've observed this bug on my system, I've tested all sorts of things to try to reproduce the effect intentionally.  I've never had any success in doing so.  The effect always happens when I'm NOT focusing on trying to diagnose it, and I don't notice it until later.  As a troubleshooter (and former programmer), myself, I know that this is a frustrating and a difficult thing to pin down.

I regret that I lack the time, motivation, and strength of will to do the things that friend, guru, and developer Jorg has suggested to track and troubleshoot this issue.  I know that it is up to me to do this, and I cannot.

I am a computer tech, pessimist, and realist.  My worst fear is that there is something systemic in how I do things that is causing this.  In that case, there is probably nothing anyone can do to help me, for I am already lost.

On the other hand, Thunderbird is the ONLY program that has continually caused me unpredictable problems (not just this one), over the last few years.  This is excusing Windows itself, which is always unpredictable.  So I can't help but to think that the faults are with Thunderbird.

This is a significant thing for ME to say.  I like Thunderbird, promote it, and support it through the support forum.  When someone in the support forum says such a thing as this, my automatic reaction is to discount such accusations.  I am now going to resist that reaction, but still try to help people with Thunderbird as best as I can.

Additional notes:
- The only thing I can think of that is uncommon about my setup is that I have 19 address books, which I assume is more than most people.  I can't imagine trying to manage all my different categories of contacts with only one address book, like the unorganized people do.  (...Shudder...)

- The issue has continued no matter what AV I have used, which is 4 different ones during the time I have seen this issue.  That's the only external thing to Thunderbird that might have any effect on it.  For all four to have this one specific effect, is rather unbelievable.
The list of addressbooks fr the junk filtering is stored in your profile in the file prefs.js as follows:
user_pref("mail.server.server<N>.whiteListAbURI", "values...")

server<N> is the id of the server in question. You say it happens on more accounts so this will vary.
Your first step could be to get an alert when the list changes. You may be able to setup a batchfile that starts TB and after it closes compare the previous and new prefs.js whether this pref has changed for any account.
If you immediately get and alert when it changes, you may remember what you did in that session.

Also, how do you get notification that the ABs got deselected? Does the junk classification stop working right? Or do you occasionally go into Account settings and then find ABs deselected? We need to know this exactly, whether some backend code does it, or the Account settings dialog itself.
Which backend code deselects those? We could inspect that.

There's also a bug at least in Trunk. If the junk settings are disabled, then the list entries are greyed out, however, if you scroll the list, non-greyed out ones move into view.
(In reply to Jorg K (GMT+2) from comment #7)
> Which backend code deselects those? We could inspect that.

Hopefully none, we can see where "whiteListAbURI" is touched in the code. But that was just an example. An addon may be doing it.

> There's also a bug at least in Trunk. If the junk settings are disabled,
> then the list entries are greyed out, however, if you scroll the list,
> non-greyed out ones move into view.

I can't observe that on Linux.
(In reply to :aceman from comment #8)
> > There's also a bug at least in Trunk. If the junk settings are disabled,
> > then the list entries are greyed out, however, if you scroll the list,
> > non-greyed out ones move into view.
> 
> I can't observe that on Linux.

Ok, I can see something. If I toggle disabling junk, the checkboxes of the scrolled out ABs look active. DOM Inspector sees, they do not get the disabled=true attribute.
(In reply to :aceman from comment #9)
> Ok, I can see something. If I toggle disabling junk, the checkboxes of the
> scrolled out ABs look active. DOM Inspector sees, they do not get the
> disabled=true attribute.

I have a fix for this one. Do you think the reported problem may have a similar cause (offscreen ABs (user said 19)) just do not get their checkboxes checked visually? That is why I wanted to debug whether some "backend" OR the account manager UI may do it.
(In reply to :aceman from comment #10)
> just do not get their checkboxes checked visually?
They don't? The scroll into view checked, as they should, just not greyed out (if the SPAM feature is turned off).

I've looked at https://dxr.mozilla.org/comm-central/search?q=whiteListAbURI&redirect=false.
As far as I can see, that is never set, other than in the AM.

I'm discounting this one |rv = SetWhiteListAbURI(whiteListAbURI.get());| in nsSpamSettings.cpp which is a "shuffling around". Of course we have |attribute string whiteListAbURI;|, so it's fair game for any add-on to manipulate this.

I've searched add-ons, only one manipulates this:
addons/2533/chrome/AddressbooksSync/content/addressbookssync.js
(In reply to Jorg K (GMT+2) from comment #11)
> (In reply to :aceman from comment #10)
> > just do not get their checkboxes checked visually?
> They don't?

I'm asking if that is your theory for the reporter's problem.

>The scroll into view checked, as they should, just not greyed
> out (if the SPAM feature is turned off).

For me only the checkboxes are not greyed out, the labels are. But that may be a theme difference. The disabled attribute is not set on the offscreen items. I can work around it. There are known problems in XUL like that, see e.g. bug 448582.

> I've looked at
> https://dxr.mozilla.org/comm-central/search?q=whiteListAbURI&redirect=false.
> As far as I can see, that is never set, other than in the AM.
> 
> I'm discounting this one |rv = SetWhiteListAbURI(whiteListAbURI.get());| in
> nsSpamSettings.cpp which is a "shuffling around".

Yes.
I find out that they have been de-selected either by the junk classification not working right (e-mail from someone in the de-selected address book gets marked as junk), but more often I discover this because I've gotten paranoid about the issue, and check the settings.

It just happened again between yesterday and today.  The last I checked yesterday, it was OK.

The address books that were still selected were all the address books that were in the current view.  ALL the de-selected ones were the "scrolled out" address books.  So the cause may be similar to the other bug mentioned.

I created batch files to do a lot of things, but never to analyze file contents.  The best I can do is create a batch file that will backup "prefs.js", then another batch file to run later to trigger ExamDiff to pop up comparing the backup to the current file.  Then I will have to scroll through to see the differences.
It just happened again, but this time only two address books were de-selected at the end of the list.  There were more above them that had been "scrolled out", but they were still selected.

I now have a batch file that backups up the "prefs.js" file, runs Thunderbird, then triggers ExamDiff when Thunderbird closes.  I suppose I will have to keep notes of what I do in every Thunderbird session.
Thanks. Please try to find out if the whitelist preference changes without you opening the Account settings in the session.
That's my plan.
I can now reproduce this myself. With a lot of ABs (like 20) just entering the Junk setting panel and quickly clicking OK produces the error. The .checked state of the listitems (ABs) is not read back properly, and there is even an error in the Error console that .checked property does not exist. Which means the menulist binding (behaviour) wasn't yet bound to the offscreen elements. This is an optimization in XUL, but has already caused some problems in the past if forgotten.
I think I can now fix this.
Thanks again to Jorg for finding the right clue :)
Assignee: nobody → acelists
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Version: 52 Branch → 52
Great teamwork here and fantastic turnaround time!
Attached patch patch (obsolete) — Splinter Review
This works for me. Jorg please test if it fixes your problem. If you are not happy reviewing these XUL games, you do not need to do it :)
Attachment #8872101 - Flags: feedback?(jorgk)
I just reproduced it that way also.  See the Junk settings for the account and click OK.  I guess I didn't pay close enough to my troubleshooting before to see this.  My bad.

From now on, I will click Cancel instead.  It's not like I'm making changes.  I usually go there to remind myself of the options when I am advising people.
Comment on attachment 8872101 [details] [diff] [review]
patch

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

Looks good to me and works. I've only tested the "scroll into view" issue. We'll get Bruce to test is on Daily after landing.

::: mailnews/base/prefs/content/am-junk.js
@@ +60,3 @@
>    // Ensure the whitelist is empty
> +  for (let i = wList.itemCount - 1; i >= 0; i--) {
> +    wList.removeItemAt(i);

Please add comment why we're doing it in the clumsy way.

@@ +74,5 @@
>    }
>  
>    // Sort the list
>    function sortFunc(a, b) {
> +    return a.label.toLowerCase().localeCompare(b.label.toLowerCase());

This looks like a miscellaneous "boy scout" change thrown in for good measure ;-)

@@ +202,5 @@
>  
>    for (var i = 0; i < wList.getRowCount(); i++)
>    {
> +    // Due to bug 448582, do not trust any properties of the listitems
> +    // as they may not return the right value or can even not exist.

Nit: ... may even not exist.
Attachment #8872101 - Flags: feedback?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #21)
> ::: mailnews/base/prefs/content/am-junk.js
> @@ +60,3 @@
> >    // Ensure the whitelist is empty
> > +  for (let i = wList.itemCount - 1; i >= 0; i--) {
> > +    wList.removeItemAt(i);
> 
> Please add comment why we're doing it in the clumsy way.

This is not clumsy but the correct way. See bug 1075883. I consider tearing away elements from below a XUL element via direct DOM manipulation the invalid way that can only cause problems like this one. Often it doesn't, but we should not rely on the internal representation of the element. That is why I also rewrote the populating of the listbox later on.
 
> @@ +74,5 @@
> >    }
> >  
> >    // Sort the list
> >    function sortFunc(a, b) {
> > +    return a.label.toLowerCase().localeCompare(b.label.toLowerCase());
> 
> This looks like a miscellaneous "boy scout" change thrown in for good
> measure ;-)

Yeah, this is true :) I felt like fixing the sorting for localized builds, like the other sorting in the file does :)
(In reply to Jorg K (GMT+2) from comment #21)
> >    // Ensure the whitelist is empty
> > +  for (let i = wList.itemCount - 1; i >= 0; i--) {
> > +    wList.removeItemAt(i);
> 
> Please add comment why we're doing it in the clumsy way.

And it looks ugly because listbox does not implement .removeAllItems() method, like e.g. menulist does.
Attached patch patch v1.1Splinter Review
Attachment #8872101 - Attachment is obsolete: true
Attachment #8872147 - Flags: review+
Ready to checkin, right? What's happening to bug 1075883? Neil didn't like the thing you're now doing here.
Neil didn't explain what the problem is, maybe he just wanted to get that loop into the base XUL implementations (as removeAllItems for listbox).

So you decide ;)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b8d187a8af7e03716ec91c4bff5b2f73905b4051
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Comment on attachment 8872147 [details] [diff] [review]
patch v1.1

[Approval Request Comment]
Regression caused by (bug #): Never worked, I assume :-(
Attachment #8872147 - Flags: approval-comm-esr52?
Attachment #8872147 - Flags: approval-comm-beta+
Attachment #8872147 - Flags: approval-comm-esr52? → approval-comm-esr52+
You need to log in before you can comment on or make changes to this bug.