1389380 - Add test cases for storage pressure event

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement, P2)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Add test cases for storage pressure event

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

+++ This bug was initially created as a clone of Bug #1294400 +++

Implement Storage pressure event to notify UI

6.1. Storage Pressure

When the user agent notices it comes under storage pressure and it cannot free up sufficient space by clearing network storage and non-persistent boxes within site storage, then the user agent should alert the user and offer a way to clear persistent boxes.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Wait, I didn't think we want to implement storage pressure events.  Anne, is this correct?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Oh, this is to alert the user or is it an event fired to content script?

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Oh, this is to alert the user or is it an event fired to content script?

It's just notifying UI to show disk full, not a event to content script. Sorry for misleading.

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

It should be fine to cancel ni? since I've provided information.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Yea, sorry for my confusion.  There was some spec discussion about content pressure events and I was afraid maybe we were going down that path.  Thanks!

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1389380 - Add a test to verify storage-pressure is received after restarting the browser

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Move patch from https://bug1384492.bmoattachments.org/attachment.cgi?id=8900637 to this bug, since it covers the test originally using simple quota client.

Comment 8

[Tom Tung [:tt, :ttung]]

Since both bug 1294400 and bug 1361330 were fixed, I think it's good to have a test to ensure the behavior meets expectation. I'll rebase and change the patch a bit when I have time.

Comment 9

[Tom Tung [:tt, :ttung]]

I hit the assertion about QueryInterface in SimpleDB while testing this test. It seems that |nsCOMPtr<nsISimpleEnumerator> entries| [1] should be updated to |nsCOMPtr<nsIDirectoryEnumerator> entries| after bug 1462483. 

I'll modify that locally to complete rebasing the test.

[1] https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/dom/simpledb/ActorsParent.cpp#1912

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Add a test for storage pressure event

Comment 11

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #10)
> Created attachment 9004536 [details] [diff] [review]
> Add a test for storage pressure event

This patch uses the similar function as a function in bug 1389390. I'll move that function to helper.js here after the patch in bug 1389390 gets r+. Then, send it to review.

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: Add a test for storage pressure event

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: Add a test for storage pressure event

Comment 14

[Tom Tung [:tt, :ttung]]

Comment on attachment 9012881 [details] [diff] [review]
Add a test for storage pressure event

Jan, this patch is a test for storage pressure event. Could you help me to review it? Thanks!

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: Add a test for storage pressure event

Comment 16

[Tom Tung [:tt, :ttung]]

Comment on attachment 9013240 [details] [diff] [review]
Add a test for storage pressure event

Jan, this patch mainly to verify QM fires a storage pressure event in two conditions. One is an incoming data will make current usage greater than the global limit. The other is that QM found calculating limit is less than the current usage. Besides, this patch also moves function |fillOrigin()| to head.js and adds one more argument to it to reuse the code. Could you help me review the patch? Thanks!

I checked either |reset()| or |clear()| is called after either |setGlobalLimit()| and |resetGlobalLimit()| and tried to make this patch more easier to be read.

Comment 17

[Jan Varga [:janv]]

Attachment: suggestions

Comment 18

[Jan Varga [:janv]]

Comment on attachment 9013240 [details] [diff] [review]
Add a test for storage pressure event

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

See the "suggestions" patch, I'll provide more details later.

Comment 19

[Jan Varga [:janv]]

Comment on attachment 9013240 [details] [diff] [review]
Add a test for storage pressure event

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

::: dom/quota/test/unit/test_storagePressure.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +Cu.import("resource://gre/modules/Services.jsm");

I think we can put this to head.js, I plan to use it in head.js in future.

@@ +10,5 @@
> +  // local variables & functions
> +  const globalLimitKB = 2;
> +
> +  const topic = "QuotaManager::StoragePressure";
> +  let promise_resolve = Promise.resolve;

Assigning |Promise.resolve| is not needed.
Anyway, I think all this observer and promise logic deserves a separate function.
Having various pieces of this logic spread across the test is not very readable.
There's also another problem. We want to catch the storage pressure event at specific time (just before writing a byte or reducing the global limit).
If we don't do that, then test will pass even when the event fires much sooner.

@@ +39,5 @@
> +    let spec = "https://persist.example.com";
> +    request = persist(getPrincipal(spec));
> +    await requestFinished(request);
> +
> +    await fillOrigin(getPrincipal(spec), globalLimitKB * 1024);

I don't think we should have 2 fillOrigin calls here. fillOrigin needs to open, write, close each time you call it.
Also, if you close the database, then next time you need to call seek(), if you want to append data to the database/file.
You avoided this problem by using a different origin, but it would be nicer to have just one origin.

@@ +54,5 @@
> +      request = reset();
> +      await requestFinished(request);
> +
> +      // Ensuring QuotaManager::CheckTemporaryStorageLimits() is called.
> +      request = initOrigin(getPrincipal(spec), "default");

Use initTempStorage(), then you don't need the comment above

@@ +62,5 @@
> +
> +      await fillOrigin(getPrincipal(spec), 1, /* expectStoragePressure */ true);
> +    }
> +
> +    info("Waitting for a storage pressure event");

Technically, the event already fired, so we are not waiting for it here.

Comment 20

[Jan Varga [:janv]]

Comment on attachment 9013240 [details] [diff] [review]
Add a test for storage pressure event

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

::: dom/quota/test/unit/test_storagePressure.js
@@ +28,5 @@
> +
> +  info("Running tests");
> +
> +  let storagePressureEventFired;
> +  for (let storagePressureFiredInInit of [false, true]) {

Maybe we can get rid of this loop and just do those two tests one by one like Shawn did in his original patch.
In other words, after writing 1 byte and checking that the pressure event was fired, we can just reduce the global limit and expect another event.

Comment 21

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #19)
Thanks for the review!

> > +Cu.import("resource://gre/modules/Services.jsm");
> 
> I think we can put this to head.js, I plan to use it in head.js in future.

Will do

> @@ +10,5 @@
> > +  // local variables & functions
> > +  const globalLimitKB = 2;
> > +
> > +  const topic = "QuotaManager::StoragePressure";
> > +  let promise_resolve = Promise.resolve;
> 
> Assigning |Promise.resolve| is not needed.
> Anyway, I think all this observer and promise logic deserves a separate
> function.
> Having various pieces of this logic spread across the test is not very
> readable.
> There's also another problem. We want to catch the storage pressure event at
> specific time (just before writing a byte or reducing the global limit).
> If we don't do that, then test will pass even when the event fires much
> sooner.

I see

> @@ +39,5 @@
> > +    let spec = "https://persist.example.com";
> > +    request = persist(getPrincipal(spec));
> > +    await requestFinished(request);
> > +
> > +    await fillOrigin(getPrincipal(spec), globalLimitKB * 1024);
> 
> I don't think we should have 2 fillOrigin calls here. fillOrigin needs to
> open, write, close each time you call it.
> Also, if you close the database, then next time you need to call seek(), if
> you want to append data to the database/file.
> You avoided this problem by using a different origin, but it would be nicer
> to have just one origin.

I see.

> > +      // Ensuring QuotaManager::CheckTemporaryStorageLimits() is called.
> > +      request = initOrigin(getPrincipal(spec), "default");
> 
> Use initTempStorage(), then you don't need the comment above

Indeed. Will do.

> > +    info("Waitting for a storage pressure event");
> 
> Technically, the event already fired, so we are not waiting for it here.

Yeah, should be something like "checking" or "verifying"


(In reply to Jan Varga [:janv] from comment #20)
> > +  for (let storagePressureFiredInInit of [false, true]) {
> 
> Maybe we can get rid of this loop and just do those two tests one by one
> like Shawn did in his original patch.
> In other words, after writing 1 byte and checking that the pressure event
> was fired, we can just reduce the global limit and expect another event.

Will do

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: Add a test for storage pressure event

Addressed comments and checked the suggestion one time.

Will check one time more and re-check whether addressing all the comments again before sending to review

Comment 23

[Tom Tung [:tt, :ttung]]

Attachment: Add a test for storage pressure event

Comment 24

[Tom Tung [:tt, :ttung]]

Comment on attachment 9014338 [details] [diff] [review]
Add a test for storage pressure event

Jan, I addressed the comments and aligned with the suggestion. However, I changed a few info messages and used initTemporaryStorage() to set the set the limits rather than clear().

Could you help me to review it? Thanks!

Comment 25

[Jan Varga [:janv]]

Comment on attachment 9014338 [details] [diff] [review]
Add a test for storage pressure event

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

::: dom/quota/test/unit/test_storagePressure.js
@@ +6,5 @@
> +function awaitStoragePressure()
> +{
> +  let promise_resolve;
> +
> +  let promise = new Promise(resolve => {

Nit: sorry, I haven't noticed this earlier, we should probably keep the same style here as elsewhere:
let promise = new Promise(function(resolve) {

@@ +27,5 @@
> +
> +/**
> + * This test is mainly to check the storage pressure event. It verifies storage
> + * pressure event is fired when the global usage is over the global limit by
> + * increasing the global usage or reducing the global limit.

This could be a bit more explanatory:
/**
 * This test is mainly to verify that the storage pressure event is fired when
 * the eviction process is not able to free some space when a quota client
 * attempts to write over the global limit or when the global limit is reduced
 * below the global usage.
 */

@@ +40,5 @@
> +  info("Setting limits");
> +
> +  setGlobalLimit(globalLimitKB);
> +
> +  let request = initTemporaryStorage();

I like this, good catch :)

@@ +43,5 @@
> +
> +  let request = initTemporaryStorage();
> +  await requestFinished(request);
> +
> +  info("Testing storage pressure by writing over the global limit");

Nit: I would move this info:
-info("Testing storage pressure by writing over the global limit");

 info("Persisting and filling an origin");

...

+info("Testing storage pressure by writing over the global limit");

 info("Storing one more byte to get the storage pressure event while writing");

Comment 26

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1389380 - Add a test for storage pressure event; r=janv

Thanks for the reviews!

Comment 27

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84d56a9b30cee06d7bc62f5a3af5c94e9ac1bb76

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6de689423ff6
Add a test for storage pressure event; r=janv

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6de689423ff6

Comment 30

[Tom Tung [:tt, :ttung]]

Close the bug since it's already lived in m-c and I don't think it needs to be leave-open