Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 Build ID: 20170725030209 [Affected versions]: Nightly 56.0a1 [Affected platforms]: Windows 10 x64 [Steps to reproduce]: 1.Install FF latest Nightly 56.0a1 on a partition with 2GB available 2.Set "browser.storageManager.pressureNotification.testStorageLimitKB" to 2000000 3.Restart the browser 4.Store 1.3 MB persistent data 5.Minimize disk space until 2.34 MB 6.Restart the browser [Expected result]: Firefox should store max. 50% of disk space Notification bar should be displayed [Actual result]: No notification bar displayed

NI myself, we will triage this in Storage project meeting.

I will take a look today. Thanks for reporting.

(In reply to roxana.leitan@softvision.ro [PTO from 08-04-2017 until 08-21-2017] from comment #0) > Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 > Firefox/56.0 > Build ID: 20170725030209 > > [Affected versions]: > Nightly 56.0a1 > > [Affected platforms]: > Windows 10 x64 > > [Steps to reproduce]: > 1.Install FF latest Nightly 56.0a1 on a partition with 2GB available > 2.Set "browser.storageManager.pressureNotification.testStorageLimitKB" to > 2000000 > 3.Restart the browser > 4.Store 1.3 MB persistent data > 5.Minimize disk space until 2.34 MB Step 5, Firefox still consider available space 1GB, can you clarify how do you minimize disk space? Through Firefox (write more data using Firefox) or consume more space in an another application. If this has NOT done by Firefox but through other applications, since Firefox doesn't always dynamically poll available space. Can you confirm the issue? > 6.Restart the browser > > [Expected result]: > Firefox should store max. 50% of disk space > Notification bar should be displayed > > [Actual result]: > No notification bar displayed

If Step 5, the disk space consumes by other application, we did not cover about this problem now since the original design doesn't handle that. However, I noticed that if Step 5, disk space consumed by Firefox, I found that the warning shown up until only 200MB disk space left. This is an unexpected behavior.

Hi Brindusa, As Roxana is taking off these two weeks, is it possible that you could help the questions in comment 4 be answered earlier so we get a chance to move this forward before 57 mid-nightly? Thanks!

(In reply to Shawn Huang [:shawnjohnjr] from comment #4) > If Step 5, the disk space consumes by other application, we did not cover > about this problem now since the original design doesn't handle that. We need to implement a function periodically to update current space left, unless |QuotaManager::EnsureOriginIsInitializedInternal| is called, which is getting the current disk space left only when QuotaManager is initialized. But it will be a question how often we update disk space, considering |MaybeUpdateSize| may be executed on different threads and whenever we update the disk space we produced extra locking overhead. We used to discuss with Jan and he mentioned that this problem before. I guess this is why the default mechanism is implemented yet due to performance concern. > However, I noticed that if Step 5, disk space consumed by Firefox, I found > that the warning shown up until only 200MB disk space left. This is an > unexpected behavior. For the previous comment I did, I did further investigation and know why there is an unexpected behavior. If only 2GB left in disk space, global limit is supposed to be 1GB (50% of available disk), and we shall expect warning notification dialog shown up around 90%. However, step 2 (Set "browser.storageManager.pressureNotification.testStorageLimitKB" to 2000000) affects the behavior since storage limit has been configured to 2GB. The code will check 2GB if the property has been set. Then warning notification dialog is shown around 200MB left. This matched the observed behavior. See: [1] http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/dom/quota/ActorsParent.cpp#5122 [2] http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/dom/quota/ActorsParent.cpp#1546 [3] http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/dom/quota/ActorsParent.cpp#2659

(In reply to Shawn Huang [:shawnjohnjr] from comment #6) > (In reply to Shawn Huang [:shawnjohnjr] from comment #4) > > If Step 5, the disk space consumes by other application, we did not cover > > about this problem now since the original design doesn't handle that. > > We used to discuss with Jan and he mentioned that this problem before. I > guess this is why the default mechanism is implemented yet due to typo: why the default mechanism is "NOT" implemented yet > performance concern.

(In reply to Shawn Huang [:shawnjohnjr] from comment #6) > (In reply to Shawn Huang [:shawnjohnjr] from comment #4) > > If Step 5, the disk space consumes by other application, we did not cover > > about this problem now since the original design doesn't handle that. > > We need to implement a function periodically to update current space left, > unless |QuotaManager::EnsureOriginIsInitializedInternal| is called, which is > getting the current disk space left only when QuotaManager is initialized. > But it will be a question how often we update disk space, considering > |MaybeUpdateSize| may be executed on different threads and whenever we > update the disk space we produced extra locking overhead. > We used to discuss with Jan and he mentioned that this problem before. I > guess this is why the default mechanism is implemented yet due to > performance concern. > > > However, I noticed that if Step 5, disk space consumed by Firefox, I found > > that the warning shown up until only 200MB disk space left. This is an > > unexpected behavior. > > For the previous comment I did, I did further investigation and know why > there is an unexpected behavior. > If only 2GB left in disk space, global limit is supposed to be 1GB (50% of > available disk), and we shall expect warning notification dialog shown up > around 90%. Correction is nearly 100%. But it depends on how much you're going to write.

(In reply to roxana.leitan@softvision.ro [PTO from 08-04-2017 until 08-21-2017] from comment #0) > [Steps to reproduce]: > 1.Install FF latest Nightly 56.0a1 on a partition with 2GB available > 2.Set "browser.storageManager.pressureNotification.testStorageLimitKB" to > 2000000 > 3.Restart the browser > 4.Store 1.3 MB persistent data > 5.Minimize disk space until 2.34 MB If disk space is consumed by other applications in Step 5., because we don't always get the latest available disk size whenever writing data. I wonder how aggressively we should get the latest size. We currently only get available disk size when QuotaManager do initialized, that happened only when persisted and the very first time write data into persistent space. Mark, do you have any thought from UX side? > 6.Restart the browser > > [Expected result]: > Firefox should store max. 50% of disk space > Notification bar should be displayed

Adrian, please take a look on this bug.

Ideally we should keep track of the latest available size and show the notification warning accordingly, but if that could undermine Firefox's performance then we should compromise with the fact that notification bar won't show immediately. (Might need to set an interval to check the available size though, or set a special condition that would trigger the checking operation.) (In reply to Shawn Huang [:shawnjohnjr] from comment #9) > (In reply to roxana.leitan@softvision.ro [PTO from 08-04-2017 until > 08-21-2017] from comment #0) > > [Steps to reproduce]: > > 1.Install FF latest Nightly 56.0a1 on a partition with 2GB available > > 2.Set "browser.storageManager.pressureNotification.testStorageLimitKB" to > > 2000000 > > 3.Restart the browser > > 4.Store 1.3 MB persistent data > > 5.Minimize disk space until 2.34 MB > > If disk space is consumed by other applications in Step 5., because we don't > always get the latest available disk size whenever writing data. > I wonder how aggressively we should get the latest size. > We currently only get available disk size when QuotaManager do initialized, > that happened only when persisted and the very first time write data into > persistent space. > > Mark, do you have any thought from UX side? > > > 6.Restart the browser > > > > [Expected result]: > > Firefox should store max. 50% of disk space > > Notification bar should be displayed

(In reply to Mark Liang(:mark_liang) from comment #11) > Ideally we should keep track of the latest available size and show the > notification warning accordingly, but if that could undermine Firefox's > performance then we should compromise with the fact that notification bar > won't show immediately. (Might need to set an interval to check the > available size though, or set a special condition that would trigger the > checking operation.) I'm checking telemetry data to see how we can proceed this problem.

Session lengths by OS https://sql.telemetry.mozilla.org/queries/48#81 It looks like many people don't close Firefox less than one day. But I also saw that large portion keep browsing session for more than one day.

So, if my understanding is correct, current architecture doesn't allow us (in a performance fashionable sense) to be able to always know the exact size on disk, unless the browser is restart. Nowadays, with the hardware advancements, I reckon we are less likely to bounce on this issue, however, my first thought when encountering situations like this is: consistency. So with this in mind, even if we compromise for performance sake, we still should find a middle ground, something like: re-check size on disk once every x hrs. when browser has not been restart.

(In reply to Adrian Florinescu [:AdrianSV] from comment #14) > So, if my understanding is correct, current architecture doesn't allow us > (in a performance fashionable sense) to be able to always know the exact > size on disk, unless the browser is restart. Nowadays, with the hardware > advancements, I reckon we are less likely to bounce on this issue, however, > my first thought when encountering situations like this is: consistency. So > with this in mind, even if we compromise for performance sake, we still > should find a middle ground, something like: re-check size on disk once > every x hrs. when browser has not been restart. It is doable to re-check available disk space every x hrs. I'm trying to finding telemetry data to figure out how we choose this value. But can you confirm in Step 5, "Minimize disk space until 2.34 MB", this has been done by a non-Firefox application? I just want to make sure this is the right problem we're attacking with.

I will assume the answer we had in Comment 15 is disk space is reduced by a non-Firefox application, since Bug 1376444 Comment 17 shows a notification prompt.

(In reply to Shawn Huang [:shawnjohnjr] from comment #16) > I will assume the answer we had in Comment 15 is disk space is reduced by a > non-Firefox application, since Bug 1376444 Comment 17 shows a notification > prompt. Yup, I confirmed and that's correct, the disk minimize is done from outside FF, so your assumption is correct.

I've discussed with module owner Jan about this issue in the Storage bi-weekly meeting. He mentioned that this bug doesn't look like P1 blocker. As Comment 0 described that expectation is to see notification after browser restart (Step 6). And this is the only thing we need to fix. Regarding my comments in Comment 15, which targets on different problems, and Jan thought it's not a good idea to implement polling available disk space inside QuotaManager as we have discussed in Comment 11, Comment 14, it should be some other standalone services (ex:LowDiskWatcher) to notify QuotaManager to inform users. So we probably won't fix this scenario for QuotaManager. What impacts users should be less since operating system still shows low disk warning, if other applications continue to reduce disk space. Since Comment 0 mentioned restarting the browser in Step 6, which means Firefox will get the latest disk available, then Firefox shall send storage pressure to observer whenever QuotaManager is initialized. This is our conclusion and targets to this bug right now.

If we have to poll here (and not use inotify (or something newer on Linux?) or whatever on other OSes) let's please not just have a timer but do a timer only when idle like what was added in bug 1358476.

Jan, I'm not sure I need to update disk space periodically in this bug since we've had discussion about this in the meeting. After I saw Andrew replied the bug, it seems I need to fix that? If we really need to fix that, can I use the suggestion in Comment 19 to update disk space? I can think the drawback is to hold an extra lock (that will be the major concern) to update mTemporaryStorageLimit via GetTemporaryStorageLimit() when Idle.

No, this bug doesn't need to update internal limits. I just reported to Andrew about what we talked at storage meeting, about the other issue that we might need to fix in future (which will need to update internal limits). He also thinks that a simple timer is not ideal in that case.

This is just one WIP patch not completely finish yet.

Per comment 18 mainly, moving to P2 as the problem scope and impact reduced much more , but we still want to see this in 57.

I'm looking into writing tests for this case, however it seems hard to restart browsers in mochitest or browser test. So maybe I should figure out some other ways to do similar things.

(In reply to Shawn Huang [:shawnjohnjr] from comment #25) > I'm looking into writing tests for this case, however it seems hard to > restart browsers in mochitest or browser test. So maybe I should figure out > some other ways to do similar things. Maybe I can try reset QM instead of actually restarting browsers.

Yes, reset() emulates a browser restart.

Comment on attachment 8900641 [details] [diff] [review] Bug 1384492 - Part 1: Notify storage pressure in CheckTemporaryStorageLimits Review of attachment 8900641 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +3038,5 @@ > > if (!sizeToBeFreed) { > MutexAutoUnlock autoUnlock(quotaManager->mQuotaMutex); > > + quotaManager->NotifyStoragePressure(); This is not a problem of this patch, but we unlock too early here. mTemporaryStorageUsage can be accessed only on the I/O thread or on some other thread but protected by the mutex. I think we should assign mTemporaryStorageUsage to a local variable while still being protected by the mutex and then pass it to NotifyStoragePressure(usage). @@ +5285,5 @@ > } > > +void > +QuotaManager::NotifyStoragePressure() > +{ This could assert that the mutex is not acquired since we are going to call dispatch() ::: dom/quota/QuotaManager.h @@ +103,5 @@ > > private: > class ShutdownRunnable; > class ShutdownObserver; > + class StoragePressureRunnable; ShutdownRunnable and ShutdownObserver are declared here because for example ShutdownObserver calls a private method QuotaManager::Shutdown I think StoragePressureRunnable doesn't have to be declared here, it can be in the anon namespace.

Comment on attachment 8900637 [details] [diff] [review] Bug 1384492 - Part 2: Add a test to verify storage-pressure is received after restarting the browser Review of attachment 8900637 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/test/unit/test_pressure.js @@ +31,5 @@ > + "http://foo5.com", // 10 MB > + "http://foo6.com", // 10 MB > + ]; > + > + for (let i = 0; i < urlsToFillUpDisk.length; i++) { Is it necessary to do it in 4 separate loops ? @@ +44,5 @@ > + connectionsToBeFilled.push(connectionToBeFilled); > + } > + > + for (let i = 0; i < urlsToFillUpDisk.length; i++) { > + let bytesToBeWritten = groupLimitKB * 1024; const ? @@ +48,5 @@ > + let bytesToBeWritten = groupLimitKB * 1024; > + > + let request = > + connectionsToBeFilled[i].write(new ArrayBuffer(bytesToBeWritten)); > + request.callback = continueToNextStepSync; Should we test request.resultCode here ? The last origin is supposed to fail, right ? @@ +61,5 @@ > +} > + > +function* testSteps() > +{ > + let receivedPressureEvent, removeObserver = false; declare separately please @@ +66,5 @@ > + > + setGlobalLimit(globalLimitKB); > + > + Services.obs.addObserver(function observer(subject, topic) { > + receivedPressureEvent = true; Hm, maybe it would be better to count these notifications instead of setting/checking a bool. @@ +79,5 @@ > + clear(continueToNextStepSync); > + yield undefined; > + > + yield* fillUpDisk(); > + // Fill up disk, now usage is equal to global limit s/Fill/Filled ? @@ +85,5 @@ > + > + receivedPressureEvent = false; > + removeObserver = true; > + > + // Make sure global limit is less than usage. It would be nice to mention that now you want to test the situation when something else fills the disk even more. @@ +86,5 @@ > + receivedPressureEvent = false; > + removeObserver = true; > + > + // Make sure global limit is less than usage. > + setGlobalLimit(globalLimitKB-1); add spaces: globalLimitKB-1 -> globalLimitKB - 1 add blank line here @@ +92,5 @@ > + reset(continueToNextStepSync); > + yield undefined; > + > + initOrigin(getPrincipal("http://foo1.com"), "default", continueToNextStepSync); > + yield undefined; add blank line here ::: dom/quota/test/unit/xpcshell.ini @@ +28,5 @@ > [test_obsoleteOriginAttributesUpgrade.js] > [test_originAttributesUpgrade.js] > [test_persist.js] > [test_persist_groupLimit.js] > +[test_pressure.js] This sounds too generic, test_storagePressure.js ?

Comment on attachment 8902601 [details] [diff] [review] Bug 1384492 - Part 1: Notify storage pressure in CheckTemporaryStorageLimits Review of attachment 8902601 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +1384,5 @@ > + , mUsage(aUsage) > + { } > + > +private: > + ~StoragePressureRunnable() Nit: ~StoragePressureRunnable() { } can be written as: ~StoragePressureRunnable() = default; @@ +3012,5 @@ > uint64_t sizeToBeFreed = > quotaManager->LockedCollectOriginsForEviction(delta, locks); > > if (!sizeToBeFreed) { > + uint64_t usage = quotaManager->mTemporaryStorageUsage; nit: add a blank line here @@ +3017,3 @@ > MutexAutoUnlock autoUnlock(quotaManager->mQuotaMutex); > > + quotaManager->NotifyStoragePressure(usage); nit: add a blank line @@ +5265,5 @@ > +QuotaManager::NotifyStoragePressure(uint64_t aUsage) > +{ > + mQuotaMutex.AssertNotCurrentThreadOwns(); > + > + // Notify pressure event. Nit: I think this comment is now useless, the name of the method is self explanatory.

Comment on attachment 8904882 [details] [diff] [review] Bug 1384492 - Notify storage pressure in CheckTemporaryStorageLimits, r=janv Review of attachment 8904882 [details] [diff] [review]: ----------------------------------------------------------------- Looks good

Thank you, Jan. I will land the fix first.

Pushed by shuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/35cf15dcadbf Notify storage pressure in CheckTemporaryStorageLimits. r=janv

I've discussed with Francis. He hope I move Part 2 patch to Bug 1389380 since we don't know when simple quota client will be landed. It will be simpler for revealing the completeness of all related blockers.

Removing leave-open keyword from resolved bugs, per :sylvestre.