861903 - Avoid apps to write in indexed DB while device free storage is low

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

Bug 853350 is going to add a fanotify-based component that will notify about a low storage situation. We need to observe this notification and avoid any non-certified app to write in indexedDB until the low device storage situation is solved.

Comment 1

[Johnny Stenback (:jst)]

Ben, can you dig in here?

Comment 2

[Jason Smith [:jsmith]]

Did you mean to unnom, minus, or +? Can't tell by comment 1 what the blocking call is.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

I guess Johnny meant to tef+.

IMHO this bug needs to block for the same reasons as for example bug 861920 and bug 861894.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

This works for b2g18. Minimal changes, we're going to return a QuotaExceededError whenever we get into low disk mode. Depends on the patch in bug 853350. Also has a huge test.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

That was an old patch. This is the right one.

Comment 6

[Jan Varga [:janv]]

Comment on attachment 741307 [details] [diff] [review]
Patch for mozilla-b2g18

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

looks good, r=me

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1753,5 @@
> +      NS_WARNING("Refusing to create database because disk space is low!");
> +      return NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR;
> +    }
> +  }
> +

Nit: It seems there are only two callers of OpenDatabaseHelper::CreateDatabaseConnection() and the one in FileManager.cpp always calls it when the db file exists.
So the check added here could just live in OpenDatabaseHelper::DoDatabaseWork() I think.
But that's not a big deal.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 2: Simplify error compression (mozilla-b2g18)

Now we need to make sure that we have a sane way to do error conversions from SQLITE_FULL to QuotaError. This is the groundwork for a messy patch that changes a bunch of |NS_ENSURE_SUCCESS(rv, NS_ERROR_INDEXEDDB_UNKNOWN_ERR)|...

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 742312 [details] [diff] [review]
Part 2: Simplify error compression (mozilla-b2g18)

It's not clear that we need this for b2g really... Let's wait on the review.

Comment 9

[Julien Wajsberg [:julienw]]

FTR a "FreeSpaceWatcher" was created in Bug 818848 for the Webapps API. It was done in JS but I wonder if it's not possible to do it once (maybe in C++) and use it from everywhere it is needed.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

The space watcher component created in Bug 818848 is using a polling mechanism to check for device space while an app is being installed. We decided not to use the same strategy for Bug 853350 as having a component continuously polling for device space would be bad for performance and battery consumption. We chose to use a real time notifications strategy based on fanotify. Check Bug 853350 for more details about it.

Also, Bug 863596 will change the Webapps FreeSpaceWatcher component to observe notifications triggered by the fanotify based component.

Comment 11

[Andrew Overholt [:overholt]]

What are the next steps here?

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bug 853350 needs to land. And then I need to write a trunk patch (probably a different bug though).

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-central

Merged to trunk.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 747566 [details] [diff] [review]
Patch for mozilla-central

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

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +119,5 @@
> +      nsCOMPtr<nsIDiskSpaceWatcher> watcher =
> +        do_GetService(DISKSPACEWATCHER_CONTRACTID);
> +      if (watcher) {
> +        bool lowDiskSpaceMode;
> +        if (NS_SUCCEEDED(watcher->GetLowFreeSpace(&lowDiskSpaceMode))) {

You probably noticed this, but the idl for nsIDiskSpaceWatcher changed. So you have to s/GetLowFreeSpace/GetIsDiskFull

@@ +123,5 @@
> +        if (NS_SUCCEEDED(watcher->GetLowFreeSpace(&lowDiskSpaceMode))) {
> +          sLowDiskSpaceMode = lowDiskSpaceMode ? 1 : 0;
> +        }
> +        else {
> +          NS_WARNING("GetLowFreeSpace failed!");

ditto

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

The same comments apply for the b2g18 patch

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

Updated for newer nsIDiskSpaceWatcher changes.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-central

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/projects/birch/rev/22a77fd7ab35

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22a77fd7ab35
https://hg.mozilla.org/mozilla-central/rev/7a362c86108c
https://hg.mozilla.org/mozilla-central/rev/462c163822b3
https://hg.mozilla.org/mozilla-central/rev/c26ceab09064
https://hg.mozilla.org/mozilla-central/rev/ee537efbe1e2
https://hg.mozilla.org/mozilla-central/rev/8db9762409f4

Hilarity.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Ben, I'm going to leave this uplift for you since I have no confidence that the b2g18 patch attached here includes the fixes it took to get this compiling and I have no time to go through it today myself.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Turned the test on for most platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b8505fdd33

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/50726fbdc25e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6e1dbf036d34

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77b8505fdd33

Comment 25

[Jason Smith [:jsmith]]

lgtm on 1.01 with a partner build.

Tested in a similar fashion to what was done with appcache here - https://bugzilla.mozilla.org/show_bug.cgi?id=861894#c60.