735697 - Close old connections when switching to/from private mode

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Comment 1

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

Attachment: close connection

Comment 2

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

Attachment: close connection

Comment 3

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

Attachment: close connection

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

Comment 4

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

Attachment: close connection

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

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 606086 [details] [diff] [review]
close connection

Review of attachment 606086 [details] [diff] [review]:
-----------------------------------------------------------------

It looks fine to me, though I'm asking for further feedback from Paolo mostly cause he will have to port these changes to the new panel, plus may have more insight having worked on the code so recently.

Some comments follow, will review version fixing these.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +323,5 @@
>  nsDownloadManager::InitMemoryDB()
>  {
> +  if (mDBConn) {
> +    mGetIdsForURIStatement = 0;
> +    mUpdateDownloadStatement = 0;

I'd prefer if you ->Finalize() these, and eventually MOZ_ASSERT the rv on it.

@@ +324,5 @@
>  {
> +  if (mDBConn) {
> +    mGetIdsForURIStatement = 0;
> +    mUpdateDownloadStatement = 0;
> +    mozilla::DebugOnly<nsresult> rv = mDBConn->Close();

this file has "using namespace mozilla", so you can avoid the namespace.

@@ +355,5 @@
> +    mGetIdsForURIStatement = 0;
> +    mUpdateDownloadStatement = 0;
> +    mozilla::DebugOnly<nsresult> rv = mDBConn->Close();
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +  }

ditto

::: toolkit/mozapps/downloads/content/downloads.js
@@ +528,5 @@
>          let dl = getDownload(id.data);
>          removeFromView(dl);
>          break;
> +      case "private-browsing-change-granted":
> +        gStmt.finalize();

please also set it to null, so we don't uselessly finalize() it again in initStatement

Add a comment like:
// Finalize our statements cause the connection will be closed by the service
// during the private browsing transition.

Comment 6

[:Paolo Amadini]

Comment on attachment 606086 [details] [diff] [review]
close connection

I had a quick look, my only observation is that you should call

mDBConn->AsyncClose(nsnull);

because Close is not enough when anyone (in this case the Downloads window) has
executed an asynchronous statement on the connection at any time in the past.

The existing Close call should be fixed too (we're already hitting an assert
when the browser quits).

Comment 7

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

> I'd prefer if you ->Finalize() these, and eventually MOZ_ASSERT the rv on it.

I agree, but I used = 0 because you asked for it in bug 711447. Should I modify that code too?

I will upload an updated patch in a sec.

Comment 8

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

(In reply to Paolo Amadini from comment #6)
> Comment on attachment 606086 [details] [diff] [review]
> close connection
> 
> I had a quick look, my only observation is that you should call
> 
> mDBConn->AsyncClose(nsnull);
> 
> because Close is not enough when anyone (in this case the Downloads window)
> has
> executed an asynchronous statement on the connection at any time in the past.
> 
> The existing Close call should be fixed too (we're already hitting an assert
> when the browser quits).

I am not seeing the assert, what do I have to do to reproduce it?

Comment 9

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

Attachment: close connection

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

Comment 10

[Marco Bonardo [:mak]]

I think paolo is seeing assertions with the new download panel that indeed uses some async statement.

Comment 11

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

(In reply to Marco Bonardo [:mak] from comment #10)
> I think paolo is seeing assertions with the new download panel that indeed
> uses some async statement.

Ah, I see. In which case changing this code to use AsyncClose should be part of that bug, no this one, no? As he noted, we already have a use of Close in the code.

Comment 12

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

Attachment: don't redefine rv

https://hg.mozilla.org/try/rev/dec28f57ad4d

Comment 13

[:Paolo Amadini]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #11)
> Ah, I see. In which case changing this code to use AsyncClose should be part
> of that bug, no this one, no? As he noted, we already have a use of Close in
> the code.

You're right, I remembered incorrectly that the Downloads window now used calls
that worked on the asynchronous thread, but it's still on the main thread (using
timeouts to make the load asynchronous). So, no need to AsyncClose until the
panel lands.

Comment 14

[:Paolo Amadini]

Comment on attachment 607598 [details] [diff] [review]
don't redefine rv

Review of attachment 607598 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +358,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  if (mDBConn) {
> +    mGetIdsForURIStatement = 0;
> +    mUpdateDownloadStatement = 0;

I suppose ->Finalize() should be called inside InitFileDB() too.

Which makes me think that we could create a separate function that finalizes the statements and closes the connection, that would get called at the appropriate time (including the current shutdown path).

Comment 15

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

Attachment: factor finalizing and closing the db to a helper method

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

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 607712 [details] [diff] [review]
factor finalizing and closing the db to a helper method

Review of attachment 607712 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +331,5 @@
> +  mUpdateDownloadStatement = 0;
> +
> +  rv = mDBConn->Close();
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));
> +  mDBConn = NULL;

rather than nullifying the connection (we already had unexpected shutdown crashes with this approach and cached statements), you may GetConnectionReady and check it. if you don't want to duplicate the condition in all points you may just check it at the beginning of CloseDB and have callpoints just invoke it without conditions.

::: toolkit/components/downloads/nsDownloadManager.h
@@ +108,5 @@
>    };
>  
>    nsresult InitDB();
>    nsresult InitFileDB();
> +  void Close();

Both for coherence with existing helpers and for clarity, CloseDB

Comment 17

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

Attachment: don't null the db

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

For the record: I think that not nulling the db is just creating a cozy place for bugs to hide and I am sorry for the next person debugging this.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 607935 [details] [diff] [review]
don't null the db

Review of attachment 607935 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +327,5 @@
> +  mGetIdsForURIStatement = 0;
> +
> +  rv = mUpdateDownloadStatement->Finalize();
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));
> +  mUpdateDownloadStatement = 0;

please don't nullify statements, just finalize them.
It's true that this makes easier to notice crashes and thus misuses, but it's also true doing this we handled a shutdown crash to millions users the last time.
I think it's better to handle this in Storage by making the misuse more visible (fatal assert where something is used at the wrong time) than crashing users.

@@ +1960,5 @@
>      nsDownload *dl2 = FindDownload(id);
>      if (dl2)
>        return CancelDownload(id);
>    } else if (strcmp(aTopic, "profile-before-change") == 0) {
> +    Close();

CloseDB as well

::: toolkit/components/downloads/nsDownloadManager.h
@@ +108,5 @@
>    };
>  
>    nsresult InitDB();
>    nsresult InitFileDB();
> +  void Close();

should be changed to CloseDB

Comment 19

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

> please don't nullify statements, just finalize them.
> It's true that this makes easier to notice crashes and thus misuses, but
> it's also true doing this we handled a shutdown crash to millions users the
> last time.
> I think it's better to handle this in Storage by making the misuse more
> visible (fatal assert where something is used at the wrong time) than
> crashing users.

It is this kind of coding the made the firefox codebase what it is and why I had to spend months hunting down statements to finalize and connections to close. We are just creating the next set of problems for someone else to debug some months from now.

I will attach a new patch, but it is already very different from what I wrote. More importantly, it is not a patch I would r+ myself. Would you mind submitting it as yours if you are ok with it?

Comment 20

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

Attachment: don't null anything

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

Comment 21

[Marco Bonardo [:mak]]

I'd agree with you if we'd be working on a software for internal use, but try to ship a software that crashes each time you shut it down, and tell users that's fine cause it helps the development.  Just see what happened with hang detector that was crashing users on hangs, surely it was useful to us, not to users.
I don't think the changes here make anything harder to do regarding finding not finalized statements, those would still not be finalized nor nullified, you would just detect misuses after shutdown, and I said we need better alternatives to detect those, that don't involve crashing users.

Comment 22

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

(In reply to Marco Bonardo [:mak] from comment #21)
> I'd agree with you if we'd be working on a software for internal use, but
> try to ship a software that crashes each time you shut it down, and tell
> users that's fine cause it helps the development.  Just see what happened
> with hang detector that was crashing users on hangs, surely it was useful to
> us, not to users.
> I don't think the changes here make anything harder to do regarding finding
> not finalized statements, those would still not be finalized nor nullified,
> you would just detect misuses after shutdown, and I said we need better
> alternatives to detect those, that don't involve crashing users.

We agree to disagree, so it is fair to have your name in the code since this version is the one you agree with.

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 607943 [details] [diff] [review]
don't null anything

Review of attachment 607943 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +324,5 @@
> +{
> +  DebugOnly<nsresult> rv = mGetIdsForURIStatement->Finalize();
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));
> +
> +  rv = mUpdateDownloadStatement->Finalize();

nit: the above newline is quite useless

Comment 24

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

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

Comment 25

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7839dd4bcdb5