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

RESOLVED FIXED in mozilla15

Status

()

Product:
Toolkit
Component:
Storage
Type:
defect
Status:
RESOLVED FIXED
Opened:
8 years ago
Updated:
7 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

(Blocks 1 bug)

Version:
unspecified
Target:
mozilla15
Platform:
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

check that all connections are closed.
1.14 KB, patch
mak
: review+
Details | Diff | Splinter Review 
No description provided.
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.
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.
Whiteboard: [snappy]
No serious need to mark correctness fixes as snappy if they block existing snappy bugs.
Blocks: 662444
Whiteboard: [snappy]
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.
Attachment #623190 - Flags: review?(mak77)
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?
Attachment #626775 - Flags: review?(mak77) → review+
> > +    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 :)
https://hg.mozilla.org/mozilla-central/rev/b842c7baf11c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 758688
Depends on: 759078
Depends on: 760243
You need to log in before you can comment on or make changes to this bug.