711076 - Check that we call onnection::internalClose on every connection we called Connection::initialize on

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Comment 1

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

Attachment: Check that we call onnection::internalClose on every connection we called Connection::initialize on

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

Comment 2

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

An updated patch is at
https://tbpl.mozilla.org/?tree=Try&rev=a6fd4b6509df

It include the fixes for bugs 711447 and 711494.

Comment 3

[Shawn Wilsher :sdwilsh]

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

Comment 4

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

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

Comment 5

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

I pushed a new combined patch to

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

Comment 6

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

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

Comment 7

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

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.

Comment 8

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

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

Comment 9

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

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

Comment 10

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

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

Comment 11

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

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

Comment 12

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

and now on top of a working revision:

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

Comment 13

[(dormant account)]

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

Comment 14

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

Attachment: check that all connections are closed.

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

Comment 15

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

ping

Comment 16

[Marco Bonardo [:mak]]

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 17

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

Attachment: check that all connections are closed.

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

Comment 18

[Marco Bonardo [:mak]]

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?

Comment 19

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

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

Not calling Length at each iteration.

Comment 20

[Marco Bonardo [:mak]]

ah ok, thought there was some exotic reason :)

Comment 21

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

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

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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