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

RESOLVED FIXED in mozilla12

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

(Blocks: 1 bug)

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1])

Attachments

(2 attachments, 6 obsolete attachments)

8.41 KB, patch
Details | Diff | Splinter Review
4.49 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 574764 [details] [diff] [review]
finalize the statements in services/sync/modules/engines/forms.js

https://tbpl.mozilla.org/?tree=Try&rev=553f738c53db
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #574764 - Flags: review?(mak77)
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!
OS: Mac OS X → All
Hardware: x86 → All
(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 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 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.
Attachment #574764 - Flags: review?(mak77) → review?(dolske)
Created attachment 575309 [details] [diff] [review]
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.
Attachment #574764 - Attachment is obsolete: true
Attachment #574764 - Flags: review?(dolske)
Attachment #575309 - Flags: review?(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.
Attachment #575309 - Flags: review?(dolske) → review-
> 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.
Created attachment 576272 [details] [diff] [review]
finalize the statements in services/sync/modules/engines/forms.js

https://tbpl.mozilla.org/?tree=Try&rev=143bb13281ed
Attachment #575309 - Attachment is obsolete: true
Attachment #576272 - Flags: review?(dolske)
Created attachment 577338 [details] [diff] [review]
finalize the statements in services/sync/modules/engines/forms.js

https://tbpl.mozilla.org/?tree=Try&rev=b8a84d842fe1
Attachment #576272 - Attachment is obsolete: true
Attachment #576272 - Flags: review?(dolske)
Attachment #577338 - Flags: review?(dolske)
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?
Created attachment 577668 [details] [diff] [review]
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.
Attachment #577338 - Attachment is obsolete: true
Attachment #577338 - Flags: review?(dolske)
Attachment #577668 - Flags: review?(rnewman)
Attachment #577668 - Flags: feedback?(dolske)
Ping. Is this blocking on my end?
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!
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.
(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.
Created attachment 581956 [details] [diff] [review]
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
Attachment #577668 - Attachment is obsolete: true
Attachment #577668 - Flags: review?(rnewman)
Attachment #577668 - Flags: feedback?(dolske)
Attachment #581956 - Flags: review?(rnewman)
Attachment #581956 - Flags: feedback?(dolske)

Updated

7 years ago
Whiteboard: [Snappy:P1]

Comment 19

7 years ago
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 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;
Attachment #581956 - Flags: review?(rnewman)
Attachment #581956 - Flags: review?(dolske)
Attachment #581956 - Flags: review+
Attachment #581956 - Flags: feedback?(dolske)
Created attachment 583157 [details] [diff] [review]
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?
Attachment #581956 - Attachment is obsolete: true
Attachment #581956 - Flags: review?(dolske)
Attachment #583157 - Flags: review?(dolske)
I have no preference on which of the above patches gets used. Dolske, please select one.
Comment on attachment 583192 [details] [diff] [review]
another way of fixing this

Yes, this looks much more sensible to me!
Attachment #583192 - Flags: review?(dolske) → review+
Attachment #583157 - Flags: review?(dolske)
https://hg.mozilla.org/mozilla-central/rev/13a57f89f8cc
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.