Closed Bug 1493908 Opened 1 year ago Closed 1 year ago

Add a test to verify that persisted origins are not constrained by the group limit

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: janv, Assigned: janv)

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Tom, can you take a look at this patch ?
I would like to use it as a template for other tests that you are going to add in bug 1389390 and bug 1389378.
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Attachment #9011717 - Flags: feedback?(shes050117)
Comment on attachment 9011717 [details] [diff] [review]
patch

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

LGTM, I'll revise the patches in bug 1389390 and bug 1389378 based on this patch. Thanks!
Attachment #9011717 - Flags: feedback?(shes050117) → feedback+
Attachment #9011717 - Flags: review?(bugmail)
Comment on attachment 9011717 [details] [diff] [review]
patch

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

I'm not really comfortable with adding more tests that use the generator idiom.  There was an argument for deferring adoption of async/await (supported since Fx52) until we fixed microtasks (bug 1193394 since Firefox 60), but it's fixed and as of the recent Fx62 release, ESR52 is now unsupported and we have ESR60, so I think we can/should make the transition to async/await.

In particular I've always been concerned about how the generator idiom allows the test to desynchronize if iter.next() is accidentally called twice in some spots, which can in turn paper over failures to invoke iter.next().  Promises only resolve/reject once which helps avoid systemic test driver issues like this.  (Also, the fact that `await` throws rejections results in major error-handling improvements.)

The tests a directory up in dom/quota/test/ already are using a Promise-based idiom for browser tests and mochitests.  Is there a reason we can't port/copy the xpcshell wrapper changes so we can at least ensure new tests are using async/await so we don't make the problem worse?
Attachment #9011717 - Flags: review?(bugmail) → review-
Priority: -- → P2
(In reply to Andrew Sutherland [:asuth] from comment #3)
> The tests a directory up in dom/quota/test/ already are using a
> Promise-based idiom for browser tests and mochitests.  Is there a reason we
> can't port/copy the xpcshell wrapper changes so we can at least ensure new
> tests are using async/await so we don't make the problem worse?

Only browser tests are using promises because they have to. Mochitests and xpcshell tests are still using generators.

I don't want to convert all the tests at the moment, but I could change testSteps to be an async function/generator in this new test and then change all |yield* requestFinished(request)| to await |requestFinished2(request)|. requestFinished2 would return a new promise. I'll also try to figure out a better name for requestFinished2.

Andrew, would that be enough for now ?
(In reply to Jan Varga [:janv] from comment #4)
> requestFinished2 would return a new promise. I'll also try to figure out a
> better name for requestFinished2.

Given that there's only one test that is using requestFinished, I can replace existing requestFinished with a promise based version and update test_simpledb.js to use |await| too.
Yes, that would be great.  Having new tests be async/await-based is a great first step.
Attachment #9011717 - Attachment is obsolete: true
Attachment #9012076 - Flags: review?(bugmail)
Comment on attachment 9012076 [details] [diff] [review]
Part 1: Convert requestFinished() to use a Promise-based idiom

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

Thanks very much for the async/await conversion!

::: dom/quota/test/helpers.js
@@ +346,5 @@
>  
> +function requestFinished(request) {
> +  return new Promise(function(resolve, reject) {
> +    request.callback = SpecialPowers.wrapCallback(function(request) {
> +      if (request.resultCode == SpecialPowers.Cr.NS_OK) {

existing code nit: This should be triple-equals instead of double-equals I think.

::: dom/quota/test/unit/test_simpledb.js
@@ +6,5 @@
>  var disableWorkerTest = "SimpleDB doesn't work in workers yet";
>  
>  var testGenerator = testSteps();
>  
> +async function* testSteps()

The "*" shouldn't be present for "async function".
Attachment #9012076 - Flags: review?(bugmail) → review+
Comment on attachment 9012076 [details] [diff] [review]
Part 1: Convert requestFinished() to use a Promise-based idiom

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

::: dom/quota/test/unit/test_simpledb.js
@@ +4,5 @@
>   */
>  
>  var disableWorkerTest = "SimpleDB doesn't work in workers yet";
>  
>  var testGenerator = testSteps();

Whoops, missed a nuance related to this I only noticed when reviewing the next patch.

I think this might be a little problematic given how the head.js uses testGenerator like a generator, invoking .next() on it.  That things work seems like it might be related to use of "async function*"?

I think the easiest way to adapt head.js might be to change run_test to be something like this:
```
if (!this.runTest) {
  this.runTest = function()
  {
    do_get_profile();

    enableTesting();

    Cu.importGlobalProperties(["indexedDB", "File", "Blob", "FileReader"]);

    // In order to support converting tests to using add_task(async function() {...}) from
    // using a test "testGenerator", we detect add_task tests by knowing that xpcshell adds
    // them to _gTests.
    if (_gTests.length) {
      Assert.ok(!testGenerator, "There should be no testGenerator if using add_task");
      // Do run our existing cleanup function that would normally be called by the generator's
      // call to finishTest().
      add_task(() => { resetTesting(); });
      // Since we defined run_test, we must invoke run_next_test() to start the async tests.
      run_next_test();
    } else {
      do_test_pending();
      testGenerator.next();
    }
  }
}
```

@@ +53,2 @@
>  
>    finishTest();

This wants to be removed from this idiom then.
Comment on attachment 9012076 [details] [diff] [review]
Part 1: Convert requestFinished() to use a Promise-based idiom

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

::: dom/quota/test/unit/test_simpledb.js
@@ +4,5 @@
>   */
>  
>  var disableWorkerTest = "SimpleDB doesn't work in workers yet";
>  
>  var testGenerator = testSteps();

Uh, and of course that would mean that the test decl below would become:
```
add_task(async function testSteps() {
...
});
```

And the testGenerator here would either stop being defined or just become `var testGenerator;` to leave it undefined but present in the lexical scope.
Comment on attachment 9012077 [details] [diff] [review]
Part 2: Add a test to verify that persisted origins are not constrained by the group limit

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

A few more async/await changes are necessary, but looking great and this really proves the brilliance of SimpleDB for making it easier to write tests without having to worry about overhead bytes of SQLite databases and all that.

Restating; it's fine if you want to throw this in as a comment in the test file if you agree it's what's happening:
- Set the limits as small as our limits allow.  This does result in needing to perform 10 megs of writes which is a lot for a test but not horrible.
- Create databases for 2 origins under the same group.
- Have the foo2 origin use up the shared group quota.
- Verify neither origin can write additional data (via a single byte write).
- Do navigator.storage.persist() for that foo2 origin.
- Verify that both origins can now write an additional byte.  This demonstrates that:
  - foo2 no longer counts against the group limit at all since foo1 can write a byte.
  - foo2 is no longer constrained by the group limit itself.

::: dom/quota/test/unit/test_persist_groupLimit.js
@@ +4,5 @@
> + */
> +
> +var testGenerator = testSteps();
> +
> +async function* testSteps()

(Change to `add_task(async function()` like in previous patch review.)

@@ +13,5 @@
> +  const groupLimitKB = 10 * 1024;
> +  const globalLimitKB = groupLimitKB * 5;
> +
> +  const urls = [
> +    "http://foo1.bar.com",

nit: probably better to use ".example.com" instead of ".bar.com".

@@ +25,5 @@
> +  info("Setting limits");
> +
> +  setGlobalLimit(globalLimitKB);
> +
> +  let request = clear(continueToNextStepSync);

The argument to clear is mooted by the call to requestFinished next, but I'd suggest changing this to `= clear();` for clarity.

@@ +65,5 @@
> +  }
> +
> +  info("Persisting origin");
> +
> +  persist(getPrincipal(urls[foo2Index]), continueToNextStepSync);

Needs idiom update to:
request = persist(urls[foo2Index]);
await requestFinished(request);

(with removal of yield undefined.)
Attachment #9012077 - Flags: review?(bugmail) → review+
Cool, thank you for the comments and suggestions. I'll be more than happy to incorporate them all.
Well, helpers.js needs some adjustments too, but I think it's doable and we will have support for both idioms (generator functions and async functions) in the end.
Attached patch part 1 interdiff (obsolete) — Splinter Review
This is slightly different since there can be only one top level async function, but I think that should be ok for now.
Attachment #9012619 - Flags: review?(bugmail)
Comment on attachment 9012619 [details] [diff] [review]
part 1 interdiff

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

This is much less hacky than what I was proposing.  Sweet!
Attachment #9012619 - Flags: review?(bugmail) → review+
Attachment #9012076 - Attachment is obsolete: true
Attachment #9012619 - Attachment is obsolete: true
Attachment #9012728 - Flags: review+
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/415b25aeccca
Part 1: Add support for using async functions in tests; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b37ec34b104
Part 2: Add a test to verify that persisted origins are not constrained by the group limit; r=asuth
https://hg.mozilla.org/mozilla-central/rev/415b25aeccca
https://hg.mozilla.org/mozilla-central/rev/3b37ec34b104
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.