Closed Bug 861920 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

All
macOS
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: ferjm)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 8 obsolete files)

1.48 KB, application/zip
Details
7.71 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
3.52 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
5.69 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
3.61 KB, patch
ferjm
: review+
Details | Diff | Splinter Review
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.
Blocks: low-storage
blocking-b2g: --- → tef?
Depends on: 853350
Summary: Avoid apps to write use localStorage while device free storage is low → Avoid apps to write in its local storage while device free storage is low
Assignee: nobody → ferjmoreno
Blocking per todays triage meeting.
blocking-b2g: tef? → tef+
Whiteboard: [status: part of low storage work, in progress]
To confirm: this is intended to land on b2g18 branch?
Attached patch WIP (obsolete) — Splinter Review
I already received positive feedback from Honza in a conversation via IRC about this approach.
Whiteboard: [status: part of low storage work, in progress] → [status: has WIP patch]
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #738531 - Attachment is obsolete: true
Attachment #739590 - Flags: review?(honzab.moz)
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.
Attached file Test app
Simple test application.
Whiteboard: [status: has WIP patch] → [status: has WIP patch, needs review, blocked on kernel change]
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.
Attachment #739590 - Flags: review?(honzab.moz)
Attached patch v2 (b2g18) (obsolete) — Splinter Review
Thanks for the feedback Honza!

This patch addresses your comments.
Attachment #739590 - Attachment is obsolete: true
Attachment #740878 - Flags: review?(honzab.moz)
Attached patch v1 (inbound) (obsolete) — Splinter Review
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.
Attachment #740883 - Flags: feedback?(honzab.moz)
Attached patch Tests (obsolete) — Splinter Review
Untested tests. I still need to set my build to run tests properly.
Attachment #740883 - Flags: feedback?(doug.turner)
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)
Attachment #740883 - Flags: feedback?(honzab.moz)
Attachment #740883 - Flags: feedback?(doug.turner)
Attachment #740883 - Flags: feedback-
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)
Attachment #740878 - Flags: review?(honzab.moz) → review-
Component: Storage → DOM
Product: Toolkit → Core
Target Milestone: --- → 1.0.1 IOT1 (10may)
Attached patch v2 (inbound) (obsolete) — Splinter Review
Thanks for the feedback Honza!

I think this patch addresses your comments.
Attachment #740883 - Attachment is obsolete: true
Attachment #743543 - Flags: review?(honzab.moz)
Attached patch Mochitests (obsolete) — Splinter Review
This tests are green on b2g18 and inbound.
Attachment #740885 - Attachment is obsolete: true
Attachment #743545 - Flags: review?(honzab.moz)
Attached patch v2 (b2g18) (obsolete) — Splinter Review
Now using "NS_LITERAL_STRING().Equals()" as suggested in comment 12 and with green tests ("Mochitests" attachment).
Attachment #740878 - Attachment is obsolete: true
Attachment #743554 - Flags: review?(honzab.moz)
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?
Attachment #743543 - Flags: review?(honzab.moz) → review-
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.
Attachment #743545 - Flags: review?(honzab.moz) → review+
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.
Attachment #743554 - Flags: review?(honzab.moz) → review+
Whiteboard: [status: has WIP patch, needs review, blocked on kernel change] → [status: patches and reviews underway]
Attached patch v3 (inbound)Splinter Review
This patch ensures that the check for device storage is done in the parent.
Attachment #743543 - Attachment is obsolete: true
Attachment #745147 - Flags: review?(honzab.moz)
Attached patch MochitestsSplinter Review
Final patch for the tests. r+ from comment 17.
Attachment #743545 - Attachment is obsolete: true
Attachment #745151 - Flags: review+
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.
Attachment #745147 - Flags: review?(honzab.moz) → review+
Whiteboard: [status: patches and reviews underway] → [status: r+'d patches, needs bug 853350]
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
Depends on: 861894
Whiteboard: [status: r+'d patches, needs bug 853350] → [status: awaiting try build, will reland]
Backed out for mochitest failures.
https://hg.mozilla.org/projects/birch/rev/519b04170b2f

https://tbpl.mozilla.org/php/getParsedLog.php?id=22834849&tree=Birch
Whiteboard: [status: awaiting try build, will reland] → [status: backed out from birch due to test failures]
And a follow-up to remove debug logs http://hg.mozilla.org/projects/birch/rev/187f533ec992 Sorry about that :\
Attached patch v2 (b2g18)Splinter Review
Patch updated for b2g18
Attachment #743554 - Attachment is obsolete: true
Attachment #748545 - Flags: review+
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.
Attachment #748546 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/02517f90a812
https://hg.mozilla.org/mozilla-central/rev/ee6972ad4e34
https://hg.mozilla.org/mozilla-central/rev/187f533ec992
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [status: backed out from birch due to test failures]
Keywords: verifyme
QA Contact: jsmith
Depends on: 880870
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.
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.