Closed
Bug 1702600
Opened 4 years ago
Closed 3 years ago
[QM_TRY] Failures in dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize
Categories
(Core :: Storage: Quota Manager, defect, P2)
Core
Storage: Quota Manager
Tracking
()
RESOLVED
FIXED
103 Branch
People
(Reporter: jstutte, Assigned: janv)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
1.08 KB,
patch
|
pascalc
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 8 | 16 | 16 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7094:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4101:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4164:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4202:NS_ERROR_ILLEGAL_VALUE |
| 4 | 5 | 5 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7081:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4093:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4156:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4194:NS_ERROR_ILLEGAL_VALUE |
| 3 | 3 | 3 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7081:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4093:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4156:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4194:NS_ERROR_ILLEGAL_VALUE |
|
Reporter | |
Updated•4 years ago
|
Severity: -- → S3
Priority: -- → P2
|
|
Reporter | |
Comment 1•4 years ago
•
|
||
|
|
Reporter | |
Comment 2•4 years ago
|
||
|
|
Reporter | |
Comment 3•4 years ago
|
||
|
|
Reporter | |
Comment 4•4 years ago
|
||
It seems, we are not able to deserialize a usage value.
The expected format is:
"<Prefix><DecIntValue> <Prefix><DecIntValue> ..." with Prefix in "I","C","S","L"
Assuming that those values are only ever generated by ClientUsageArray::Serialize, I can see only two possible errors:
- The array string has been truncated during read or write right after a prefix character
- The integer value has been written with a different locale and 1000 separator than we use while reading
I was not able to look up easily, if 2. is possible starting from aText.AppendInt(clientUsage.value()); and Substring(token, 1).ToInteger(&rv);. But the error seems to happen quite often such that I would not expect it to be caused (only) by this (unlikely) edge case.
But also 1. seems scary and unlikely.
|
|
Reporter | |
Updated•4 years ago
|
Flags: needinfo?(jvarga)
|
|
Reporter | |
Comment 5•4 years ago
|
||
- possibility: What happens if we serialize a -1 that gets transformed into a
uint64_tbefore serializing and then deserialized?
|
|
Reporter | |
Comment 6•4 years ago
|
||
|
|
Reporter | |
Comment 7•4 years ago
|
||
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 1 | 1 | 1 | dom/quota/ActorsParent.cpp:<Unknown 7081> | dom/quota/ActorsParent.cpp#7081:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4093:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4156:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4194:NS_ERROR_ILLEGAL_VALUE |
|
|
Reporter | |
Comment 8•4 years ago
|
||
See bug 1708119, might be related to badly flushed files, too.
|
|
Reporter | |
Comment 9•4 years ago
|
||
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 16 | 25 | 25 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| 1 | 1 | 1 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 27 | 49 | 51 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| 1 | 1 | 1 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 16 | 25 | 25 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| 1 | 1 | 1 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
|
|
Reporter | |
Comment 10•4 years ago
|
||
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 31 | 66 | 67 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| 16 | 36 | 36 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 1 | 3 | 3 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 25 | 79 | 82 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| 1 | 1 | 1 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
|
|
Reporter | |
Comment 11•4 years ago
|
||
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 60 | 170 | 172 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 16 | 22 | 22 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7085:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4097:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4160:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4198:NS_ERROR_ILLEGAL_VALUE |
|
|
Reporter | |
Comment 12•4 years ago
•
|
||
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 6 | 16 | 17 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7093:NS_ERROR_ILLEGAL_VALUE |
|
|
Reporter | |
Updated•3 years ago
|
|
||
Comment 13•3 years ago
|
||
Taken from Attachment 9239371 [details].
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 6 | 19 | 19 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7122:NS_ERROR_ILLEGAL_VALUE |
|
|
||
Comment 14•3 years ago
|
||
Taken from Attachment 9261549 [details].
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 60 | 134 | 134 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7367:NS_ERROR_ILLEGAL_VALUE |
| 14 | 22 | 23 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7367:NS_ERROR_ILLEGAL_VALUE |
|
|
||
Comment 15•3 years ago
|
||
Taken from Attachment 9263751 [details].
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 113 | 254 | 254 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7367:NS_ERROR_ILLEGAL_VALUE |
|
|
Reporter | |
Comment 16•3 years ago
|
||
We see this also as WARNING:
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 100 | 222 | 222 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7367:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4202:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4286:NS_ERROR_ILLEGAL_VALUE <- dom/quota/ActorsParent.cpp#4328:NS_ERROR_ILLEGAL_VALUE |
|
|
||
Comment 17•3 years ago
|
||
Taken from Attachment 9265725 [details].
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 70 | 154 | 154 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7372:NS_ERROR_ILLEGAL_VALUE |
|
|
||
Comment 18•3 years ago
|
||
Taken from Attachment 9267680 [details].
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 44 | 125 | 126 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7372:NS_ERROR_ILLEGAL_VALUE |
|
|
||
Comment 19•3 years ago
|
||
Taken from Attachment 9268673 [details].
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 53 | 130 | 130 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7395:NS_ERROR_ILLEGAL_VALUE |
|
|
||
Comment 20•3 years ago
|
||
Taken from Attachment 9273630 [details].
| Clients | Sessions | Hits | Anchor | Stack |
|---|---|---|---|---|
| 46 | 149 | 150 | dom/quota/ActorsParent.cpp:ClientUsageArray::Deserialize | dom/quota/ActorsParent.cpp#7459:NS_ERROR_ILLEGAL_VALUE |
|
Assignee | |
Updated•3 years ago
|
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Flags: needinfo?(jvarga)
|
|
Assignee | |
Comment 21•3 years ago
|
||
This will enable writting a gtest for the class.
|
|
Assignee | |
Comment 22•3 years ago
|
||
We use uint64_t for usages internally, but the deserializing function was using
ToInteger which can handle only 32bit integers. The problem became more visible
after increasing the group limit from 2GB to 10GB, but it was possible to
experience the problem even before that because persisted origins are not
limited by the group limit.
Depends on D148479
|
||
Comment 23•3 years ago
|
||
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5daf2a3470cc
Move ClientUsageArray class to separate source files; r=dom-storage-reviewers,jstutte
https://hg.mozilla.org/integration/autoland/rev/653fe656560d
Change ToInteger to ToInteger64 in ClientUsageArray::Deserialize; r=dom-storage-reviewers,jstutte
|
||
Comment 24•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/5daf2a3470cc
https://hg.mozilla.org/mozilla-central/rev/653fe656560d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox103:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch
|
|
Assignee | |
Comment 25•3 years ago
|
||
|
|
Assignee | |
Comment 26•3 years ago
|
||
Comment on attachment 9280611 [details] [diff] [review]
Patch for Beta and ESR
Beta/Release Uplift Approval Request
- User impact if declined: Users with large storage folder would still experience slow startups without this fix.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): It's a simple change, we just use a function which can handle bigger numbers.
- String changes made/needed: None
- Is Android affected?: Unknown
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: Serious performance regression which has been discovered in bug 1567863.
- User impact if declined: Users with large storage folder would still experience slow startups without this fix.
- Fix Landed on Version: 103
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): It's a simple change, we just use a function which can handle bigger numbers.
Attachment #9280611 -
Flags: approval-mozilla-esr102?
Attachment #9280611 -
Flags: approval-mozilla-beta?
|
|
Assignee | |
Updated•3 years ago
|
Attachment #9279999 -
Flags: approval-mozilla-beta?
|
|
Assignee | |
Updated•3 years ago
|
Attachment #9279999 -
Flags: approval-mozilla-beta?
|
||
Comment 27•3 years ago
|
||
Comment on attachment 9280611 [details] [diff] [review]
Patch for Beta and ESR
Approved for 102 beta 7, thanks.
Attachment #9280611 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
|
|
||
Comment 28•3 years ago
|
||
| bugherder uplift | ||
status-firefox102:
--- → fixed
|
|
||
Updated•3 years ago
|
Attachment #9280611 -
Flags: approval-mozilla-esr102?
You need to log in
before you can comment on or make changes to this bug.
Description
•