24594 - Must restart the application for Biff / Check for new messages every x mins to work (or stop working)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Ninoschka Baca]

Build 2000012009M13: NT4
Haven't tried Linux or Mac yet.

Overview: For Biff to work I must restart Mail.

Steps to reproduce:
1. Open Mail to a POP or IMAP account
2. Open Account Settings (select Edit|Account Setup)
3. Select the server panel for the POP server
4. Select to "check for new mail ever" 1 "minutes"
5. Click OK to close Account Setup
6. Send a message to the POP account and wait 1 minute

Actual Results: Notice that the Biff icon never appears in the folder pane.
After exiting and restarting Mail then it works as expected.

Expected Results: After selecting to check messages after a certain number of 
minutes, then Bif should work without the need to restart the application.

Comment 1

[Ninoschka Baca]

Setting QA Contact to fenella.

Comment 2

[laurel]

*** Bug 28263 has been marked as a duplicate of this bug. ***

Comment 3

[Akkana Peck]

Even after restarting, I'm not seeing biff happening.  I have
user_pref("mail.server.server1.check_new_mail", true);
user_pref("mail.server.server1.check_time", 1);
but when I start mail, I wait and wait for new mail to come in, and finally get
suspicious and click GetMsgs, and fifteen messages come in.  Biff just isn't
working reliably.

Comment 4

[scottputterman]

are you logged on? Right now we don't  biff unless you are logged on.

Comment 5

[Akkana Peck]

Yes.  I start the app, I get the imap password dialog and type in my password, I
see my inbox and start reading messages, finish reading messages, and wait ...
and nothing more comes in.

If I manually do GetMsg a few times, then it seems to start biffing by itself,
but I almost never see new mail appear without having to hit GetMsg at least
once in a session first.

Comment 6

[scottputterman]

I think what akkana mentions was fixed last week by alec.  The bug that Fenella
filed is something that's never been hooked up yet.

Comment 7

[Akkana Peck]

Mail has been biffing fine for me this week, so I think the bug I mentioned has
been fixed.

Comment 8

[scottputterman]

reassigning to gayatrib.

Comment 9

[selmer (gone)]

Cleanup work, marking M18.

Comment 10

[lchiang]

Fenella - can you see if this is working now?

Comment 11

[fenella]

Linux (2000-07-11-08 M17)
Win32 (2000-07-11-09 M17)
Mac (2000-07-11-08 M17)
This problem has been fixed.

Comment 12

[fenella]

Verified it.

Comment 13

[scottputterman]

reopening. I don't think this has been fixed. You still have to restart to get 
changes.

Comment 14

[scottputterman]

*** Bug 52369 has been marked as a duplicate of this bug. ***

Comment 15

[Suresh]

*** Bug 55852 has been marked as a duplicate of this bug. ***

Comment 16

[Sheela Ravindran]

*** Bug 62305 has been marked as a duplicate of this bug. ***

Comment 17

[(not reading, please use [e-mail] instead)]

updating summary.  turning off biff doesn't seem to happen until I restart
either.  yikes.  I think we should bump up the priority on this and fix it for
the next release.

Comment 18

[jsnod]

CC-ing self.

Comment 19

[scottputterman]

*** Bug 60072 has been marked as a duplicate of this bug. ***

Comment 20

[scottputterman]

reassigning to racham.  marking nsbeta1+ and moving to mozilla0.9

Comment 21

[fenella]

Sheelar is the new QA now

Comment 22

[scottputterman]

marking nsbeta1- and moving to future milestone.

Comment 23

[racham]

*** Bug 60072 has been marked as a duplicate of this bug. ***

Comment 24

[lohphat]

2002040203 win32

Biff shows new mail, after mail is read biff doesn't go away.

I'm finding it hard to reproduce.  It seems to be a timing thing related to get
new mail on startup -- when mail is read during the pop-up notification might be
the casue.

Developing...

Comment 25

[grylchan]

*** Bug 134803 has been marked as a duplicate of this bug. ***

Comment 26

[grylchan]

*** Bug 130781 has been marked as a duplicate of this bug. ***

Comment 27

[grylchan]

nominating for nsbeta1

Comment 28

[Frank Wein [:mcsmurf]]

*** Bug 142156 has been marked as a duplicate of this bug. ***

Comment 29

[Navin Gupta]

Bhuvan, I can take this one if you are not working on it.

Comment 30

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 180305 has been marked as a duplicate of this bug. ***

Comment 31

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 166477 has been marked as a duplicate of this bug. ***

Comment 32

[scottputterman]

reassigning to sspitzer

Comment 33

[R.K.Aa.]

*** Bug 185782 has been marked as a duplicate of this bug. ***

Comment 34

[Yuliy Pisetsky]

Well now in 2003060409 (OS = Windows XP) I get the reverse effect: I DONT want
it to automatically check for mail but it does anyway

Comment 35

[R.K.Aa.]

*** Bug 267876 has been marked as a duplicate of this bug. ***

Comment 36

[David :Bienvenu]

*** Bug 137298 has been marked as a duplicate of this bug. ***

Comment 38

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 40

[Joel Maher ( :jmaher ) (UTC -8)]

I have verified this is still a problem with Thunderbird 2.0.0.17 on Windows XP.

Comment 41

[liuchongjin]

This bug is still existing in Thunderbird 2.0.0.19 on solaris

Comment 42

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #41)
> This bug is still existing in Thunderbird 2.0.0.19 on solaris

Not surprising, since it's still open.

Comment 45

[Valerio Messina]

on bug 536359 there some procedure to reproduce the bug

Comment 47

[alta88]

Attachment: biff.patch

Comment 48

[Marcin Orlowski]

This bug is opened for 16 years. Just a few more and my kids will have a chance to fix it :)

Comment 49

[alta88]

(In reply to Marcin Orlowski from comment #48)
> This bug is opened for 16 years. Just a few more and my kids will have a
> chance to fix it :)

If I had known for certain your kids would fix it, I would have let it ripen some more.. ;)

Comment 50

[alta88]

Comment on attachment 8801201 [details] [diff] [review]
biff.patch


whoever has time first.

Comment 51

[Jorg K (CEST = GMT+2)]

Comment on attachment 8801201 [details] [diff] [review]
biff.patch

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

As a reviewer you visit code you've never seen before and then you get to ask (silly) questions. First question: What does "biff" stand for?

Comment #0 says that when you change between checks, that's not obeyed without a restart. I can see that nsIMsgIncomingServer.idl has a few attributes, amongst them doBiff and biffMinutes. So the question is: Why don't you do this in SetBiffMinutes()? Another question below.

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +1394,5 @@
> +    do_GetService(NS_MSGBIFFMANAGER_CONTRACTID, &rv);
> +  if (NS_SUCCEEDED(rv) && biffService)
> +  {
> +    if (aDoBiff)
> +      (void) biffService->AddServerBiff(this);

Why is it OK to call "add" here? What happens of SetDoBiff(true) is called twice in succession? Or some add-on can do |doBiff = true;| twice. Or are you ignoring an "already added" error here? Shouldn't that be checked explicitly?

Comment 52

[alta88]

(In reply to Jorg K (GMT+2) from comment #51)
> Comment on attachment 8801201 [details] [diff] [review]
> biff.patch
> 
> Review of attachment 8801201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As a reviewer you visit code you've never seen before and then you get to
> ask (silly) questions. First question: What does "biff" stand for?
> 

yes, that tradeoff was well considered..  in this context, biff is an automatic poll for new messages from the server.  according to internet lore, it was the name of some unix guy's dog who barked when the mailman came.

> Comment #0 says that when you change between checks, that's not obeyed
> without a restart. I can see that nsIMsgIncomingServer.idl has a few
> attributes, amongst them doBiff and biffMinutes. So the question is: Why
> don't you do this in SetBiffMinutes()? Another question below.
> 

the timer of a switch has nothing to do with with the switch being on or off.

> ::: mailnews/base/util/nsMsgIncomingServer.cpp
> @@ +1394,5 @@
> > +    do_GetService(NS_MSGBIFFMANAGER_CONTRACTID, &rv);
> > +  if (NS_SUCCEEDED(rv) && biffService)
> > +  {
> > +    if (aDoBiff)
> > +      (void) biffService->AddServerBiff(this);
> 
> Why is it OK to call "add" here? What happens of SetDoBiff(true) is called
> twice in succession? Or some add-on can do |doBiff = true;| twice. Or are
> you ignoring an "already added" error here? Shouldn't that be checked
> explicitly?

no, it's explicitly not checked because AddServerBiff does the right thing and ignores servers already in the array.

this is set via the nsIMsgIncomingServer setter in account manager server settings checkbox, on closing the dialog.  the only question is whether a biff should be performed immediately, per reasonable user expectation on turning a switch 'on', or until the biffminutes interval expires.

Comment 53

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #52)
> the timer of a switch has nothing to do with with the switch being on or off.
Sorry, I misread comment #0. It's not changing the check interval, it's about switching the checking on.

> no, it's explicitly not checked because AddServerBiff does the right thing
> and ignores servers already in the array.
I don't agree with ignoring the return status here. If you look at nsMsgBiffManager::AddServerBiff() it ignores servers already in the array. There is however a path that could return an error, so it's not OK to ignore it. Equally the return code of nsMsgBiffManager::RemoveServerBiff() should be checked although it currently always returns OK.

> this is set via the nsIMsgIncomingServer setter in account manager server
> settings checkbox, on closing the dialog.  the only question is whether a
> biff should be performed immediately, per reasonable user expectation on
> turning a switch 'on', or until the biffminutes interval expires.
Yes, thanks for explaining this.

Comment 54

[Jorg K (CEST = GMT+2)]

In fact, you could add a comment like:
Adding/removing existing/non-existing server is handled without error.

Comment 55

[alta88]

(In reply to Jorg K (GMT+2) from comment #54)
> In fact, you could add a comment like:
> Adding/removing existing/non-existing server is handled without error.

that's all i will do, because it makes no sense to check, for either case.  it is completely ok to ignore it if there's nothing needing to be done about it.  the only other use, on startup, doesn't check either and people have been biffing for 20 years.

Comment 56

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #55)
> that's all i will do, because it makes no sense to check, for either case. 
> it is completely ok to ignore it if there's nothing needing to be done about
> it.
OK.

For ignoring the errors, M-C invented a new scheme to be seen here:
https://dxr.mozilla.org/comm-central/source/mozilla/accessible/ipc/other/ProxyAccessible.cpp#26

Comment 57

[alta88]

yeah i know about that, it's not necessary here instead of void, it's for overriding sensitive possibly crashy functions explicitly defined so that callers must get the rv. ie not these functions.

Comment 58

[Jorg K (CEST = GMT+2)]

Comment on attachment 8801201 [details] [diff] [review]
biff.patch

Well, Magnus asked me to use |Unused <<| here: Bug 1301706 comment #5 and further. So I'm not sure. If you don't want to check the return value and it's not mandatory to check, why use the |(void)| in the first place?

Comment 59

[Jorg K (CEST = GMT+2)]

I forgot to say: Let's see what Magnus thinks.

Comment 60

[alta88]

well, nsIURI.getSpec is one of the things MOZ_MUST_USE was designed for, and they will or have changed the idl for nsIURI.  again, not the level of these funcs.  void is to avoid warning spew at compile time, and a good practice to show intentional non check.

Comment 61

[Jorg K (CEST = GMT+2)]

Comment on attachment 8801201 [details] [diff] [review]
biff.patch

OK then. You're going to add that comment we talked about?

Comment 62

[alta88]

Attachment: biff.patch

udpated for comment.

Comment 63

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/652110fac32ecde08024215d58d45d1c950b69cd