Closed Bug 1389390 Opened 5 years ago Closed 4 years ago

Add a test to verify eviction works fine on persisted origin

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: tt, Assigned: tt)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

6.16 KB, patch
Details | Diff | Splinter Review
4.32 KB, patch
janv
: review+
Details | Diff | Splinter Review
We decide to move the test in bug 1298329 to here.
Blocks: 1254428
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee: ttung → nobody
Status: ASSIGNED → NEW
Assignee: nobody → shes050117
Status: NEW → ASSIGNED
Priority: P2 → P3
Attachment #8896120 - Attachment is obsolete: true
Attachment #9004273 - Attachment is obsolete: true
Attachment #9004274 - Attachment is obsolete: true
Comment on attachment 9004482 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

Hi Jan,

I add this test to try to verify the persisted origin won't be evicted, but general origin will.

Could you help me to review that? Thanks!
Attachment #9004482 - Flags: review?(jvarga)
Comment on attachment 9004482 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

Drop the review request for now. I'll revise it based on the patch in bug 1493908 and then send a review request again.
Attachment #9004482 - Flags: review?(jvarga)
Revise the patch:
- Using requestFinished
- Rebase on the top of patch in bug 1493908
Attachment #9004482 - Attachment is obsolete: true
Comment on attachment 9011830 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

Jan, I rebase this patch based on your patch in Bug 1493908. This patch tests two eviction mechanism in QM.

The function testEviction() store the given origin at first to make it become the oldest origin. Then, store four other origins to full the disk so that the eviction will be triggered by storing one byte more in another origin. At the end of this function, it checks the directory of oldest/given origin to see whether it was evicted. This test expected that if the oldest origin is not a persisted origin, then it should be evicted. If it's, then it shouldn't be evicted.
Attachment #9011830 - Flags: review?(jvarga)
Comment on attachment 9011830 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

Based on Bug 1493908 comment 6, it seems that this patch might need to use async/await as well.
Attachment #9011830 - Flags: review?(jvarga)
Using async/await
Attachment #9011830 - Attachment is obsolete: true
Priority: P3 → P2
Attachment #9012098 - Attachment is obsolete: true
Attachment #9012803 - Attachment is obsolete: true
Jan, I think this patch is ready to be reviewed. Could you review that? Thanks!

This patch adds a test to verify a temporary storage is evicted when the usage is about to exceed the limit and a persisted storage isn't in the same condition.
Attachment #9012804 - Attachment is obsolete: true
Attachment #9012808 - Flags: review?(jvarga)
Working on it, I'm almost done.
Attached patch interdiffSplinter Review
I think the basic idea is very simple (if the oldest origin is persisted then it shouldn't be evicted) so the test can look much simpler too.
I'll send review comments later.
Comment on attachment 9012808 [details] [diff] [review]
Implement a test to verify the behavior of eviction.

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

::: dom/quota/test/unit/test_eviction.js
@@ +6,5 @@
> +const groupLimitKB = 10 * 1024;
> +// The group limit is calculated as 20% of the global limit.
> +const globalLimitKB = groupLimitKB * 5;
> +
> +async function openSimpleDBWithSize(origin, size) {

The name of this function indicates that a database will be opened and returned in that state. However, it does something else. It opens a database, writes data, closes the database and returns nothing. I would rather name it fillOrigin().

@@ +11,5 @@
> +  let database = getSimpleDatabase(getPrincipal(origin));
> +  let request = database.open("dummyName");
> +  await requestFinished(request);
> +
> +  let buffer = getRandomBuffer(size);

getRandomBuffer() is very slow for big sizes, I don't think random data is important here. getBuffer() should be enough.

@@ +26,5 @@
> +}
> +
> +async function testEviction(originToTest, dirToTest, evictionExpected) {
> +  info("Step 1: Storing a origin with full usage");
> +  await openSimpleDBWithSize(originToTest, groupLimitKB * 1024);

This 1st origin can be filled in one loop together with those 4 origins below.

@@ +33,5 @@
> +    "https://group1.example1.com",
> +    "https://group2.example2.com",
> +    "https://group3.example3.com",
> +    "https://group4.example4.com"
> +  ];

There's obviously a simple pattern, so we can dynamically create these urls in a loop.

@@ +41,5 @@
> +  for (let origin of originsToFullDisk) {
> +    await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> +  }
> +
> +  info("Step 3: Attempting to trigger eviction");

"Attempting" indicates that it may or may not happen. The thing is that eviction will be triggered no matter what.
If the oldest origin is not persisted then the first origin will be evicted.
If the oldest origin is persisted then the second origin will be evicted.

@@ +42,5 @@
> +    await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> +  }
> +
> +  info("Step 3: Attempting to trigger eviction");
> +  const originMaybeTriggerEviction = "http://url.to.trigger.com";

dtto

@@ +43,5 @@
> +  }
> +
> +  info("Step 3: Attempting to trigger eviction");
> +  const originMaybeTriggerEviction = "http://url.to.trigger.com";
> +  await openSimpleDBWithSize(originMaybeTriggerEviction, 1);

This 6th origin can be filled in one loop together with those 4 origins above.

@@ +49,5 @@
> +  info("Step 4: Verifying if the oldest origin was evicted or not");
> +  let oldestOriginDir = getRelativeFile(dirToTest);
> +  ok(!oldestOriginDir.exists() == evictionExpected,
> +     evictionExpected ? "The origin " + originToTest + " was evicted."
> +                      : "The origin " + originToTest + " was not evicted.");

It would be safer to check all origin directories.

@@ +59,5 @@
> +    {
> +      origin: "http://best-effort.example.com",
> +      directory: "storage/default/http+++best-effort.example.com",
> +      evictionExpected: true,
> +      persisted: false,

persisted is redundant since it can be expressed as !evictionExpected

@@ +69,5 @@
> +      persisted: true,
> +    }
> +  ];
> +
> +  setGlobalLimit(globalLimitKB);

setGlobalLimit and resetGlobalLimit should be always paired with clear() or reset(), otherwise it won't take any effect.

::: dom/quota/test/unit/xpcshell.ini
@@ +22,5 @@
>  
>  [test_basics.js]
>  [test_bad_origin_directory.js]
>  [test_defaultStorageUpgrade.js]
> +[test_eviction.js]

I don't think this a generic test for eviction. I would name it test_persist_eviction.js
Attachment #9012808 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #16)
Thanks for the review! I still have a lot of things to learn.

Some comments can be applied on patches in bug 1389378 and bug 1389380. I will drop the requests of them and revise them again as well.

I wanted to make this test generic so that it could be extended easily, but it made the test lose its efficacy. Also, that's no any reason to do that now, I could make them generic if it's needed in the future.

> > +async function openSimpleDBWithSize(origin, size) {
> 
> The name of this function indicates that a database will be opened and
> returned in that state. However, it does something else. It opens a
> database, writes data, closes the database and returns nothing. I would
> rather name it fillOrigin().

I see. OpenXXXDatabase() does imply to return the handle of a database.

> @@ +11,5 @@
> > +  let buffer = getRandomBuffer(size);
> 
> getRandomBuffer() is very slow for big sizes, I don't think random data is
> important here. getBuffer() should be enough.

I see.

I saw test_simpledb.js use that, but it's because it compared the content of databases after that. I also checked the functions in "unit/head.js", but I neglected to check functions in "head-shared.js". I should search all the function name and compare the relatives before using them.

> > +  await openSimpleDBWithSize(originToTest, groupLimitKB * 1024);
> 
> This 1st origin can be filled in one loop together with those 4 origins
> below.

I considered about that. I was worry that it's hard to point out the certain order of origin without a magic number, but it seems that it may not be as bad as I thought.

> > +    "https://group4.example4.com"
> > +  ];
> 
> There's obviously a simple pattern, so we can dynamically create these urls
> in a loop.

That's a good point, I should have noticed that.
> +  info("Step 3: Attempting to trigger eviction");
> 
> "Attempting" indicates that it may or may not happen. The thing is that
> eviction will be triggered no matter what.
> If the oldest origin is not persisted then the first origin will be evicted.
> If the oldest origin is persisted then the second origin will be evicted.

Indeed, I was trying to focus on the testing origin. I guess I should have written: "Attempting to evict XXX origin".

> @@ +42,5 @@
> > +    await openSimpleDBWithSize(origin, groupLimitKB * 1024);
> > +  }
> > +
> > +  info("Step 3: Attempting to trigger eviction");
> > +  const originMaybeTriggerEviction = "http://url.to.trigger.com";
> 
> dtto
> 
> @@ +43,5 @@
> > +  }
> > +
> > +  info("Step 3: Attempting to trigger eviction");
> > +  const originMaybeTriggerEviction = "http://url.to.trigger.com";
> > +  await openSimpleDBWithSize(originMaybeTriggerEviction, 1);
> 
> This 6th origin can be filled in one loop together with those 4 origins
> above.

I was trying to reduce the size of buffer written into the disk. 

> @@ +49,5 @@
> > +  info("Step 4: Verifying if the oldest origin was evicted or not");
> > +  let oldestOriginDir = getRelativeFile(dirToTest);
> > +  ok(!oldestOriginDir.exists() == evictionExpected,
> > +     evictionExpected ? "The origin " + originToTest + " was evicted."
> > +                      : "The origin " + originToTest + " was not evicted.");
> 
> It would be safer to check all origin directories.

That's true.

> @@ +59,5 @@
> > +      persisted: false,
> 
> persisted is redundant since it can be expressed as !evictionExpected

That's correct.

> @@ +69,5 @@
> > +      persisted: true,
> > +    }
> > +  ];
> > +
> > +  setGlobalLimit(globalLimitKB);
> 
> setGlobalLimit and resetGlobalLimit should be always paired with clear() or
> reset(), otherwise it won't take any effect.

I didn't call rest() at the end of this test (I assumed every test call clear() at the beginning of themselves, but I found it's problematic to assume that), but here clear() was called after that in the for-loop (or it's because it might confuse others since it's not right after that?).

> ::: dom/quota/test/unit/xpcshell.ini
> @@ +22,5 @@
> >  
> >  [test_basics.js]
> >  [test_bad_origin_directory.js]
> >  [test_defaultStorageUpgrade.js]
> > +[test_eviction.js]
> 
> I don't think this a generic test for eviction. I would name it
> test_persist_eviction.js

Sure
Addressed comments, merged the inter-diff patch and modified commit message
Attachment #9012808 - Attachment is obsolete: true
Comment on attachment 9013231 [details] [diff] [review]
Bug 1389390 - Add a test to verify a persisted origin won't be evicted; r=janv

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

Looks good, thanks!
Attachment #9013231 - Flags: review+
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed04fa44d07
Add a test to verify a persisted origin won't be evicted; r=janv
https://hg.mozilla.org/mozilla-central/rev/3ed04fa44d07
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.