548584 - IMAP Condstore server results in wrong folder size

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Nico Haase]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.1.8) Gecko/20100216 Lightning/1.0b1 Thunderbird/3.0.2

Even if the bug with condstore resulting in unexpected unread mails is fixed, the size of a message folder is still wrong calculated. 

Reproducible: Always

Steps to Reproduce:
1. ensure that condstore is enabled
2. enable the "size"-column in the folder column on the left
3. enter a folder
Actual Results:  
Wrong foldersize is shown: at my accounts often under 10 KB even if there are more than 3.000 mails in the account with huge attachments

Expected Results:  
Actual foldersize after having condstore disabled once more: about 60 MB

Thunderbird 3.0.2 connected to Dovecot 1.2.10

Comment 1

[David :Bienvenu]

Ah, yeah - thx for filing this. No one has filed it before, probably because you need an extension to see the folder size.

Comment 2

[David :Bienvenu]

Attachment: proposed fix

Don't update folder size if we're a condstore partial fetch.

Instead of doing checks for partialUIDFetch for every message, I decided to just do one at the end, since in the vast majority of the passes through this loop, partialUIDFetch will be false. I use a PRUint64 to prepare for the day where we handle IMAP folders > 4GB correctly here, though that change is beyond the scope of this bug (I'll file a follow-on fix for that)

Comment 3

[David :Bienvenu]

 Bug 548602 filed for 64 bit folder size issue.

Comment 4

[[e-mail]]

Comment on attachment 428948 [details] [diff] [review]
proposed fix

I don't have CONDSTORE so I can't really test this.

>     if (NS_SUCCEEDED(dbHdr->GetMessageSize(&messageSize)))
>-      mFolderSize += messageSize;
>+      newFolderSize += messageSize;
Worth doing this only for non-partial fetches?

>+  if (!partialUIDFetch && newFolderSize != oldFolderSize)
>+  {
>+    mFolderSize = newFolderSize;
>     NotifyIntPropertyChanged(kFolderSizeAtom, oldFolderSize, mFolderSize);
I thought the use of oldFolderSize was confusing. Maybe use mFolderSize in the condition, then have an oldFolderSize temporary inside this block?

Comment 5

[David :Bienvenu]

(In reply to comment #4)
> (From update of attachment 428948 [details] [diff] [review])
> I don't have CONDSTORE so I can't really test this.
> 
> >     if (NS_SUCCEEDED(dbHdr->GetMessageSize(&messageSize)))
> >-      mFolderSize += messageSize;
> >+      newFolderSize += messageSize;
> Worth doing this only for non-partial fetches?

I think it's worth not checking the boolean, since the vast majority of the time through this loop, it will be false.

> 
> >+  if (!partialUIDFetch && newFolderSize != oldFolderSize)
> >+  {
> >+    mFolderSize = newFolderSize;
> >     NotifyIntPropertyChanged(kFolderSizeAtom, oldFolderSize, mFolderSize);
> I thought the use of oldFolderSize was confusing. Maybe use mFolderSize in the
> condition, then have an oldFolderSize temporary inside this block?

that's a thought - I'll do that.

Comment 6

[David :Bienvenu]

Attachment: fix addressing comment

gets rid of temp oldFolderSize variable...

Comment 7

[[e-mail]]

Comment on attachment 428975 [details] [diff] [review]
fix addressing comment

>-  if (oldFolderSize != mFolderSize)
>-    NotifyIntPropertyChanged(kFolderSizeAtom, oldFolderSize, mFolderSize);
>+  if (!partialUIDFetch && newFolderSize != mFolderSize)
>+  {
>+    NotifyIntPropertyChanged(kFolderSizeAtom, mFolderSize,
>+                            (PRUint32) newFolderSize);
>+    mFolderSize = newFolderSize;
>+  }
The old code obviously updated the folder size before notifying. Whether this is a problem depends on how you want to notify for a 64-bit folder size changing; are you going to create a new notification, or fire the old notification and expect consumers to read the new value? Note that the only caller of NotifyIntPropertyChanged that fires the notification first is ironically SetSizeOnDisk, which has one caller, which passes the current size!

Comment 8

[David :Bienvenu]

(In reply to comment #7)

> The old code obviously updated the folder size before notifying. Whether this
> is a problem depends on how you want to notify for a 64-bit folder size
> changing; are you going to create a new notification, or fire the old
> notification and expect consumers to read the new value? Note that the only
> caller of NotifyIntPropertyChanged that fires the notification first is
> ironically SetSizeOnDisk, which has one caller, which passes the current size!

yeah, you're right - I need the temporary var.

Comment 9

[David :Bienvenu]

Attachment: make sure size is right before sending notification

I'm either going to add a new notification, or evilly make NotifyIntPropertyChanged take a 64 bit int.

Comment 10

[David :Bienvenu]

fix checked in.

Comment 11

[Wayne Mery (:wsmwk)]

is it correct to cite this bug in http://gsfn.us/t/ogo5 ? (not sure if it was me or someone else)

and do the consequences of this bug justify pushing into 3.0.x?

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 429015 [details] [diff] [review]
make sure size is right before sending notification

Didn't make 3.0.6, will re-consider for 3.0.7.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 429015 [details] [diff] [review]
make sure size is right before sending notification

Having thought about the risk/benefit and the fact that we know most people using the extension aren't on 3.0.x, I've decided not to take this - the fix is in 3.1.x which we're offering for updates at the moment.