Now that I think of it, sOpenConnectionCount should probably be a file static. The MOZ_ASSERT might also have to be an NS_ASSERTION for now. I will add a reviewer once I have the try results. https://tbpl.mozilla.org/?tree=Try&rev=1c9e56225e3d

An updated patch is at https://tbpl.mozilla.org/?tree=Try&rev=a6fd4b6509df It include the fixes for bugs 711447 and 711494.

I don't understand why we need this. Can you elaborate?

(In reply to Shawn Wilsher :sdwilsh (Vacation Dec 17 - Jan 5) from comment #3) > I don't understand why we need this. Can you elaborate? It is a sanity check. Making sure connections are closed makes sure that all relevant statements are finalized and the corresponding threads are shut down. A thread that sometimes survives too long is the cause of 673017 for example.

I pushed a new combined patch to https://tbpl.mozilla.org/?tree=Try&rev=0209f9c7d338

And now with a missing null check added: https://tbpl.mozilla.org/?tree=Try&rev=c7e4b3f4413a

The xpcshell failure was because places was not spinning. A new push is at https://tbpl.mozilla.org/?tree=Try&rev=f8b24142c8ed If this is green I will split and send for review.

A new combined patch is at https://tbpl.mozilla.org/?tree=Try&rev=398cde3291f2

New try push at https://tbpl.mozilla.org/?tree=Try&rev=d5f7d7218ecc

New try push at https://tbpl.mozilla.org/?tree=Try&rev=d8591f907567

new push at https://tbpl.mozilla.org/?tree=Try&rev=ad06f61d0f5d

and now on top of a working revision: https://tbpl.mozilla.org/?tree=Try&rev=807c02faf7c5

No serious need to mark correctness fixes as snappy if they block existing snappy bugs.

ping

Comment on attachment 623190 [details] [diff] [review] check that all connections are closed. Review of attachment 623190 [details] [diff] [review]: ----------------------------------------------------------------- will r+ next version ::: storage/src/mozStorageService.cpp @@ +884,5 @@ > + nsTArray<nsRefPtr<Connection> > connections; > + getConnections(connections); > + for (PRUint32 i = 0; i < connections.Length(); i++) { > + MOZ_ASSERT(!connections[i]->ConnectionReady()); > + } all of this is debug, since you just use MOZ_ASSERT, so you can well wrap everything into an ifdef DEBUG and avoid looping at runtime.

Comment on attachment 626775 [details] [diff] [review] check that all connections are closed. Review of attachment 626775 [details] [diff] [review]: ----------------------------------------------------------------- ::: storage/src/mozStorageService.cpp @@ +847,5 @@ > + > +#ifdef DEBUG > + nsTArray<nsRefPtr<Connection> > connections; > + getConnections(connections); > + for (PRUint32 i = 0, n = connections.Length(); i < n; i++) { why the temporary n var?

> > + for (PRUint32 i = 0, n = connections.Length(); i < n; i++) { > > why the temporary n var? Not calling Length at each iteration.

ah ok, thought there was some exotic reason :)