714960 - Use asyncClose in toolkit/components/satchel/nsFormHistory.js

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use asyncClose in toolkit/components/satchel/nsFormHistory.js

https://tbpl.mozilla.org/?tree=Try&rev=65ada3bb7e4e

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 585555 [details] [diff] [review]
Use asyncClose in toolkit/components/satchel/nsFormHistory.js

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

I assume this is needed because Sync uses an async statements... well the problem is that _dbCleanup tires to remove the database adter invoking _dbClose, and it will never succeed.

Yes, this is really problematic since all the init process is synchronous...

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> I assume this is needed because Sync uses an async statements... well the
> problem is that _dbCleanup tires to remove the database adter invoking
> _dbClose, and it will never succeed.

Something in xpcshell tests used async. I will investigate what is the best place to add a spin for now.

> Yes, this is really problematic since all the init process is synchronous...

Yes, I have some ideas on how to make the sync/async interface less painful, but would like to have seen a few more cases before I try to implement it.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use asyncClose in toolkit/components/satchel/nsFormHistory.js

With ideas taken from bug 566746.

https://tbpl.mozilla.org/?tree=Try&rev=2f2a3cf13738

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 585755 [details] [diff] [review]
Use asyncClose in toolkit/components/satchel/nsFormHistory.js

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

please ask next review to dolske, since this is a bit more related to the service functionality I don't feel like to just approve it with my limited powers.
f+ on the looping approach, but it can be improved imo:

::: toolkit/components/satchel/nsFormHistory.js
@@ +884,2 @@
>              }
> +	};

mozIStorageCompletionCallback has the "function" decorator, so I suspect you may simplify this as

let completed = false;
this.dbConnection.asyncClose(function () { completed = true; });

@@ +886,5 @@
> +
> +        try {
> +            this.dbConnection.asyncClose(blockingClose);
> +        } catch (e) {
> +            Components.utils.reportError(e);

if this throws the next loop will run forever!

@@ +891,4 @@
>          }
> +
> +	let thread =
> +            Cc["@mozilla.org/thread-manager;1"].getService().currentThread;

you can use Services.tm.currentThread

@@ +893,5 @@
> +	let thread =
> +            Cc["@mozilla.org/thread-manager;1"].getService().currentThread;
> +	while (!blockingClose.completed) {
> +	    thread.processNextEvent(true);
> +	}

I suspect would be better to distinguish the reason _dbClose has been invoked, we should do this spinning only when it's invoked to remove the database, on shutdown we should not really block on the async thread.
So I'd add an aReason argument

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: use asyncClose in toolkit/components/satchel/nsFormHistory.js

https://tbpl.mozilla.org/?tree=Try&rev=b1e8965c4805

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 585806 [details] [diff] [review]
use asyncClose in toolkit/components/satchel/nsFormHistory.js

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

::: toolkit/components/satchel/nsFormHistory.js
@@ +868,5 @@
>       * _dbClose
>       *
>       * Finalize all statements and close the connection.
>       */
> +    _dbClose : function FH__dbClose(aBlocking) {

I'd probably suggest an aOnShutdown or aSynchronous, blocking sounds a bit scary... btw let's see what dolske suggests.

@@ +881,5 @@
> +        let f = function () {completed = true; };
> +        if (!aBlocking) {
> +            f = null;
> +            completed = true;
> +        }

you could simplify this a bit

let completed = false;
try {
  this.dbConnection.asyncClose(function () { completed = true; });
} catch (e) {
  Components.utils.reportError(e);
}

while (aBlocking && !completed) ...

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Ping, do you have an ETA on when you can review this?

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

ping

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 585806 [details] [diff] [review]
use asyncClose in toolkit/components/satchel/nsFormHistory.js

Trying another reviewer

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

ping

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 585806 [details] [diff] [review]
use asyncClose in toolkit/components/satchel/nsFormHistory.js

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

Close but I'd like to take a quick look after you incorporate Marco's advice.

::: toolkit/components/satchel/nsFormHistory.js
@@ +868,5 @@
>       * _dbClose
>       *
>       * Finalize all statements and close the connection.
>       */
> +    _dbClose : function FH__dbClose(aBlocking) {

Blocking should sounds scary, because it is, so let's keep this aBlocking (also lines up with what the async form history looks like).

@@ +881,5 @@
> +        let f = function () {completed = true; };
> +        if (!aBlocking) {
> +            f = null;
> +            completed = true;
> +        }

I like Marco's suggestion - it's cleaner. Marco - what happens if asyncClose throws? I think we still want to set completed = true in the catch. Actually, do we even need to catch? (asyncClose only throws when closed from a different thread, but this is JS so that shouldn't ever happen).

Comment 13

[Marco Bonardo [:mak]]

(In reply to Paul O'Shannessy [:zpao] from comment #12)
> I like Marco's suggestion - it's cleaner. Marco - what happens if asyncClose
> throws? I think we still want to set completed = true in the catch.
> Actually, do we even need to catch? (asyncClose only throws when closed from
> a different thread, but this is JS so that shouldn't ever happen).

asyncClose throws rarely, right, just in fallible oom conditions, thread problems or initialization failed. Though may be saner to catch it.
I agree if it trows we should set completed to true, I didn't pay enough attention to the pseudo code there :(

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: use asyncClose in toolkit/components/satchel/nsFormHistory.js

https://tbpl.mozilla.org/?tree=Try&rev=374f3f8991f8

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 593036 [details] [diff] [review]
use asyncClose in toolkit/components/satchel/nsFormHistory.js

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

::: toolkit/components/satchel/nsFormHistory.js
@@ +866,5 @@
>  
>      /**
>       * _dbClose
>       *
>       * Finalize all statements and close the connection.

may you please document the @param here?

@@ +873,5 @@
>          for each (let stmt in this.dbStmts) {
>              stmt.finalize();
>          }
>          this.dbStmts = {};
> +        if (this.dbConnection === undefined)

actually there is alread a bug filed on this, don't remember its number offhand, this should not use this.dbConnection but FormHistory.prototype.dbConnection, otherwise here it may init the connection just to shutdown it.

@@ +878,5 @@
> +            return;
> +
> +        let completed = false;
> +        try {
> +            this.dbConnection.asyncClose(function () {completed = true; });

add space after the opening breace

Comment 16

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Marco Bonardo [:mak] from comment #15)
> > +        if (this.dbConnection === undefined)
> 
> actually there is alread a bug filed on this, don't remember its number
> offhand, this should not use this.dbConnection but
> FormHistory.prototype.dbConnection, otherwise here it may init the
> connection just to shutdown it.

Bug 720311

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 593036 [details] [diff] [review]
use asyncClose in toolkit/components/satchel/nsFormHistory.js

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

Mostly what Marco said, again :)

::: toolkit/components/satchel/nsFormHistory.js
@@ +873,5 @@
>          for each (let stmt in this.dbStmts) {
>              stmt.finalize();
>          }
>          this.dbStmts = {};
> +        if (this.dbConnection === undefined)

I mentioned that bug, but I'm not sure that fixes it. The prototype still has the getter which would get accessed right? Anyway, let's not worry about that here and do it in the other bug.

Comment 18

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: final version

mozilla-inbound is closed. This is what I will push once it opens.

Comment 19

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=de8e1de24f37

Comment 20

[Marco Bonardo [:mak]]

(In reply to Paul O'Shannessy [:zpao] from comment #17)
> I mentioned that bug, but I'm not sure that fixes it. The prototype still
> has the getter which would get accessed right? Anyway, let's not worry about
> that here and do it in the other bug.

I don't remember the code offhand, I thought we were setting the connection on the prototype dinamically, btw thanks for pointing the bug, would be nice to fix it.

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/de8e1de24f37