459487 - OnItemIntPropertyChanged not fired with BiffState atom when IMAP + folder selected for offline use (No New Mail/Biff Notification)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

I noticed this just after checking in the growl code.

If you receive email on an imap account, and it is selected for offline use, then we don't get OnItemIntPropertyChanged being fired with BiffState (we do get other notifications).

Hence we don't get the biff notifications we're expecting.

I'm expecting this is cross-platform, but I'm reporting this on Mac.

Comment 1

[David :Bienvenu]

this is WFM - on windows, I get new mail alerts, and on the mac, I get a growl alert. On both machines, the imap inbox is selected for offline use.

Comment 2

[David :Bienvenu]

actually, I get the first alert, but after that, I don't get alerts - in my test case, I stay in the imap inbox and send myself new mail, read it, and reply to it, and the second mail doesn't cause a notification.  I wonder if the db is getting closed or something...

Comment 3

[David Humphrey (:humph)]

So I thought I'd caused this on Mac with my growl stuff, but further testing says it's not me.  However, I'm hitting it too and it sucks.

I did notice, while researching this, that in OnItemIntPropertyChanged I get "BiffState" only the first time new mail comes in, and subsequent mail only causes "FolderSize", "TotalMessages", and "TotalUnreadMessages".  When I go back to the app, the next new mail will in fact trigger "BiffState" again.  But the same process recurs if the app loses focus.

Comment 4

[David :Bienvenu]

now I'm finding this very hard to reproduce...

Comment 5

[David :Bienvenu]

I'm going to move this to b2, it's not nearly so common as I thought. It would be great to figure out how to reproduce this, and then fix it.

Comment 6

[David Humphrey (:humph)]

Unless something has changed in the last week that is affecting this, I hit it every time, such that I get BiffState only once per account until I clear it.  Is there something you want me to test for you?

Comment 7

[David :Bienvenu]

we don't send biff notifications if biff hasn't cleared - that's a separate but related issue...

Comment 8

[David Humphrey (:humph)]

After further discussion with David on irc, it's clear I'm not hitting this bug as such, but thinking of the way we do biff in general (i.e., once per account until cleared).  Comment 6 above is not right: I've never hit it.

Comment 9

[David Humphrey (:humph)]

Fixing some other bugs, I've confirmed I'm hitting this.  David and I spent the morning debugging and it seems to be related to how we set m_performingBiff in nsImapMailFolder.cpp, specifically, when it gets checked (twice) in HeaderFetchCompleted.  The second IF guarding the call to SetBiffState is the real problem, and m_performBiff being false is what keeps us out of there.

Further, I've found that it always manifests itself in this way: a single mail comes in, BiffState is not reported, a second mail comes in later, putting the recent mail count to 2, BiffState goes through.  I'm not sure how significant the count being >=+2 is, but I wonder if we're at -1 and go to 1 with the +2 count, causing it to flip.

This bug sucks and has caused me real headaches working on bug 465985.  I'd love to fix it, but I'm needing some tips.  David suggested that we could simply ignore it (i.e., remove both checks on m_performingBiff), but I've found that this causes way too many other non-biff-like imap protocol stuff to get through--it probably wouldn't be an issue with just BiffState, but with bug 465985 I'm adding subsequent atoms to go through, so this hurts.

David also suggested that we could keep track of the downloaded header count, and pass that into HeaderFetchComplete.  This could be used to get a better sense of whether we really have new mail or no.

Comment 10

[David Humphrey (:humph)]

> through--it probably wouldn't be an issue with just BiffState, but with bug
> 465985 I'm adding subsequent atoms to go through, so this hurts.

bug 465381, sorry.

Comment 11

[David :Bienvenu]

Attachment: possible fix - checked in.

thx to humph's persistence and hard work, we think what's going on here is that the imap offline autodownload stuff is occasionally calling UpdateFolder on the inbox, which is triggering header downloads w/o us having SetPerformingBiff to be true. So this fix basically says if a non UI task (i.e., no msg window) calls update folder on the inbox, then we pretend it's biff, so we'll get the notifications.

It may seem like an edge case, but people get really annoyed when biff doesn't fire :-)

Comment 12

[Mark Banner (:standard8)]

I've tried this a couple of times now and it doesn't seem to work for my case at least. I've been starting up TB, getting a mail with the button (which is actually biffing me as well, with and without the patch - is that right?), then clearing the biff (click on the dot to mark read) and then send myself new email and wait a few minutes.

The new email comes, but no biff notification.

I'm also starting to think that the autosync calling update folder will be annoying for those who set their server new mail update times to long periods and don't want to get mail in between times.

Comment 13

[David :Bienvenu]

autosync isn't supposed to update more frequently than the biff interval, which should ameliorate that issue.

Not sure why it didn't help for you...I wonder if autosync is passing in a msg window...

Comment 14

[David Humphrey (:humph)]

(In reply to comment #12)
> I've been starting up TB, getting a mail with the button (which is
> actually biffing me as well, with and without the patch - is that right?),
> then clearing the biff (click on the dot to mark read) and then send myself
> new email and wait a few minutes.
> 
> The new email comes, but no biff notification.

I've tried your pattern, and for me, it works with this patch in (i.e., second new mail causes biffstate).  I wonder what is different between our two cases?

I'm unable to reproduce the missed-biff case with this patch in place. 

David, can you comment on the question about whether clicking Get Mail should cause biff?

Comment 15

[David :Bienvenu]

Traditionally, it should not. Looking at the code, I'm surprised that it does.

Comment 16

[David Humphrey (:humph)]

(In reply to comment #15)
> Traditionally, it should not. Looking at the code, I'm surprised that it does.

I looked at this, and when we click Get Mail, we clear biff for selectedFolders[0] in GetNewMessages() (see http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#53).  Then, in nsImapFolder::GetNewMessages, we fill performingBiff using the server's value (root folder?), which is true when I debug.

In order to insure that we don't notify for this manual 'Get Mail' case, the folder's m_performingBiff has to be false (see http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#5121).  Are we misaligned with respect to inbox vs. root when we clear biff?

Comment 17

[Mark Banner (:standard8)]

bienvenu: ping for response to comment 16.

Comment 18

[David :Bienvenu]

>  Are we misaligned with respect to inbox vs. root when we clear biff?

That might explain why we're notifying on manual get new mail, but it's not clear to me that we shouldn't notify then. I don't know that it has to do with this bug, however.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 349501 [details] [diff] [review]
possible fix - checked in.

I'm a little bit dubious about taking this, as I can't see it fixing my issue, but it does seem the right thing to do. So lets get it in now and see what effect it has on things.

Comment 20

[David :Bienvenu]

Comment on attachment 349501 [details] [diff] [review]
possible fix - checked in.

changeset:   1453:500ee1026b79

yes, let's see if this helps.

Comment 21

[David :Bienvenu]

moving off to b3 - haven't got complaints about this in a while.

Comment 22

[David :Bienvenu]

has anyone seen this recently? I'm tempted to mark it WFM.

Comment 23

[David Humphrey (:humph)]

Your fix has fixed it for me.  I've never seen it again in my setup or testing.

Comment 24

[David :Bienvenu]

thx, humph - I'm going to mark this fixed, then. We can start a new bug if we figure out how to recreate what Mark saw...