Comment on attachment 568675 [details] [diff] [review] close connections and misc cleanups in dom mayhemer would make a much better reviewer here.

Comment on attachment 568675 [details] [diff] [review] close connections and misc cleanups in dom Review of attachment 568675 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/storage/nsDOMStorage.cpp @@ +281,4 @@ > if (!os) > return NS_OK; > > + // FIXME: we should RemoveObserver these somewhere. file a followup bug and add its bug number here

Comment on attachment 568675 [details] [diff] [review] close connections and misc cleanups in dom Review of attachment 568675 [details] [diff] [review]: ----------------------------------------------------------------- Ah, please see the bug 688913 comment 29 for review details on this patch. r- Please, create your patches with context of 8 lines, add to .hgrc: [defaults] diff=-U 8 -p qdiff=-U 8 [diff] git=true showfunc=true unified=8

So, here is left to address DOMStorage review comments in the first part of https://bugzilla.mozilla.org/show_bug.cgi?id=688913#c29

here is a copy of the review comments from honza: > ::: 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. >

Comment on attachment 576222 [details] [diff] [review] Finish statements Review of attachment 576222 [details] [diff] [review]: ----------------------------------------------------------------- Am I wrong or this does not yet close the connection (mConnection, exactly), while both the method names and the comments suggest it is doing? I mean, surely you may let the destructor do that, but it's fairly after profile-before-change, that is when we should stop doing profile work. Btw, again Honza is a better reviewer here. ::: dom/src/storage/nsDOMStorage.cpp @@ +483,5 @@ > rv = DOMStorageImpl::InitDB(); > NS_ENSURE_SUCCESS(rv, rv); > > DOMStorageImpl::gStorageDB->RemoveOwner(aceDomain, true); > + } else if (!strcmp(aTopic, "profile-before-change")) { you can't just remove XPCOM_SHUTDOWN from here without removing the addObserver from Initialize() @@ +489,5 @@ > DebugOnly<nsresult> rv = > DOMStorageImpl::gStorageDB->FlushAndDeleteTemporaryTables(true); > NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), > "DOMStorage: temporary table commit failed"); > + DOMStorageImpl::gStorageDB->Close(); Since it was said this components wants to allow restarting on a new profile, I think at this point may make sense to delete and nullify gStorageDB after invoking Close on it, since otherwise InitDB() would be unable to reinit it on a new database (honestly, this already looks broken, I can't see how it could move to another database if it doesn't even close the current connection).

Updated patch that also closes the connection. https://tbpl.mozilla.org/?tree=Try&rev=db34f6446f8c Alternatively, if there are still more statements to be finalized on these connections, would a patch with just the "finalize" bits be OK?

I was never able to reproduce the mochitest-1 failure and did a new push to https://tbpl.mozilla.org/?tree=Try&rev=7c0ce7c7588b to check if the problem was not in the revision I pushed on top.

Comment on attachment 576556 [details] [diff] [review] close connections Review of attachment 576556 [details] [diff] [review]: ----------------------------------------------------------------- looks fine with a couple changes ::: dom/src/storage/nsDOMStorage.cpp @@ +489,5 @@ > NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), > "DOMStorage: temporary table commit failed"); > + DOMStorageImpl::gStorageDB->Close(); > + delete DOMStorageImpl::gStorageDB; > + DOMStorageImpl::gStorageDB = NULL; nsnull please ::: dom/src/storage/nsDOMStoragePersistentDB.cpp @@ +463,5 @@ > + mRemoveAllStatement = nsnull; > + mGetOfflineExcludedUsageStatement = nsnull; > + mGetFullUsageStatement = nsnull; > + > + mConnection->AsyncClose(NULL); Why AsyncClose? I don't think DOMStorage is using any asynchronous API. So this should just be Close()

Includes the changes mak asked for in the previous review. https://tbpl.mozilla.org/?tree=Try&rev=f175caa791bf

Comment on attachment 576967 [details] [diff] [review] close connections Review of attachment 576967 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me, a small hint that I just thought of: ::: dom/src/storage/nsDOMStoragePersistentDB.cpp @@ +463,5 @@ > + mRemoveAllStatement = nsnull; > + mGetOfflineExcludedUsageStatement = nsnull; > + mGetFullUsageStatement = nsnull; > + > + mConnection->Close(); I suggest adding a breaking assertion if Close() fails, since that would mean something has not been properly finalized, and would be much easier to catch that kind of errors. Something like: nsDebugOnly<nsresult> rv = mConnection->Close(); MOZ_ASSERT(NS_SUCCEEDED(rv));

Comment on attachment 577085 [details] [diff] [review] close connections and assert that it worked. Review of attachment 577085 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/storage/nsDOMStoragePersistentDB.cpp @@ +463,5 @@ > + mRemoveAllStatement = nsnull; > + mGetOfflineExcludedUsageStatement = nsnull; > + mGetFullUsageStatement = nsnull; > + > + mozilla::DebugOnly<nsresult> rv = mConnection->Close(); I think it'd be fine if you would just add to the top of the file a using namespace mozilla; in future there may be more usage of namespaced utils from it.

Comment on attachment 577290 [details] [diff] [review] close connections and assert that it worked. Review of attachment 577290 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with following comments, try is green. ::: dom/src/storage/nsDOMStorage.cpp @@ +489,5 @@ > NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), > "DOMStorage: temporary table commit failed"); > + DOMStorageImpl::gStorageDB->Close(); > + delete DOMStorageImpl::gStorageDB; > + DOMStorageImpl::gStorageDB = nsnull; Please, create a static method nsDOMStorageManager::ShutdownDB() for deleting gStorageDB. Then use this new method here and in nsDOMStorageManager::Shutdown(). ::: dom/src/storage/nsDOMStoragePersistentDB.cpp @@ +449,5 @@ > return NS_OK; > } > > +void > +nsDOMStoragePersistentDB::Close() { nsDOMStoragePersistentDB::Close() { Open brackets on a new line. ::: dom/src/storage/nsDOMStoragePersistentDB.h @@ +65,5 @@ > /** > + * Close the connection, finalizing all the cached statements. > + */ > + void > + Close(); Maybe also worth to add a comment bellow the statement members declaration instructing programmers to add newly added statements to the Close() method too.

hm looks like this has been backed out, but the backout not annotated here.

yes, sorry. I misunderstood what was requested to be in nsDOMStorageManager::ShutdownDB. A new push is in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b6811f220aba