702848 - finalize the statements in services/sync/modules/engines/forms.js

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Comment 1

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

Attachment: finalize the statements in services/sync/modules/engines/forms.js

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

Comment 2

[Richard Newman [:rnewman]]

Hi Rafael,

Love him though we do, mak isn't a Sync module reviewer:

https://wiki.mozilla.org/Services/Process/Code_Review#Firefox_Sync

It might be best for you to split this patch in two (one part Satchel, one part Sync) so they can be assessed and landed separately. I'm happy to review the Sync portion.

Thanks!

Comment 3

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

(In reply to Richard Newman [:rnewman] from comment #2)
> Hi Rafael,
> 
> Love him though we do, mak isn't a Sync module reviewer:
> 
> https://wiki.mozilla.org/Services/Process/Code_Review#Firefox_Sync
> 
> It might be best for you to split this patch in two (one part Satchel, one
> part Sync) so they can be assessed and landed separately.

They cannot, the Sync part uses the one in Satchel. Can you "coreview" the patch and I land it once I get an OK for both?

> I'm happy to
> review the Sync portion.
> 
> Thanks!

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 574764 [details] [diff] [review]
finalize the statements in services/sync/modules/engines/forms.js

Drive by review: if you change the interface (nsIFormHistory2), you also need to rev its uuid.

Also, I don't think nsFormHistory.js itself finalizes statements in all cases. See bug 586339.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 574764 [details] [diff] [review]
finalize the statements in services/sync/modules/engines/forms.js

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

Dolske should review the formhistory part, I don't feel like reviewing an idl change in that component.

But here is some drive-by comments.
I think this is a good approach and likely soon others will follow (Places will likely), but actually if you do this, you should remove DBConnection from the interface, for a real simple reason: add-ons or anyone else in newer code may use it and your additional method would then be pointless. That may be a little more complex until bug 697377 is fixed.

Maybe for now it would be better to just finalize the statement in Sync shutdown code?
Richard, doesn't Sync finalize statements in _stmts cache? If it does, may be it's done too late? Sync is a profile component and should not init shutdown later than profile-before-change.

::: toolkit/components/satchel/nsIFormHistory.idl
@@ +98,5 @@
>     *        The end of the timeframe, in microseconds
>     */
>    void removeEntriesByTimeframe(in long long aBeginTime, in long long aEndTime);
>  
> +  mozIStorageAsyncStatement dbCreateAsyncStatement(in AUTF8String aSQLStatement);

If we keep this approach, that imo is the right long-term one, I'd just call this getStatement. The synchronous createStatement should disappear from this module btw.

and yeah, you should rev the uuid of this interface.

Comment 6

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

Attachment: finalize the statements in services/sync/modules/engines/forms.js

I implemented some of the comments. I opened a followup bug to remove direct access to the connection. I changed the method name to getAsyncStatement. My idea for removing the connection is adding a getSyncStatement. I know that we want to remove the sync statements, but one step at a time.

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 575309 [details] [diff] [review]
finalize the statements in services/sync/modules/engines/forms.js

I don't understand this. Consumers creating statements should be responsible for finalizing them when they're no longer needed.

Also, bug 566746 is coming along, so it may make more sense to leave the current code as-is and either refine the implementation there or fix things on top of that.

Comment 8

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

> I don't understand this. Consumers creating statements should be responsible
> for finalizing them when they're no longer needed.

I am not familiar with the code and didn't see a convenient place for doing it. Note that we must finalize *all* the statements is dbCleanup. So the options that I see is nsFormHistory.js knowing all of them or it having some form of observers so that it can let them know that the connection is going to be closed.

> Also, bug 566746 is coming along, so it may make more sense to leave the
> current code as-is and either refine the implementation there or fix things
> on top of that.

Lets please not add an unnecessary dependency. Changing this code to use async statements and changing it to correctly finalize them are independent improvements.

Comment 9

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

Attachment: finalize the statements in services/sync/modules/engines/forms.js

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

Comment 10

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

Attachment: finalize the statements in services/sync/modules/engines/forms.js

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

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 577338 [details] [diff] [review]
finalize the statements in services/sync/modules/engines/forms.js

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

Reviewing the Sync portion of this.

Please address question about re-registering.

::: services/sync/modules/engines/forms.js
@@ +76,5 @@
>      }
>  
> +    if (!this._registered) {
> +      let self = this;
> +      Svc.Form.registerCloseCallback(function() {self._closeCallback();});

We use Function.bind() for this. E.g.,

  Svc.Form.registerCloseCallback(this._closeCallback.bind(this));

But in this case there's only ever one FormWrapper, so you could avoid bind entirely by writing _closeCallback in terms of FormWrapper instead of `this`. Or just inline the whole darn thing:

  if (!this._registered) {
    Svc.Form.registerCloseCallback(function () {
      for each (let stmt in FormWrapper._stmts) {
        stmt.finalize();
      }
      FormWrapper._stmts = {};
    });
  }

::: toolkit/components/satchel/nsFormHistory.js
@@ +130,5 @@
>          this.updatePrefs();
>  
>          this.dbStmts = {};
>  
> +        this.callbacks = [];

Shouldn't this be called "closeCallbacks"?

@@ +882,5 @@
> +
> +        for each (let callback in this.callbacks) {
> +            callback.complete();
> +        }
> +        this.callbacks = [];

If you're wiping out the callbacks here, then surely we need a way to be told to re-register? Or is the DB only finalized once before shutdown?

Comment 12

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

Attachment: finalize the statements in services/sync/modules/engines/forms.js

Thanks for the feedback!

The finalize function will eventually also close the database, in which case I think it makes sense to clear the callback list and have clients register again, but we can do that when we change it to close the connection.

Comment 13

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

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

Comment 14

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

Ping. Is this blocking on my end?

Comment 15

[Richard Newman [:rnewman]]

I can't speak for dolske, but for my part I have been swamped by P0 Android Sync work. I will get to this when that ships. Sorry!

Comment 16

[Justin Dolske [:Dolske]]

What problem are you trying to solve here? I really don't like adding callbacks like this, and the better long-term fix is for sync to use the new API anyway.

Comment 17

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

(In reply to Justin Dolske [:Dolske] from comment #16)
> What problem are you trying to solve here? I really don't like adding
> callbacks like this, and the better long-term fix is for sync to use the new
> API anyway.

To close a connection we have to finalize the statements. The first solution was not using the connection directly, but having a getAsyncStatement, so that we could have a central location with all the statements that we have to finalize.

You didn't liked it, so I switched to having a callback that lets users of the connection know that it is going away and can finalize the statements.

Comment 18

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

Attachment: finalize the statements in services/sync/modules/engines/forms.js

Rebased and includes a suggestion from mak on how to better iterate the callback array.

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

Comment 19

[(dormant account)]

Richard/Dolske, can we get some movement on this bug? This a blocker for cleaning up sqlite connections properly on exit which is required for a snappy:p1 bug 662444.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 581956 [details] [diff] [review]
finalize the statements in services/sync/modules/engines/forms.js

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

The Sync and JS portion look OK to me, but I have given this only a cursory consideration. Over to dolske for review on the rest.

::: services/sync/modules/engines/forms.js
@@ +79,5 @@
> +      Svc.Form.registerCloseCallback(function () {
> +        for each (let stmt in FormWrapper._stmts) {
> +          stmt.finalize();
> +        }
> +        FormWrapper._stmts = {};

+ FormWrapper._registered = false;

Comment 21

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

Attachment: finalize the statements in services/sync/modules/engines/forms.js

I included rnewman's comments, rebased and push to

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

Is it OK?

Comment 22

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

Attachment: another way of fixing this

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

Comment 23

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

I have no preference on which of the above patches gets used. Dolske, please select one.

Comment 24

[Justin Dolske [:Dolske]]

Comment on attachment 583192 [details] [diff] [review]
another way of fixing this

Yes, this looks much more sensible to me!

Comment 25

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

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

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13a57f89f8cc