1098965 - test_sqlite.js: test_close_database_on_gc uses one Promise for 100 databases

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Alex Vincent [:WeirdAl]]

let deferred = Promise.defer();

  for (let i = 0; i < 100; ++i) {
    let c = yield getDummyDatabase("gc_" + i);
    c._connectionData._deferredClose.promise.then(deferred.resolve);
  }
// ...
  yield deferred.promise;

The way I read that, once a single call to deferredClose passes, the test has essentially passed.

Filed as UNCO because I don't know SQLite or this code at all.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: promiseAllDBs.diff

If this bug is legitimate, this patch should suffice.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8522742 [details] [diff] [review]
promiseAllDBs.diff

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

let's have David take a look, I think the bug is legitimate but maybe there was a reason for that.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Actually, this is a trick used in several gc-related tests. Since the gc used to be conservative, we can end up in a situation in which something in memory is misinterpreted as a reference to a JS object, which then used to prevent that object from being garbage-collected. Using 100 objects instead of 1 made (almost) sure that at least one of the objects didn't have any fake reference pointing to it.

Now, these days, the gc has been rewritten, and I think it has become exact. If so, we can get rid of this trick. Jason, can you confirm that?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8522742 [details] [diff] [review]
promiseAllDBs.diff

Canceling review until we have info from Jason.

Comment 5

[Jason Orendorff [:jorendorff]]

Forwarding.

Comment 6

[Terrence Cole [:terrence]]

Yes, the conservative scanner is disabled on all branches; you should not need any tricks like this anymore.

Comment 7

[Alex Vincent [:WeirdAl]]

Comment on attachment 8522742 [details] [diff] [review]
promiseAllDBs.diff

Restoring review request per comment 3 and comment 6.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8522742 [details] [diff] [review]
promiseAllDBs.diff

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

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +1048,3 @@
>      let c = yield getDummyDatabase("gc_" + i);
>      c._connectionData._deferredClose.promise.then(deferred.resolve);
> +    collectedPromises.push(deferred.promise);

Let's take the opportunity to use `new Promise` instead of `Promise.defer()`.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: promiseAllDBs.diff

Updated per feedback comments.  Also, bringing it closer to bug 1095267.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8526177 [details] [diff] [review]
promiseAllDBs.diff

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

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +25,5 @@
> +  }
> +  let resolve, reject;
> +  let promise = new Promise(forward);
> +  return [promise, resolve, reject];
> +}

Rather than reinventing it, you should use PromiseUtils.defer.

@@ +1068,5 @@
>    // references. This is needed at the moment, otherwise
>    // garbage-collection takes place after the shutdown barrier and the
>    // test will timeout. Once that is fixed, we can remove this line
>    // and be fine as long as at least one connection is
>    // garbage-collected.

"as long as at least one connection is garbage-collected" => "as long as the connections are garbage-collected"

Comment 11

[Alex Vincent [:WeirdAl]]

Attachment: promiseAllDBs.diff

We can't fully convert over to native promises for this test, because that causes a hang (see bug 1095267 comment 1).  Further testing, however, shows we can make Promise.jsm's Promise.all work with PromiseUtils.defer()'s native promises.  So the hang has to be in Promise.all.

Comment 12

[Carsten Book [:Tomcat]]

Hi Alex, would it be possible to get a try link ? Thanks!

Comment 13

[Carsten Book [:Tomcat]]

yoric, alex told me he don't have commit rights - could you do the try push, maybe we could give alex also L1 access

Comment 14

[David Teller [:Yoric] - still alive but not very active]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1f1b1cb8755d
WeirdAl, if you wish to apply for L1, I can vouch for you.

Comment 15

[Alex Vincent [:WeirdAl]]

Please tell me this patch isn't being held because of the needinfo for L1...

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Alex Vincent [:WeirdAl] from comment #15)
> Please tell me this patch isn't being held because of the needinfo for L1...

nope that was more because the checkin-needed keyword was deleted in comment 13 while waiting for the try run and then never added back again. 

Will do the checkin

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/7c95c7d31d45

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c95c7d31d45