1636665 - Status bar is displaying incorrect quota indication (infinity, full) for TeraByte quotas

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect, P1)

Description

[Mark Banner (:standard8)]

Attachment: Screenshot 2020-05-09 at 09.24.32.png

On Thunderbird 76, I'm getting a quota indication in the status bar that I didn't get in 68 (separate profile bug with the same account).

It says "Infinity%" and indicates red, i.e. there's no space left.

When I go to folder properties and look at the quota, it also says the same but "full".

If I go back to my Thunderbird 68 profile, in the properties dialog it says 0% full.

This is with my mozilla.com account that is hosted by Google.

Requesting tracking as this is bad UI that's on the main display and obvious regression.

Comment 1

[Magnus Melin [:mkmelin]]

It will probably be from bug 841906. I wonder why it ends up with infinity - that certainly doesn't happen for everyone.
Maybe around here: https://hg.mozilla.org/comm-central/rev/54a83a246c9e#l1.65 or maybe something getting wrongly parsed from the imap response?

Do you have an imap log?

Comment 2

[gene smith]

I can cause the "Infinity%" to occur only if I set the "limit" decoded from the gmail storage quota response to 0 in the Trunk tb code. This is my info from tb log where quota is obtain from gmail:

[(null) 11848: IMAP]: I/IMAP 0x7f4b8c662800:imap.gmail.com:S-INBOX:SendData: 11 getquotaroot "INBOX"
[(null) 11848: IMAP]: D/IMAP ReadNextLine [stream=0x7f4b8d1a8740 nb=24 needmore=0]
[(null) 11848: IMAP]: I/IMAP 0x7f4b8c662800:imap.gmail.com:S-INBOX:CreateNewLineFromSocket: * QUOTAROOT "INBOX" ""
[(null) 11848: IMAP]: D/IMAP ReadNextLine [stream=0x7f4b8d1a8740 nb=38 needmore=0]
[(null) 11848: IMAP]: I/IMAP 0x7f4b8c662800:imap.gmail.com:S-INBOX:CreateNewLineFromSocket: * QUOTA "" (STORAGE 968704 15728640)
[(null) 11848: IMAP]: D/IMAP ReadNextLine [stream=0x7f4b8d1a8740 nb=15 needmore=0]
[(null) 11848: IMAP]: I/IMAP 0x7f4b8c662800:imap.gmail.com:S-INBOX:CreateNewLineFromSocket: 11 OK Success

Can you tell us what yours shows for the same getquotaroot imap command. Re: https://wiki.mozilla.org/MailNews:Logging
In the log above, 968704=usage, 15728640=limit.

Comment 3

[Mark Banner (:standard8)]

Sorry for the delay, I get:

[(null) 55617: IMAP]: I/IMAP 0x13095a000:imap.gmail.com:S-INBOX:SendData: 11 getquotaroot "INBOX"
[(null) 55617: IMAP]: D/IMAP ReadNextLine [stream=0x12e640190 nb=24 needmore=0]
[(null) 55617: IMAP]: I/IMAP 0x13095a000:imap.gmail.com:S-INBOX:CreateNewLineFromSocket: * QUOTAROOT "INBOX" ""
[(null) 55617: IMAP]: D/IMAP ReadNextLine [stream=0x12e640190 nb=47 needmore=0]
[(null) 55617: IMAP]: I/IMAP 0x13095a000:imap.gmail.com:S-INBOX:CreateNewLineFromSocket: * QUOTA "" (STORAGE 2305081 7881299347898368)
[(null) 55617: IMAP]: D/IMAP ReadNextLine [stream=0x12e640190 nb=15 needmore=0]
[(null) 55617: IMAP]: I/IMAP 0x13095a000:imap.gmail.com:S-INBOX:CreateNewLineFromSocket: 11 OK Success

So the limit is 7 petabytes.

From what I can see of the patch in bug 841906, the limit is a 32 bit unsigned limit. It also uses atoi for which it looks like the behaviour is undefined when an integer is out of range.

Comment 4

[gene smith]

Can probably use atol or atoll I think to fix it. But interesting how google provides you with such a huge quota? Most I see advertised for paid G-Suite is 30T. My free account is only 15G.

I think the quota is supposed to be in KBytes so with speedcrunch I see something for yours like 8x10^^18 bytes:

7881299347898368 * 1024
= 8.070450532247928832e18

Comment 5

[Magnus Melin [:mkmelin]]

Since the number is in KB I guess the limit is 7 TB.

Comment 6

[Magnus Melin [:mkmelin]]

Gene, do you want to take this?

Comment 7

[gene smith]

It appear to me that the imap quota RFC doesn't require that the storage number is in KB. It only shows an example with the numbers in "1024 octet" for the units of STORAGE. But it seems that has been adopted as a convention by the various imap server vendors that STORAGE is in kbytes (1024 octets).
https://tools.ietf.org/html/rfc2087#page-2

However, the Melnikov draft rfc wants to define STORAGE explicitly in KB:
https://tools.ietf.org/id/draft-melnikov-extra-quota-00.html#rfc.section.5.1

Since the number is in KB I guess the limit is 7 TB.

The raw number is 7881299347898368. This is exactly 7 Peta since a Peta is 1024 to the 5th power:
7881299347898368 /1024^5
= 7

So if gmail is reporting STORAGE in bytes and not in KB, the limit is 7 PB and not 7 TB. But if gmail is reporting the quota in KB, the STORAGE limit is actually 7*1024 PB which is also essentially unlimited I would think.

Gene, do you want to take this?

Ok, sounds like "fun". (Still working on the UTF8=ACCEPT/Skräppost problem.)

Comment 8

[Magnus Melin [:mkmelin]]

GMail reports in KB - my gmail account shows 15G quota which sounds about right.

We'll also need to adjust the JS calculations to use BigInt, since JS will only be able to handle a quota of around 8TB - after which we reach too large numbers.

Comment 9

[Mark Banner (:standard8)]

I just dug around a bit, it looks like we have 'unlimited' storage according to Google. I guess 7PB is how they express that at the moment.

Comment 10

[gene smith]

GMail reports in KB - my gmail account shows 15G quota which sounds about right.

Yes, at least "free" gmail reports in KB and is exactly 15G for me too:
15728640 * 1024 /1024^3
= 15

I just dug around a bit, it looks like we have 'unlimited' storage according to Google. I guess 7PB is how they express that at the moment.

Google may be reporting a 1024*7 PB limit. I assume this a company-paid commercial account of some sort. The largest advertised account I see is for 30TB and is $300 per month: https://one.google.com/storage?hl=en

We'll also need to adjust the JS calculations to use BigInt, since JS will only be able to handle a quota of around 8TB - after which we reach too large numbers.

Thanks. I'll have to learn about this too.

Comment 11

[Magnus Melin [:mkmelin]]

Any update on this?

Comment 12

[Magnus Melin [:mkmelin]]

Sending this over to Ping.

Comment 13

[Ping Chen [:rnons] (away)]

Attachment: 1636665.patch

• Support petabyte in formatFileSize
• Use BigInt when computing folder usage percentage

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9160025 [details] [diff] [review]
1636665.patch

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

::: mail/base/content/commandglue.js
@@ +140,5 @@
>      return;
>    }
>  
> +  let quotaUsagePercentage = q =>
> +    Number((100n * BigInt(q.usage)) / BigInt(q.limit));

You're not rounding it like before. Won't that cause 10.9879879879% type of displays?

::: mailnews/base/test/unit/test_formatFileSize.js
@@ +26,5 @@
>      b: "byteAbbreviation2",
>      kb: "kiloByteAbbreviation2",
>      mb: "megaByteAbbreviation2",
>      gb: "gigaByteAbbreviation2",
> +    pb: "petaByteAbbreviation2",

next after GB is TB: terabyte

Comment 15

[Ping Chen [:rnons] (away)]

Attachment: 1636665.patch

You're not rounding it like before. Won't that cause 10.9879879879% type of displays?

Seems not, for example 5n / 2n is 2n not 2.5n, so Number((100n * BigInt(q.usage)) / BigInt(q.limit)) is effectively equivalent to Math.floor for small integers.

next after GB is TB: terabyte

I knew something was wrong, but failed to notice I missed TB. Fixed now.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9160236 [details] [diff] [review]
1636665.patch

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

Did you verify it's parsing large enough numbers? I think https://searchfox.org/comm-central/rev/35207b3950db6de34661c7a1239dc863fb1e66c6/mailnews/imap/src/nsImapServerResponseParser.cpp#2617 should be atol() and types changed to handle long?

Please also add the strings to the suite messenger.properties

Comment 17

[Ping Chen [:rnons] (away)]

Attachment: 1636665.patch

• Changed to use atol and replace more u_int32 with u_int64

I was hoping the compile can stop me or at least warn me when passing u_int64 to u_int32.

Comment 18

[gene smith]

I think it might be better to use atoll() when you are saving into a uint64_t. Re: https://searchfox.org/comm-central/search?q=atoll%28&path=&case=false&regexp=false

Also, the original code used PR_sscanf() instead of atol(). I was intending to go back to that and use something like %lld to scan into the two uint64_t's (usage and limit). PR_sscanf() also can return an error that atoll() won't.

Anyhow, I'll test the patch as is with my Dovecot server that I've set up with multiple quotaroots.
Thanks!

Comment 19

[gene smith]

Seems to work fine as is with just atol(). I caused dovecot to return the same storage string as google/gmail 7881299347898368 and tb now reports the storage limit as 7168 PB. I also tested in the TB range and GB range and still looks good. The % calc looks right too.

I then changed atol to atoll and see no difference. Since I'm on a 64bit system with a 64bit build, there may be problems with 32bit, not sure. (Do we still support 32bits?)

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9160352 [details] [diff] [review]
1636665.patch

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

Let's update to atoll. 
r=mkmelin with that changed

Comment 21

[Ping Chen [:rnons] (away)]

Attachment: 1636665.patch

• Changed to atoll
• Added /* global: BigInt */ to commandglue.js

Comment 22

[Ping Chen [:rnons] (away)]

Attachment: 1636665.patch

Added BigInt to .eslintrc

  globals: {
    BigInt: "readonly",
  },

Or is it better to use /* global BigInt */ to where it is used?

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9160856 [details] [diff] [review]
1636665.patch

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

I guess I'd add the global to both files instead.
Don't really understand why it's needed though since it's a built in type.

Comment 24

[Ping Chen [:rnons] (away)]

Attachment: 1636665.patch

Changed to use /* global BigInt */.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e8ecd7792a74
Support petabyte in imap folder quote size and percentage calculations. r=mkmelin

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9160904 [details] [diff] [review]
1636665.patch

[Approval Request Comment]
Regression caused by (bug #): bug 841906.
User impact if declined: Bad UI shown to users with large quotas
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): no good alternatives.

Comment 27

[Wayne Mery (:wsmwk)]

Comment on attachment 9160904 [details] [diff] [review]
1636665.patch

Approved for beta
Approved for esr78

Comment 28

[Rob Lemley [:rjl]]

Thunderbird 79.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/1d7aea3ffc5d

Comment 29

[Rob Lemley [:rjl]]

Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/6bf50791d325