Last Comment Bug 696399 - close connections and misc cleanups in dom
: close connections and misc cleanups in dom
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on: 697989
Blocks: 662444 687726
  Show dependency treegraph
 
Reported: 2011-10-21 09:22 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-12-01 11:37 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
close connections and misc cleanups in dom (5.45 KB, patch)
2011-10-21 09:22 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
honzab.moz: review-
mak77: feedback+
Details | Diff | Splinter Review
Finish statements (4.69 KB, patch)
2011-11-22 11:58 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: feedback-
Details | Diff | Splinter Review
close connections (5.55 KB, patch)
2011-11-23 11:11 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: feedback+
Details | Diff | Splinter Review
close connections (5.54 KB, patch)
2011-11-25 10:30 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: feedback+
Details | Diff | Splinter Review
close connections and assert that it worked. (5.60 KB, patch)
2011-11-26 07:09 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: feedback+
Details | Diff | Splinter Review
close connections and assert that it worked. (5.61 KB, patch)
2011-11-26 10:14 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: feedback+
Details | Diff | Splinter Review
close connections and assert that it worked. (6.21 KB, patch)
2011-11-28 09:01 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
honzab.moz: review+
mak77: feedback+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-10-21 09:22:40 PDT
Created attachment 568675 [details] [diff] [review]
close connections and misc cleanups in dom
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-21 09:26:25 PDT
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] (Away 6-20 Aug) 2011-10-21 14:56:23 PDT
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) 2011-10-21 15:22:25 PDT
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] (Away 6-20 Aug) 2011-10-25 07:32:35 PDT
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] (Away 6-20 Aug) 2011-10-25 07:34:10 PDT
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) 2011-11-22 11:58:02 PST
Created attachment 576222 [details] [diff] [review]
Finish statements

https://tbpl.mozilla.org/?tree=Try&rev=99bdf7f011b5
Comment 7 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-11-22 16:36:33 PST
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) 2011-11-23 11:11:56 PST
Created attachment 576556 [details] [diff] [review]
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) 2011-11-24 12:12:18 PST
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] (Away 6-20 Aug) 2011-11-25 02:18:33 PST
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) 2011-11-25 10:30:46 PST
Created attachment 576967 [details] [diff] [review]
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] (Away 6-20 Aug) 2011-11-26 01:34:15 PST
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) 2011-11-26 07:09:39 PST
Created attachment 577069 [details] [diff] [review]
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) 2011-11-26 10:14:38 PST
Created attachment 577085 [details] [diff] [review]
close connections and assert that it worked.

https://tbpl.mozilla.org/?tree=Try&rev=f58a935ac647
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-11-28 05:54:12 PST
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) 2011-11-28 09:01:34 PST
Created attachment 577290 [details] [diff] [review]
close connections and assert that it worked.
Comment 17 Honza Bambas (:mayhemer) 2011-11-30 14:45:24 PST
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) 2011-11-30 16:33:17 PST
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1b9344ef3810
Comment 19 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-01 04:46:26 PST
https://hg.mozilla.org/mozilla-central/rev/1b9344ef3810
Comment 20 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-01 04:48:34 PST
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) 2011-12-01 05:32:16 PST
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) 2011-12-01 11:37:35 PST
https://hg.mozilla.org/mozilla-central/rev/b6811f220aba

Note You need to log in before you can comment on or make changes to this bug.