696399 - close connections and misc cleanups in dom

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Attachment: close connections and misc cleanups in dom

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 568675 [details] [diff] [review]
close connections and misc cleanups in dom

mayhemer would make a much better reviewer here.

Comment 2

[Marco Bonardo [:mak]]

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 3

[Honza Bambas (:mayhemer)]

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

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Marco Bonardo [:mak]]

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 6

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

Attachment: Finish statements

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

Comment 7

[Marco Bonardo [:mak]]

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).

Comment 8

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

Attachment: close connections

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?

Comment 9

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

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 10

[Marco Bonardo [:mak]]

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()

Comment 11

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

Attachment: close connections

Includes the changes mak asked for in the previous review.

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

Comment 12

[Marco Bonardo [:mak]]

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 13

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

Attachment: close connections and assert that it worked.

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

Comment 14

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

Attachment: close connections and assert that it worked.

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

Comment 15

[Marco Bonardo [:mak]]

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 16

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

Attachment: close connections and assert that it worked.

Comment 17

[Honza Bambas (:mayhemer)]

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.

Comment 18

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

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

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1b9344ef3810

Comment 20

[Marco Bonardo [:mak]]

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

Comment 21

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

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

Comment 22

[Matt Brubeck (:mbrubeck)]

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