688913 - Finalize statements in extensions/cookie and toolkit/components/contentprefs

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

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

Attachment: Close connections.

Found this while working on 687726, since the assert added by that patch was failing.

Comment 1

[Shawn Wilsher :sdwilsh]

Comment on attachment 562192 [details] [diff] [review]
Close connections.

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

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +3297,1 @@
>      mConnection = nsnull;

Setting the connection to `nsnull` here should be enough (due to `Connection`s destructor).

Why do we need to do this?

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 562192 [details] [diff] [review]
Close connections.

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

::: dom/src/storage/nsDOMStorageDBWrapper.cpp
@@ +76,5 @@
>  
>  nsDOMStorageDBWrapper::~nsDOMStorageDBWrapper()
>  {
> +  mPersistentDB.Close();
> +  mChromePersistentDB.Close();

I think this happens uselessy late, dom storage is a profile-based component, it should finalize its stuff at profile-before-change, indeed it does here http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.cpp#469
The databases should be closed through a call from there imo, adding a method to nsDOMStorageDBWrapper that will then invoke these connections close()

::: dom/src/storage/nsDOMStoragePersistentDB.cpp
@@ +448,5 @@
>  }
>  
> +void
> +nsDOMStoragePersistentDB::Close() {
> +  mConnection->AsyncClose(0);

I think 0 here looks like a magic number, eve if finally it's the same, would be more correct to pass nsnull.

::: dom/src/storage/nsDOMStoragePersistentDB.h
@@ +61,5 @@
>    nsresult
>    Init(const nsString& aDatabaseName);
>  
> +  void
> +  Close();

needs a javadoc, as any other method in this header

::: extensions/cookie/nsPermissionManager.cpp
@@ +792,4 @@
>        RemoveAllInternal();
>      } else {
>        RemoveAllFromMemory();
> +      mDBConn->AsyncClose(0);

I think it looks bogus having a shutdown path like
if (a) {
  call_shutdown_method_that_asyncClose internally();
} else {
  call_alt_shutdown_method();
  asyncClose();
}

imo both should be asyncClosed externally from the RemoveAll methods, eventually even out of the if/else (with a mDBConn null check).

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +3297,1 @@
>      mConnection = nsnull;

I think this is correct (as before I'd rather pass nsnull though), since the connection destructor doesn't force an async close, causing us to report the async connection leak I think. isn't it?

Comment 3

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

> Setting the connection to `nsnull` here should be enough (due to
> `Connection`s destructor).
> 
> Why do we need to do this?

The destructor uses the sync one, which does nothing if an async operation was used.

Comment 4

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

> >  nsDOMStorageDBWrapper::~nsDOMStorageDBWrapper()
> >  {
> > +  mPersistentDB.Close();
> > +  mChromePersistentDB.Close();
> 
> I think this happens uselessy late, dom storage is a profile-based
> component, it should finalize its stuff at profile-before-change, indeed it
> does here
> http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.
> cpp#469
> The databases should be closed through a call from there imo, adding a
> method to nsDOMStorageDBWrapper that will then invoke these connections
> close()

Thanks!

I was having a hard time figuring out where it should go.

...


> I think this is correct (as before I'd rather pass nsnull though), since the
> connection destructor doesn't force an async close, causing us to report the
> async connection leak I think. isn't it?

That is it.

Trying this patch also found more statements that were not finalized. I will fix that, include your review and upload a new patch.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 562192 [details] [diff] [review]
Close connections.

I'll review this once mak's feedback has been addressed.

Comment 6

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

Attachment: Close connections.

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

Comment 7

[Marco Bonardo [:mak]]

the changes to formHistory may be clashing with the work in bug 690903, whatever lands first should communicate to the other. Will look at this soon, most likely early tomorrow.

Comment 8

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

Attachment: updated version that also finalizes some statements

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

Comment 9

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

Attachment: finish some statements

Testing the previous patch in a debug build found many statements that were not being finalized.

This patch is a first batch of statements that have to be finalized.

Comment 10

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

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

Comment 11

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

Attachment: finish some statements

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

Comment 12

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

Attachment: finish some statements

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

The difference from the previous patch is that this one removes a use of profile-before-change from a test. Using messages that are normally sent during shudown in the middle of a process looks error prone, so I just added a dummy message.

Using a dummy message looks ugly, but I am not sure if there is a better way for the test to force FlushAndDeleteTemporaryTables to run.

Comment 13

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

A new try run is at:
https://tbpl.mozilla.org/?tree=Try&rev=bedce6ffaddf

The only difference is

+  os->AddObserver(gStorageManager, "test-bug624047-flush", false);

Comment 14

[Marco Bonardo [:mak]]

does the new patch replace the old one? it's a bit unclear which patch I should look at.

Comment 15

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

Attachment: last version of the patch that finishes statements

Comment 16

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

Comment on attachment 567114 [details] [diff] [review]
updated version that also finalizes some statements

Marking this one as obsolete. I will upload a new one that just closes connections once the one that finishes statements is in.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 567729 [details] [diff] [review]
last version of the patch that finishes statements

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

ok here is a first pass, unfortunately I found some issues in the components themselves, some are trivial and may be fixed here, what you are not comfortable with needs follow-up bugs.
But globally it's a nice improvement. on next iteration I'd like you to get a review from honza, and probably dolske may signoff the other changes. But I'd be fine with reviewing those as well, if they are fine with that.

::: dom/src/storage/nsDOMStorage.cpp
@@ +287,5 @@
>    os->AddObserver(gStorageManager, "profile-after-change", false);
>    os->AddObserver(gStorageManager, "perm-changed", false);
>    os->AddObserver(gStorageManager, "browser:purge-domain-data", false);
>    // Used for temporary table flushing
> +  os->AddObserver(gStorageManager, "test-bug624047-flush", false);

I think this may be named more generically "test-flush", more tests may use it in future, we can't tell.

@@ +470,5 @@
>      DOMStorageImpl::gStorageDB->RemoveOwner(aceDomain, true);
> +  } else if (!strcmp(aTopic, "test-bug624047-flush") && DOMStorageImpl::gStorageDB) {
> +     nsresult rv = DOMStorageImpl::gStorageDB->FlushAndDeleteTemporaryTables(true);
> +     if (NS_FAILED(rv))
> +       NS_WARNING("DOMStorage: temporary table commit failed");

DebugOnly<nsresult> rv = ...
NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), ...
(may require #include "mozilla/Util.h")

@@ +475,2 @@
>    } else if (!strcmp(aTopic, "profile-before-change") || 
>               !strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {

I don't see reasoning why this should observe both profile-before-change and xpcom-shutdown, DOMStorage uses a profile file, it should never do anything before profile-after-change and after profile-before-change, that should indeed be considered an error.

If this is needed for testing purposes, I think all the harnesses send the right profile notifications (in xpcshell you have to invoke do_get_profile() before actually using a profile, otherwise you are doing it wrong).
May we remove xpcom-shutdown from here and eventually fix tests?

@@ +475,3 @@
>    } else if (!strcmp(aTopic, "profile-before-change") || 
>               !strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
> +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();

I think this is not infallible, indeed you probably want to wrap removals in an if (os) {}

@@ +475,5 @@
>    } else if (!strcmp(aTopic, "profile-before-change") || 
>               !strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
> +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +    os->RemoveObserver(this, "profile-before-change");
> +    os->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);

Interesting, looks like DOMStorage has a bunch of owning addObserver notifications, but it never invokes RemoveObserver for them? I think this is a good place to do so.
cast these to (void) since we explicitly ignore return value. (void)os->RemoveObs...

::: dom/src/storage/nsDOMStorageDBWrapper.h
@@ +89,5 @@
>    nsDOMStorageDBWrapper();
>    ~nsDOMStorageDBWrapper();
>  
> +  /**
> +   * Close the connections.

... finalizing all the cached statements.

::: dom/src/storage/nsDOMStoragePersistentDB.cpp
@@ +449,5 @@
>  
> +void
> +nsDOMStoragePersistentDB::Close() {
> +  mCopyToTempTableStatement->Finalize();
> +  mCopyToTempTableStatement = nsnull;

I think it's safer if you null check those before finalizing. Now we init them on Init() but that's not really needed, indeed I think we should file a bug to lazily init DOMStorage statements (hint: StatementCache).
I don't think you should both Finalize() and set to nsnull, sync statements are finalized on destruction, so you may avoid the null check and Finalize() by just setting them to nsnull (with a nice comment explaining what happens). Should be a more compact code to read.
Otherwise make a nice vector of statements and finalize in a loop.
cast to (void) since you explicitly ignore the return value.

::: dom/src/storage/nsDOMStoragePersistentDB.h
@@ +62,5 @@
>    nsresult
>    Init(const nsString& aDatabaseName);
>  
>    /**
> +   * Close the connection.

... finalizing all the cached statements.

::: extensions/cookie/nsPermissionManager.cpp
@@ +800,5 @@
> +      mStmtDelete = nsnull;
> +
> +      mStmtUpdate->Finalize();      
> +      mStmtUpdate = nsnull;
> +    }

I don't understand, RemoveAllInternal deletes everything from the database, finalizes statements, then it invokes initDB(true) that removes the database file. But then it creates a new database(!) Why, we are shutting down! this sounds like a lot of useless work to do on shutdown, there should probably be a mShuttingDown property and initDB should not recreate a db on shutdown after removal of the old one.
Finally, just set statements to nsnull here, add a comment about // Finalize cached statements.

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +270,5 @@
> +      this.__stmtDeleteGroupIfUnused.finalize()
> +    if (this.__stmtDeletePref)
> +      this.__stmtDeletePref.finalize()
> +    if (this.__stmtUpdatePref)
> +      this.__stmtUpdatePref.finalize()

could you make a nice array and forEach it?
ideally these statements should already be part of local _statements hash rather than being single magic properties :( Its so easy this way to forget finalizing a newly added statement.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +128,5 @@
>                          break;
>                      default:
>                          self.log("Oops! Pref not handled, change ignored.");
>                  }
>              } else if (topic == "xpcom-shutdown") {

wtf, this is shutting down too late >( please make it profile-before-change please

@@ +130,5 @@
>                          self.log("Oops! Pref not handled, change ignored.");
>                  }
>              } else if (topic == "xpcom-shutdown") {
> +                for each (let stmt in this.dbStmts)
> +                    stmt.finalize();

this is wrong, "this" in this context is the observer object (not sure why this component has a useless observer object) and .dbStmts does not exist, should be self._dbStmts
also _dbStmts is initialized as an array, but then it is used as an object adding text indices to it, please fix initialization, then you can really use for each here (should not be used on arrays).

::: toolkit/components/satchel/nsFormHistory.js
@@ +145,1 @@
>          Services.obs.addObserver(function() { self.expireOldEntries() }, "idle-daily", false);

even if you don't like that style, please be consistent with the file code style

@@ +848,5 @@
> +        // Finalize all statements to free memory, avoid errors later
> +        for each (let stmt in this.dbStmts)
> +            stmt.finalize();
> +        this.dbStmts = [];
> +    },

dbClose does not make what its name says, move .close() here, from dbCleanup.

again, dbStmts is wrongly initialized as an array and then used an an object, fix its initialization please.

Comment 18

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

I did a try push with the simple changes at

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

I will upload to this bug if it is OK. I have found some problems when closing the connections, which is why I split the patch in the first place. If it is OK with you, I would like to keep this patch small and have follow up patches. So we would have the following patches

* The current patch on try, assuming it is green. If not, I will move stuff into a followup patch.
* Refactor code to use an array in nsContentPrefService.js
* Fix shutdown in nsPermissionManager.cpp
* Close the connections

Comment 19

[Marco Bonardo [:mak]]

sounds like a plan, I'll try to be faster in checking those parts.
A couple minor things in the try patch:
- use // line comments inside methods/functions, since multiline comments are annoying to deal with when commenting parts of code.
- use DebugOnly<> when you use the result value only in debug mode (NS_WARN_ does)
- check javadocs are proper javadocs, this is not: https://hg.mozilla.org/try/rev/7e5b0e4f0375#l10.30
- check spacing (broken here https://hg.mozilla.org/try/rev/7e5b0e4f0375#l10.12)

Comment 20

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

Attachment: Close connections.

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

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 568051 [details] [diff] [review]
Close connections.

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

apart the FIXME stuff, that still has to be handled, looks good enough. when the next comments are addressed, please get review from honza at least on the approach, I can then review the final implementation.

::: dom/src/storage/nsDOMStorage.cpp
@@ +287,5 @@
>    os->AddObserver(gStorageManager, "profile-after-change", false);
>    os->AddObserver(gStorageManager, "perm-changed", false);
>    os->AddObserver(gStorageManager, "browser:purge-domain-data", false);
>    // Used for temporary table flushing
> +  os->AddObserver(gStorageManager, "test-flush", false);

actually you don't need this since the tests can directly invoke .observe with the topic, indeed the test does that.

@@ +472,5 @@
> +     NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "DOMStorage: temporary table commit failed");
> +  } else if (!strcmp(aTopic, "profile-before-change")) {
> +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +    if (os)
> +      (void)os->RemoveObserver(this, "profile-before-change");

please, either file a bug to remove all the other observers too, or do it.

::: dom/src/storage/nsDOMStoragePersistentDB.cpp
@@ +448,5 @@
>  }
>  
> +void
> +nsDOMStoragePersistentDB::Close() {
> +  /* Null the statements, this should cause them to be finalized. */

Use line comment here, unless the module wants explicitly to do different.
imo s/should cause them to be finalized/will finalize them/ but I'm not english :)

::: extensions/cookie/nsPermissionManager.cpp
@@ +792,5 @@
>      } else {
>        RemoveAllFromMemory();
>      }
> +    if (mDBConn) {
> +      // Null the statements, this should cause them to be finalized.

nit: ditto on more direct phrase

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +241,5 @@
>      this._observerSvc.removeObserver(this, "xpcom-shutdown");
>      this._observerSvc.removeObserver(this, "private-browsing");
>  
>      // Finalize statements which may have been used asynchronously.
> +    // FIXME: put them in an array.

an object cache, like many other components do, may even be better. btw I guess followup.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +88,2 @@
>  
> +        Services.obs.addObserver(this.observer, "profile-before-change", false);

where is this removed? Should I assume you forgot to change xpcom-shutdown to profile-before-change in the removal or this just forgets to remove its own observers (and if so, sigh).

@@ +130,5 @@
>                          self.log("Oops! Pref not handled, change ignored.");
>                  }
> +            } else if (topic == "profile-before-change") {
> +                for each (let stmt in self._dbStmts)
> +                    stmt.finalize();

please brace loops

::: toolkit/components/satchel/nsFormHistory.js
@@ +138,5 @@
>          this.messageManager.loadFrameScript("chrome://satchel/content/formSubmitListener.js", true);
>          this.messageManager.addMessageListener("FormHistory:FormSubmitEntries", this);
>  
>          // Add observers
> +        Services.obs.addObserver(function() { self.dbClose(); }, "profile-before-change", false);

where is this removed?

@@ +845,5 @@
> +    dbClose : function () {
> +        // FIXME: close the connection in here.
> +        // Finalize all statements to free memory, avoid errors later
> +        for each (let stmt in this.dbStmts)
> +            stmt.finalize();

please brace loops

Comment 22

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

Attachment: Finish statements

Comment 23

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

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

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 568069 [details] [diff] [review]
Finish statements

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

a couple comments, but is good.
Now please get that r from honza, and put together a plan to address those FIXME comments, either file followups and add bug number to each fixme, or address in new parts.

::: dom/tests/mochitest/localstorage/test_bug624047.html
@@ +23,5 @@
>  {
>    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>    var storageManager = Components.classes["@mozilla.org/dom/storagemanager;1"]
>      .getService(Components.interfaces.nsIObserver);
> +  storageManager.observe(null, "domstorage-flush-timer", null);

nice trick!

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +129,5 @@
>                      default:
>                          self.log("Oops! Pref not handled, change ignored.");
>                  }
> +            } else if (topic == "profile-before-change") {
> +                Services.obs.removeObserver(self.observer, "profile-before-change", false);

iirc removeObserver does not want a third param. it won't complain due to xpconnect but it's not needed.

::: toolkit/components/satchel/nsFormHistory.js
@@ +140,5 @@
>  
>          // Add observers
> +        Services.obs.addObserver(function() {
> +            self.dbClose();
> +            Services.obs.removeObserver(this, "profile-before-change", false);

you cannot use "this" here, nor arguments.callee since it's deprecated. You have to give a name to the function and use that name instead of "this"
also third param to removeObserver does not exist

Comment 25

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

Attachment: Finish statements

I will open a bug with the FIXME once the patch is in.

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

Comment 26

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

Attachment: Finish statements

I have split the bug to try to get *something* moving forward. This patch is basically the parts that didn't fit in the new bugs.

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 568692 [details] [diff] [review]
Finish statements

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

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +242,4 @@
>      this._observerSvc.removeObserver(this, "private-browsing");
>  
>      // Finalize statements which may have been used asynchronously.
> +    // FIXME: put them in an object cache like other components.

file the followup bug and put bug number here

Comment 28

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

Attachment: Finish statements

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

Comment 29

[Honza Bambas (:mayhemer)]

Comment on attachment 568151 [details] [diff] [review]
Finish statements

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

r-

::: dom/src/storage/nsDOMStorage.cpp
@@ +280,5 @@
>    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>    if (!os)
>      return NS_OK;
>  
> +  // FIXME: we should RemoveObserver these somewhere.

You don't need to deregister observers manually.  You can tell the observer service to keep them just as weak references: let nsDOMStorageManager implement weak ref, see bellow how to do that, and change the third param of AddObserver to true.  

In .h:
#include "nsWeakReference.h" and derive using:
public nsSupportsWeakReference

In .cpp the interface implementation add this interface as:
NS_IMPL_ISUPPORTSn(..., nsISupportsWeakReference)

That's it.

For JS components, see http://mxr.mozilla.org/mozilla-central/source/mobile/components/SessionStore.js#65 and #177, but be careful with JS object life time, they can get GC'ed while should be left alive right by the reference from observer service.  That doesn't of course apply to components instantiated as services.

@@ +469,5 @@
>      DOMStorageImpl::gStorageDB->RemoveOwner(aceDomain, true);
> +  } else if (!strcmp(aTopic, "profile-before-change")) {
> +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +    if (os)
> +      (void)os->RemoveObserver(this, "profile-before-change");

"profile-before-change" may come more then ones in an app life time.  E.g. SeaMonkey supports profile switching.

Implement weak ref and don't bother with removing observers.

@@ +474,2 @@
>      if (DOMStorageImpl::gStorageDB) {
> +      DebugOnly<nsresult> rv = DOMStorageImpl::gStorageDB->FlushAndDeleteTemporaryTables(true);

80-columns max please, also on other places.

::: dom/src/storage/nsDOMStorageDBWrapper.cpp
@@ +81,5 @@
> +void
> +nsDOMStorageDBWrapper::Close()
> +{
> +  mPersistentDB.Close();
> +  mChromePersistentDB.Close();

You also have to clean DOMStorageImpl::gStorageDB here (delete and nullify).  That will force databases to lazily reinitialize in the newly selected profile.

Consider putting a Close method to DOMStorageImpl where you call gStorageDB->Close() and then delete/nullify it.

You should manually test with SeaMonkey build and switch between two profiles.

Write an automated test that artificially calls Observer on the manager with profile-before-change and then profile-after-change (if there is need to wait for the database async close, then ensure it).  Then access the localStorage and check previously newly added data are still there after call of before/after-change topics.  Also check it still works for writing.

::: dom/tests/mochitest/localstorage/test_bug624047.html
@@ +23,5 @@
>  {
>    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>    var storageManager = Components.classes["@mozilla.org/dom/storagemanager;1"]
>      .getService(Components.interfaces.nsIObserver);
> +  storageManager.observe(null, "domstorage-flush-timer", null);

I want the forceFlush arg be correctly set for the test (true).  This may break the test unexpectedly.

I'm not against adding a regular (not just a test-purpose) topic aka "domstorage-flush-force" that would force flush and use it here.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +88,2 @@
>  
> +        Services.obs.addObserver(this.observer, "profile-before-change", false);

this.observer implements nsISupportsWeakReference, add it as weak then, no need to handle removal.  FormAutoComplete is a service.

@@ +129,5 @@
>                      default:
>                          self.log("Oops! Pref not handled, change ignored.");
>                  }
> +            } else if (topic == "profile-before-change") {
> +                Services.obs.removeObserver(self.observer, "profile-before-change");

...

::: toolkit/components/satchel/nsFormHistory.js
@@ +141,5 @@
>          // Add observers
> +        Services.obs.addObserver(function pbc() {
> +            self.dbClose();
> +            Services.obs.removeObserver(pbc, "profile-before-change");
> +        }, "profile-before-change", false);

Here again.

FormHistory is a service that lives between profiles.

FormHistory should implement nsIObserver, the handling code should move there, or inspire by FormAutoComplete.observer.

You can just add nsISupportsWeakRerefence to the list of implemented interfaces - no need to implement it, as said above - and do addObserver(this, "...", true) and don't bother with removal.

Comment 30

[Marco Bonardo [:mak]]

(In reply to Honza Bambas (:mayhemer) from comment #29)

> For JS components, see
> http://mxr.mozilla.org/mozilla-central/source/mobile/components/SessionStore.
> js#65 and #177, but be careful with JS object life time, they can get GC'ed
> while should be left alive right by the reference from observer service. 

indeed it's easier to removeObserver in js objects and services (That's what most of the codebase does indeed). Going riskier paths to save a couple lines doesn't seem sane.

> @@ +469,5 @@
> >      DOMStorageImpl::gStorageDB->RemoveOwner(aceDomain, true);
> > +  } else if (!strcmp(aTopic, "profile-before-change")) {
> > +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> > +    if (os)
> > +      (void)os->RemoveObserver(this, "profile-before-change");
> 
> "profile-before-change" may come more then ones in an app life time.  E.g.
> SeaMonkey supports profile switching.

Profile switching on-the-fly? I thought that could not work right now for various platforms issues.
does not profile switching just fire a profile-do-change?

Comment 31

[Marco Bonardo [:mak]]

btw notice that shutting down databases at xpcom-shutdown is wrong, they must be closed at profile notifications.

Comment 32

[Marco Bonardo [:mak]]

So I briefly asked around, Seamonkey does not profile switch during the same app instance, it restarts the app. xpfe was doing that but they are not using that path anymore. It was originally planned but never implemented correctly.
There is an effort to try to support that in future that we may try to be friendly with, but nothing more.

Comment 33

[Honza Bambas (:mayhemer)]

(In reply to Marco Bonardo [:mak] from comment #30)
> indeed it's easier to removeObserver in js objects and services (That's what
> most of the codebase does indeed). Going riskier paths to save a couple
> lines doesn't seem sane.

Au contraire, removing the observer too soon or too late is what seems to me riskier.  For those objects we either keep reference to (the example in URL in comment 29) or is a service (all that this patch is touching) we are very safe to use weak referencing.  That is my strong opinion.

> Profile switching on-the-fly? I thought that could not work right now for
> various platforms issues.
> does not profile switching just fire a profile-do-change?

This is a new information to me.  But you may know more.  Profile start event is usually profile-after-change and profile tear down event is profile-before-change.  I obey rule to respect there can be more profiles running during one application's lifetime.

I would prefer to be compatible with that.  It is not that much work to achieve that.

(In reply to Marco Bonardo [:mak] from comment #31)
> btw notice that shutting down databases at xpcom-shutdown is wrong, they
> must be closed at profile notifications.

Absolutely agree with that.

(In reply to Marco Bonardo [:mak] from comment #32)
> There is an effort to try to support that in future that we may try to be
> friendly with, but nothing more.

That supports my comment above.

Comment 34

[Marco Bonardo [:mak]]

(In reply to Honza Bambas (:mayhemer) from comment #33)
> This is a new information to me.  But you may know more.  Profile start
> event is usually profile-after-change and profile tear down event is
> profile-before-change.  I obey rule to respect there can be more profiles
> running during one application's lifetime.

You are right, ideally when profile switching was supported after-change and do-change were marking initialization, change-teardown and before-change finalization.  We stopped supporting that some time ago, I think about same time when we stopped restarting the process on startup. A bunch of components are broken regarding that, and I have not seen the will to retry that road talking around with some toolkit peers.
Btw, you are the most near thing to a module owner here, so we will respect your will to stay compatible with multiple profiles. Should mostly be matter of resetting things so they can be restarted on request. My next patches feedbacks will take this will into account.

Comment 35

[Marco Bonardo [:mak]]

Comment on attachment 568793 [details] [diff] [review]
Finish statements

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

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +271,5 @@
> +      this.__stmtDeleteGroupIfUnused.finalize()
> +    if (this.__stmtDeletePref)
> +      this.__stmtDeletePref.finalize()
> +    if (this.__stmtUpdatePref)
> +      this.__stmtUpdatePref.finalize()

heh, sorry, you should also set all of these to null, after finalize(), to allow restarting them (see the profile switch discussion, etc) through the getters.

I just figured this components shuts down at xpcom-shutdown... sigh. btw, don't want to add more work on you now, so let's proceed for now.

Comment 36

[Marco Bonardo [:mak]]

ok, so the only remaining piece to finish is the one in bug 696399, that touches DOM. please move any discussion and review related to DOM Storage there.

Comment 37

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

Attachment: Finish statements

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

Comment 38

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d44e0defe38

Comment 39

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/4d44e0defe38