1656947 - Problem deleting items from browser.storage.sync when quota exceeded

Status: VERIFIED FIXED

(WebExtensions :: Storage, defect, P2)

Description

[sebastian.weitzel+github]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.105 Safari/537.36

Steps to reproduce:

Several users of my extension reported issues after updating to Firefox 79.
The extension manages data in the browser.storage.sync area, and the data has been migrated to the new rust based storage with quota enabled. Some users have now quota usage of 300%. App stops working, but it has a integrated storage management, where items can be removed.

This manual cleanup runs browser.storage.sync.remove(item), but this fails with Error: QuotaExceededError: storage.sync API call exceeded its quota limitations.

https://blog.mozilla.org/addons/2020/07/09/changes-to-storage-sync-in-firefox-79/

Actual results:

quota exceeded when removing and entry from browser.storage.sync area

Expected results:

item should be removed, even if the quota is exceeded.

Comment 1

[sebastian.weitzel+github]

Also it would be awesome if the Storage Inspector supported the Sync Storage. Then users could cleanup storage there maybe.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Remove acts as an "update", which probably isn't ideal - we probably should allow remove to succeed even if over quota.

Comment 4

[sebastian.weitzel+github]

Thanks for the feedback Mark! Would be really awesome if a fix / enhancement could be implemented for one of the Firefox 79 patch releases. Would that be feasible?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Sorry, but the chance of a patch like this getting into the release channel is almost zero.

Comment 6

[Alex Cornestean]

Hello,

I’ve managed to reproduce the issue on the latest Release version (79.0/20200720193547, updated from 78.0) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.
On the 78.0 version of the browser I added items to the eBay watchlist (around 114 items) and then added them from the watchlist to the extension, which subsequently caused the memory usage to get to around 156% (Memory consumption in browser.storage.sync area is 156.59%). Afterwards, I’ve updated the browser to the latest version, which has a quota enabled. Then, I proceeded to use the extenion’s “Cleanup Items” function. The items have not been removed, even though a message was displayed stating that they were and repeating errors were displayed in the web console (removeArticle(X) remoevInfoFromStorage failed: Error: QuotaExceededError: storage.sync API call exceeded its quota limitations.). Please see the attached screenshots for more details.

This was not reproducible on the latest Nightly (81.0a1/20200804215246) and Beta (80.0b4/20200804180257) as the quota cannot be exceeded. The maximum memory usage, using the same watchlist, was 99.33%. Items could be properly removed.

Comment 7

[Alex Cornestean]

Attachment: 2020-08-05_14h28_27.png

Comment 8

[Alex Cornestean]

Attachment: 2020-08-05_14h08_33.png

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Mark,
Based on the other comments in this issue I have the feeling that the issue is triggered when the extension has stored more data than allowed by the client side quota and the amount of data removed in the browse.storage.sync.remove API call is not enough to be back under the quota limit.
If that is correct, I guess that one way to fix it may be to not check the client side quota limits for remove API calls, which is likely something to be fixed entirely in the rust part of the API implementation.
Are my assumptions as describe above right?

This seems something that would be nice to fix in 81 (not sure we do have enough time to aim also to uplift to 80 beta, it may be too late for that),

Is this something that you may be able to pick it up?
If you don't have the bandwidth to pick this up, we may try to get to it as soon as we can.

am I right on assuming that the fix should be applied on the github repo first and then we have to vendor a new version of the related rust package to land the fix on mozilla-central?

Comment 10

[Lina Butler (ex-Mozilla)]

Based on the other comments in this issue I have the feeling that the issue is triggered when the extension has stored more data than allowed by the client side quota and the amount of data removed in the browse.storage.sync.remove API call is not enough to be back under the quota limit.
If that is correct, I guess that one way to fix it may be to not check the client side quota limits for remove API calls, which is likely something to be fixed entirely in the rust part of the API implementation.
Are my assumptions as describe above right?

I’m not Mark 😂 but yes, that’s exactly right, Luca! Here is where we remove all the keys we’re given, and then call save_to_db to update the value in the database. Since we’re not clearing out the entire storage area, we end up in this branch, which checks the quota and fails. We’ll need to teach that branch to ignore quotas for removes, but not sets—maybe via an enum that wraps the &JsonValue with the type of operation?

If you don't have the bandwidth to pick this up, we may try to get to it as soon as we can.

We can do it, or you’re also welcome to give it a try if you’d like to land a small Rust patch.

am I right on assuming that the fix should be applied on the github repo first and then we have to vendor a new version of the related rust package to land the fix on mozilla-central?

That’s right! Please make a PR to a-s with the test, and then, once it’s landed on main, bump the rev for the application-services dependency in the tree to point to the new commit (sorry that’s so clunky! Depending on crates in Git repos is kind of hairy).

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Lina Cambridge [:lina] from comment #10)

Based on the other comments in this issue I have the feeling that the issue is triggered when the extension has stored more data than allowed by the client side quota and the amount of data removed in the browse.storage.sync.remove API call is not enough to be back under the quota limit.
If that is correct, I guess that one way to fix it may be to not check the client side quota limits for remove API calls, which is likely something to be fixed entirely in the rust part of the API implementation.
Are my assumptions as describe above right?

I’m not Mark 😂 but yes, that’s exactly right, Luca! Here is where we remove all the keys we’re given, and then call save_to_db to update the value in the database. Since we’re not clearing out the entire storage area, we end up in this branch, which checks the quota and fails. We’ll need to teach that branch to ignore quotas for removes, but not sets—maybe via an enum that wraps the &JsonValue with the type of operation?

If you don't have the bandwidth to pick this up, we may try to get to it as soon as we can.

We can do it, or you’re also welcome to give it a try if you’d like to land a small Rust patch.

Thanks a lot Lina!
Honestly, I would love to give it a try, I took a look and created mozilla/application-services#3489, hopefully I got your suggestion correctly and it may be "not too far" from what you described above ;-)

Comment 12

[Lina Butler (ex-Mozilla)]

Great, thanks Luca! I just landed your patch to main, and also cherry-picked it to the vendor-mc/78 branch, which is what we're currently vendoring. To avoid churn for the other dependencies, let's bump the rev to the latest one for that branch—it might even be a small enough change that we could uplift to 80!

I also filed bug 1658164 to bring a-s up to date, which is definitely going to be a bigger patch. 😅

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1656947 - Vendor application-services to import fix on webext-storage component. r?lina!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a82f018d47b7
Vendor application-services to import fix on webext-storage component. r=markh

Comment 15

[Andreea Pavel [:apavel]]

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

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Lina and Mark, thanks a lot for your help on preparing and landing this fix, very much appreciated!
The change landed to vendor the updated application-services seems "small enough" after all, how do you feel about requesting an uplift to 80 beta for it?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

That sounds fine to me, and Lina said the same in comment 3 (and I think it is "small enough" to qualify). I think the general policy though is that the patch author requests uplift, so the request should come from you.

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9169049 [details]
Bug 1656947 - Vendor application-services to import fix on webext-storage component. r?lina!

Beta/Release Uplift Approval Request

• User impact if declined: Some users may experience issues with some of their extensions:
  if the extension was over quota while used in a Firefox version < 79 then the extension may not be able to easily go back under the client-side quota limit because calling browse.storage.sync.remove fails while the extension is still over quota.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: The patch also include a new unit test, but it may be good to also QA verify it explicitly using the same STR used to confirm the issue, as described in comment 6.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change applied to fix the issue is small and restricted to the subset of rust code responsible for the browser.storage.sync API implementation, and it is also being covered by a new unit test.
• String changes made/needed:

Comment 19

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 9169049 [details]
Bug 1656947 - Vendor application-services to import fix on webext-storage component. r?lina!

approved for 80.0b8

Comment 20

[Julien Cristau [:jcristau] (back August 18)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8a94c004b2ca

Comment 21

[Alex Cornestean]

Hello,

Verified the fix on the latest Nightly (81.0a1/20200813092915) and Beta (80.0b8/20200813191622) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.

After proceeding as in Comment 6 (using Nightly 78.0a1 – 137.18% storage usage and Beta 78.0b9 – 115.35% storage usage), I’ve updated the browser and used the “Cleanup Items” function. All items have been successfully removed, confirming the fix.