Closed Bug 861903 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Storage: IndexedDB, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
1.0.1 IOT1 (10may)
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- verified

People

(Reporter: ferjm, Assigned: bent.mozilla)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Blocks: low-storage
blocking-b2g: --- → tef?
Depends on: 853350
Ben, can you dig in here?
Assignee: nobody → bent.mozilla
blocking-b2g: tef? → ---
Did you mean to unnom, minus, or +? Can't tell by comment 1 what the blocking call is.
Flags: needinfo?(jst)
blocking-b2g: --- → tef?
I guess Johnny meant to tef+.

IMHO this bug needs to block for the same reasons as for example bug 861920 and bug 861894.
blocking-b2g: tef? → tef+
Flags: needinfo?(jst)
Whiteboard: [status: no patch yet]
Summary: Avoid non-certified apps to write in indexed DB while device free storage is low → Avoid apps to write in indexed DB while device free storage is low
Whiteboard: [status: no patch yet] → [status: no patch yet, blocked on low storage work and kernel change]
Attached patch Patch for mozilla-b2g18 (obsolete) — Splinter Review
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.
Attachment #741306 - Flags: review?(khuey)
Attached patch Patch for mozilla-b2g18 (obsolete) — Splinter Review
That was an old patch. This is the right one.
Attachment #741306 - Attachment is obsolete: true
Attachment #741306 - Flags: review?(khuey)
Attachment #741307 - Flags: review?(khuey)
Attachment #741307 - Flags: review?(khuey) → review?(Jan.Varga)
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.
Attachment #741307 - Flags: review?(Jan.Varga) → review+
Attachment #741307 - Attachment description: Patch for mozilla-b2g18 → Part 1: Add low disk space mode (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)|...
Attachment #742312 - Flags: review?(Jan.Varga)
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.
Attachment #742312 - Flags: review?(Jan.Varga)
Target Milestone: --- → 1.0.1 IOT1 (10may)
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.
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.
What are the next steps here?
Whiteboard: [status: no patch yet, blocked on low storage work and kernel change] → [status: has r+ patch, waiting on low storage work and kernel change]
Bug 853350 needs to land. And then I need to write a trunk patch (probably a different bug though).
Attachment #741307 - Attachment description: Part 1: Add low disk space mode (mozilla-b2g18) → Patch for mozilla-b2g18
Attached patch Patch for mozilla-central (obsolete) — Splinter Review
Merged to trunk.
Attachment #742312 - Attachment is obsolete: true
Attachment #747566 - Flags: review+
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
The same comments apply for the b2g18 patch
Whiteboard: [status: has r+ patch, waiting on low storage work and kernel change] → [status: has r+ patch, needs minor work, waiting on low storage work]
Updated for newer nsIDiskSpaceWatcher changes.
Attachment #741307 - Attachment is obsolete: true
Attachment #748214 - Flags: review+
Attachment #747566 - Attachment is obsolete: true
Whiteboard: [status: has r+ patch, needs minor work, waiting on low storage work] → [status: has r+ patch, waiting on low storage work]
Whiteboard: [status: has r+ patch, waiting on low storage work] → [status: needs uplift]
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.
Keywords: verifyme
QA Contact: 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.
You need to log in before you can comment on or make changes to this bug.