2066923 - Quota manager: switch usage counters from unsigned integers to signed integers

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement)

Description

[Arnaud Bienner]

Currently we're using uint64_t for our usages values, because there was an assumption they will never go negative.
However, bug 1585978 proves that this is not true and we have bugs here: until it is resolved, we should turn some fields to signed integers, to avoid bugs like bug 2065499, where a usage value underflow, which makes it looks like a very big (unsigned) integer, making comparison like "usedStorage > maxStorage" return true, while they shouldn't.

The alternative would be to clamp the value to 0, but they are some concerns some out of order quota operations could lead to transient negative values, quickly reconciliated later. And clamping those to 0 could create a drift.

Comment 2

[Arnaud Bienner]

Attachment: Bug 2066923 - Quota manager: use signed integers for usage values which we know can underflow. r?#dom-storage

Comment 3

[Jari Jalkanen [:jari]]

Yeah, hiding the issue with clamping would lead to other issues and make it harder to fix the underlying problem with our algorithm.

Comment 4

[Arnaud Bienner]

I'll land this; as Jari said in the patch "it can make things better and can't make anything worse".

A few notes:

• Even though it's unclear if it's OK to have negative values at that stage (i.e. not having reconciled the values earlier on), this can indeed only make things better: we weren't expecting to have values going above 2^63, so it's fine to have signed integers
• Using native unsigned integers types doesn't enforce anything, it doesn't bring any safety at all: only potential problems (underflow). Should we want to enforce "this can't go negative", we should have a dedicated custom type for this (e.g. CheckedInt).
• About the point above, asuth mentioned to me that the Google coding rules suggest to always use signed integers (though our coding rules disagree on that point: Prefer unsigned types for semantically-non-negative integer values).
• As a follow up, Jari suggested we could use strong type for usage: even if it just wraps an integer type, it could prevent implicit conversion between types, making the semantic clearer

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/fa9a83b65bd9
https://hg.mozilla.org/integration/autoland/rev/26dfca9c0180
Quota manager: use signed integers for usage values which we know can underflow. r=dom-storage-reviewers,jari

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b46a320e0381
https://hg.mozilla.org/integration/autoland/rev/9a4a36bd4a02
Revert "Bug 2066923 - Quota manager: use signed integers for usage values which we know can underflow. r=dom-storage-reviewers,jari" for causing build bustages in nsRFPService.cpp.

Comment 7

[Serban Stanca [:SerbanS]]

Reverted this because it was causing build bustages in nsRFPService.cpp.

• Revert link
• Push with failures
• Failure Log
• Failure line: ../../../../../checkouts/gecko/toolkit/components/resistfingerprinting/nsRFPService.cpp:X:24: error: comparison of integer expressions of different signedness: 'uint64_t' {aka 'long unsigned int'} and 'int64_t' {aka 'long int'} [-Werror=sign-compare]

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3ec29012fc1e
https://hg.mozilla.org/integration/autoland/rev/bc0f71ea6278
Quota manager: use signed integers for usage values which we know can underflow. r=dom-storage-reviewers,jari

Comment 9

[Arnaud Bienner]

The backout was because of a compilation on gcc, which doesn't happen on clang. And my try job didn't include the Linux debug gcc build that failed :(

Quoting the comment I left in https://mozilla.slack.com/archives/C0946KB63B4/p1787836803197809

int64_t limit = 50LL * 1024LL * 1024LL * 1024LL;  // 50 GiB
  MOZ_ASSERT(limit / 5 ==
             dom::quota::QuotaManager::GetGroupLimitForLimit(limit));

For the record, an interesting fact about why this results in a compilation error on gcc (the backout's reason) but not on clang: looks like clang is smarter (or more forgiving) and see the / 5 and knows converting a number that can go up to 2^64-1 will not overflow if converted to a number which max representation is 2^63-1, if it has been divided by 5.

Landed again now it has been fixed.

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/bc0f71ea6278