861920 - Avoid apps to write in its local storage while device free storage is low

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, 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 app to write in localStorage until the low device storage situation is solved.

Comment 1

[Johnny Stenback (:jst)]

Blocking per todays triage meeting.

Comment 2

[Honza Bambas (:mayhemer)]

To confirm: this is intended to land on b2g18 branch?

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

I already received positive feedback from Honza in a conversation via IRC about this approach.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

I've successfully tested this patch with the space watcher mock from bug 861894. The approach is basically observing the notifications from the space watcher component (or checking its current state during the DB initialization) and returning quota 0 when we know that we are running out of space.

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

The attached patch is based in b2g18. It seems that the code has considerably changed recently, so I'll write a different patch for inbound.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Test app

Simple test application.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 739590 [details] [diff] [review]
v1

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

This needs a test.  Just call Observe on nsDOMStorageManager artificially in the test.  Check dom/tests/mochitest/localstorage for examples of dom storage tests.

::: dom/src/storage/nsDOMStorage.cpp
@@ -487,5 @@
>  }
>  
>  PLDHashOperator
>  SessionStorageTraverser(nsSessionStorageEntry* aEntry, void* userArg) {
> -  nsCycleCollectionTraversalCallback *cb = 

OK, please, take care only of whitespaces in your newly added code but leave existing unattended.  Sorry I wasn't clearer about this!

::: dom/src/storage/nsDOMStorageBaseDB.cpp
@@ +26,5 @@
>  
>  nsDOMStorageBaseDB::nsDOMStorageBaseDB()
>  {
>    mScopesVersion.Init(8);
> +  mDisabled = false;

Use initializer please.

::: dom/src/storage/nsDOMStorageDBWrapper.cpp
@@ +399,5 @@
> +nsDOMStorageDBWrapper::Disable()
> +{
> +  mPersistentDB.mDisabled = true;
> +  mSessionOnlyDB.mDisabled = true;
> +  mPrivateBrowsingDB.mDisabled = true;

Don't you think this should only affect the persistent DB ? ;)

No need to limit PB and SO in memory stores.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2 (b2g18)

Thanks for the feedback Honza!

This patch addresses your comments.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1 (inbound)

DOMStorage code is very different in b2g18 and m-i. This patch applies in m-i.

I followed the same approach of setting the quota to 0 when we are in a low storage situation.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Untested tests. I still need to set my build to run tests properly.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 740883 [details] [diff] [review]
v1 (inbound)

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

You are repeting the same mistake as the last time.  To explain:

DOMStorageObserver is a singleton in a process.
DOMStorageManager can exist more then one: there is one instance (service) of DOMLocalStorageManager that tracks localStorage instances, and then there are 0..N DOMSessionStorageManagers (one for each nsDocShell) which track sessionStorage's per window.
All DOM*StorageManager instances listens to the one DOMStorageObserver.
You must not disable sessionStorages when we are low on disk space.

The code has to be better placed as:

Add a getter to DOMStorageManager bool LowDiskSpace()
Make the internal observer->manager notification be "lowdiskspace", "nolowdisksace"
Let only mType == LocalStorage managers handle it.
At [1] add: if (aDelta > 0 && mManager && mManager->LowDiskSpace()) return false;
At [2] you don't need to do any changes


[1] http://hg.mozilla.org/mozilla-central/annotate/681bbf7717c1/dom/src/storage/DOMStorageCache.cpp#l197
[2] http://hg.mozilla.org/mozilla-central/annotate/681bbf7717c1/dom/src/storage/DOMStorageCache.cpp#l727

::: dom/src/storage/DOMStorageCache.cpp
@@ +770,5 @@
>  
> +  nsresult rv;
> +  nsCOMPtr<nsIDiskSpaceWatcher> diskSpaceWatcher =
> +    do_GetService("@mozilla.org/toolkit/disk-space-watcher;1", &rv);
> +  NS_ENSURE_TRUE(rv, nullptr);

Don't do this please!  When this fails, make it a silent fail.  This will disable the functionality completely when something around the watcher gets broken.

Is the watcher working well on both the parent and content process well?

Persoanlly I'd rather see this in DOMLocalStorageManager ctor.

@@ +774,5 @@
> +  NS_ENSURE_TRUE(rv, nullptr);
> +  bool lowFreeSpace;
> +  diskSpaceWatcher->GetLowFreeSpace(&lowFreeSpace);
> +  if (lowFreeSpace) {
> +    DOMStorageObserver::Self()->Notify("disable");

Add DOMStorageObserver::Self() non-null check.

::: dom/src/storage/DOMStorageManager.cpp
@@ +48,5 @@
>      preferencesInitialized = true;
>    }
>  
> +  if (DOMStorageManager::sDisabled)
> +    return 0;

Just for next time (since you are going to remove this code): DOM styling is:

if (cond) {
  statemtn;
}

even it's just a single line.

::: dom/src/storage/DOMStorageObserver.cpp
@@ +304,5 @@
>      return NS_OK;
>    }
>  
> +  if (!strcmp(aTopic, "disk-space-watcher")) {
> +    if (!nsCRT::strcmp(aData, NS_LITERAL_STRING("full").get())) {

NS_LITERAL_STRING("full").Equals(aData)

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 740878 [details] [diff] [review]
v2 (b2g18)

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

The patch is OK, but you have to have a test.  At least a simple one.

r- only because of a missing test.

::: dom/src/storage/nsDOMStorage.cpp
@@ +338,5 @@
>  
>      return DOMStorageImpl::gStorageDB->RemoveAllForApp(appId, browserOnly);
> +  } else if (!strcmp(aTopic, "disk-space-watcher")) {
> +    if ((DOMStorageImpl::gStorageDB) &&
> +        (!nsCRT::strcmp(aData, NS_LITERAL_STRING("full").get()))) {

NS_LITERAL_STRING("full").Equals(aData)

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2 (inbound)

Thanks for the feedback Honza!

I think this patch addresses your comments.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Mochitests

This tests are green on b2g18 and inbound.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2 (b2g18)

Now using "NS_LITERAL_STRING().Equals()" as suggested in comment 12 and with green tests ("Mochitests" attachment).

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 743543 [details] [diff] [review]
v2 (inbound)

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

Getting closer.  r- since few details must be made clear first and to refer the review comment.

::: dom/src/storage/DOMStorageManager.cpp
@@ +568,5 @@
>      return NS_OK;
>    }
>  
> +  if (!strcmp(aTopic, "low-disk-space")) {
> +    if (mType != SessionStorage) {

I think more appropriate is to mType == LocalStorage here.

@@ +631,5 @@
> +    do_GetService("@mozilla.org/toolkit/disk-space-watcher;1");
> +  bool lowDiskSpace;
> +  diskSpaceWatcher->GetLowFreeSpace(&lowDiskSpace);
> +  if (lowDiskSpace) {
> +    if (DOMStorageObserver::Self() != nullptr) {

if (lowDiskSpace && DOMStorageObserver::Self()) {
 
?

Either init lowDiskSpace to false or check retval of GetLowFreeSpace

@@ +632,5 @@
> +  bool lowDiskSpace;
> +  diskSpaceWatcher->GetLowFreeSpace(&lowDiskSpace);
> +  if (lowDiskSpace) {
> +    if (DOMStorageObserver::Self() != nullptr) {
> +      DOMStorageObserver::Self()->Notify("low-disk-space");

If the information (GetLowFreeSpace) is available on both parent and child processes, you should just set (this->) mLowDiskSpace flag here (not call on the observer).

Otherwise (the info is avail only on the parent process), child processes cannot initialize the flag correctly and we need to solve it somehow.

Let you know, that there is always just a single DOMLocalStorageManager instance (it's getService()'d) in a process.

::: dom/src/storage/DOMStorageManager.h
@@ +35,5 @@
>    static uint32_t GetQuota();
>    // Gets (but not ensures) cache for the given scope
>    DOMStorageCache* GetCache(const nsACString& aScope) const;
>  
> +  bool LowDiskSpace() { return mLowDiskSpace; }

IsLowDiskSpace and should be a const method and also just exposed to DOMStorageCache (there is a "friend class DOMStorageCache;" section at the bottom).

@@ +89,5 @@
>    nsTHashtable<DOMStorageCacheHashKey> mCaches;
>    const nsPIDOMStorage::StorageType mType;
>  
> +  // If mLowDiskSpace is true it indicates a low device storage situation and
> +  // so no local storage writes are allowed. Session storage writes are still

s/local storage/localStorage/
same for Session storage

::: dom/src/storage/DOMStorageObserver.cpp
@@ +302,5 @@
>  
>      return NS_OK;
>    }
>  
> +  if (!strcmp(aTopic, "disk-space-watcher")) {

Is this notification received on both child and parent?

Let you know that Notify("...") calls executed on the parent process are propagated to all child processes automatically.

Maybe we should hook to this notification only on the parent process?

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 743545 [details] [diff] [review]
Mochitests

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

r+ with comments addressed.

::: dom/tests/mochitest/localstorage/test_lowDeviceStorage.html
@@ +13,5 @@
> +- Stores an item in localStorage.
> +- Checks the stored value.
> +- Emulates a low device storage situation.
> +- Gets the stored item again.
> +- Removed the stored item.

Removes

@@ +20,5 @@
> +- Stores a new value.
> +- Checks the stored value.
> +*/
> +
> +netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

No new "UniversalXPConnect" tests please.  Use SpecialPowers.  BTW, seems like you don't need it.

@@ +28,5 @@
> +  os().notifyObservers(null, "disk-space-watcher", data);
> +}
> +
> +function startTest() {
> +  // Add a test item.

Before you do this, it would be good to check on the actual state of the disk space watcher.  We don't want intermittent failures when the device is actually in a low disk space state.  But this should apply to all LS tests...  Forget it...

@@ +46,5 @@
> +     "getItem() after removing the item");
> +
> +  // Fails storing a new item.
> +  try {
> +    localStorage.setItem("newItem", "value");

put ok(false, ...) after this line to catch we were not able to force the failure.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 743554 [details] [diff] [review]
v2 (b2g18)

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

r+ with the comment addressed.

::: dom/src/storage/nsDOMStorage.cpp
@@ +579,5 @@
> +    diskSpaceWatcherService->GetLowFreeSpace(&lowFreeSpace);
> +    if (lowFreeSpace) {
> +      gStorageDB->Disable();
> +      return NS_OK;
> +    }

This must happen in nsDOMStorageDBWrapper::Init() and not here.  This is a periodically called method that ensures DB is up and this your new code will be called on and on.

::: dom/src/storage/nsDOMStorageBaseDB.h
@@ +42,4 @@
>      return gQuotaLimit * 1024;
>    }
>  
> +  bool mDisabled;

Nit: shouldn't be public.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3 (inbound)

This patch ensures that the check for device storage is done in the parent.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Mochitests

Final patch for the tests. r+ from comment 17.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 745147 [details] [diff] [review]
v3 (inbound)

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

r=honzab but please address all the comments before landing.

Sorry for the delay.

::: dom/src/storage/DOMStorageCache.cpp
@@ +194,5 @@
>  bool
>  DOMStorageCache::ProcessUsageDelta(uint32_t aGetDataSetIndex, const int64_t aDelta)
>  {
> +  // Check if we are in a low disk space situation
> +  if (aDelta > 0 && mManager && mManager->mIsLowDiskSpace) {

Here the (missing) getter has to be used.

::: dom/src/storage/DOMStorageIPC.cpp
@@ +318,5 @@
> +        do_GetService("@mozilla.org/toolkit/disk-space-watcher;1");
> +      bool lowDiskSpace = false;
> +      diskSpaceWatcher->GetLowFreeSpace(&lowDiskSpace);
> +      if (lowDiskSpace && DOMStorageObserver::Self()) {
> +        DOMStorageObserver::Self()->Notify("low-disk-space");

Instead do mParent->Observe("low-disk-space", EmptyCString()); and add a comment what you are doing.

The way you do it you send it to all child processes (huge wasting).

Please put your whole new block under the SendScopesHavingData line.

::: dom/src/storage/DOMStorageManager.h
@@ +89,5 @@
>  
> +  // If mLowDiskSpace is true it indicates a low device storage situation and
> +  // so no localStorage writes are allowed. sessionStorage writes are still
> +  // allowed.
> +  bool mIsLowDiskSpace;

Why did you remove the getter?  Please reintroduce it also according my nit comments from the last review.

::: dom/src/storage/DOMStorageObserver.cpp
@@ +68,5 @@
>    obs->AddObserver(sSelf, "profile-before-change", true);
>    obs->AddObserver(sSelf, "xpcom-shutdown", true);
>  
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    // Observe low device storage notifications. Only in the parent.

Put the comment before the |if| line please.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/projects/birch/rev/8beafe5ccc65
https://hg.mozilla.org/projects/birch/rev/2f1fe3baec7c

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out because this apparently depends on bug 861894, which was also backed out for Windows bustage.
https://hg.mozilla.org/projects/birch/rev/41ca6c75671e

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

New try run https://tbpl.mozilla.org/?tree=Try&rev=18f6afd6825d

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

http://hg.mozilla.org/projects/birch/rev/c39ceb11cb76
http://hg.mozilla.org/projects/birch/rev/69fcebb24095

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest failures.
https://hg.mozilla.org/projects/birch/rev/519b04170b2f

https://tbpl.mozilla.org/php/getParsedLog.php?id=22834849&tree=Birch

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Try was green with mochitests for B2G https://tbpl.mozilla.org/?tree=Try&rev=589fbe25f982

http://hg.mozilla.org/projects/birch/rev/02517f90a812
http://hg.mozilla.org/projects/birch/rev/ee6972ad4e34

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

And a follow-up to remove debug logs http://hg.mozilla.org/projects/birch/rev/187f533ec992 Sorry about that :\

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2 (b2g18)

Patch updated for b2g18

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Mochitests (b2g18)

Mochitests for b2g18.

The mochitests landed in birch apply in b2g18 but would fail since it is using code that does not exists in b2g18.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02517f90a812
https://hg.mozilla.org/mozilla-central/rev/ee6972ad4e34
https://hg.mozilla.org/mozilla-central/rev/187f533ec992

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9c18bee79be0
https://hg.mozilla.org/releases/mozilla-b2g18/rev/dfbd97e3abe8

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/438a3384f31f
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f38ba45624b4

Comment 33

[Jason Smith [:jsmith]]

One non-blocking followup filed in bug 880870. Other than that, looks good with tests that analyzed similar areas seen on the appcache equivalent of this bug.