Close old connections when switching to/from private mode

RESOLVED FIXED in mozilla14

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy])

Attachments

(1 attachment, 8 obsolete attachments)

Comment hidden (empty)
Created attachment 605776 [details] [diff] [review]
close connection
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Created attachment 605818 [details] [diff] [review]
close connection
Attachment #605776 - Attachment is obsolete: true
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.
Attachment #606086 - Flags: review?(mak77) → feedback?(paolo.mozmail)

Comment 6

7 years ago
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).
Attachment #606086 - Flags: feedback?(paolo.mozmail)
> 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.
(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?
Created attachment 607581 [details] [diff] [review]
close connection

https://tbpl.mozilla.org/?tree=Try&rev=57d468648c6e
Attachment #606086 - Attachment is obsolete: true
Attachment #607581 - Flags: review?(mak77)
Attachment #607581 - Flags: feedback?(paolo.mozmail)
I think paolo is seeing assertions with the new download panel that indeed uses some async statement.
(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.
Created attachment 607598 [details] [diff] [review]
don't redefine rv

https://hg.mozilla.org/try/rev/dec28f57ad4d
Attachment #607581 - Attachment is obsolete: true
Attachment #607581 - Flags: review?(mak77)
Attachment #607581 - Flags: feedback?(paolo.mozmail)
Attachment #607598 - Flags: review?(mak77)
Attachment #607598 - Flags: feedback?(paolo.mozmail)

Comment 13

7 years ago
(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

7 years ago
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).
Attachment #607598 - Flags: feedback?(paolo.mozmail)
Created attachment 607712 [details] [diff] [review]
factor finalizing and closing the db to a helper method

https://tbpl.mozilla.org/?tree=Try&rev=49644fd6e83f
Attachment #607598 - Attachment is obsolete: true
Attachment #607598 - Flags: review?(mak77)
Attachment #607712 - Flags: review?(mak77)
Attachment #607712 - Flags: feedback?(paolo.mozmail)
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
Created attachment 607935 [details] [diff] [review]
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.
Attachment #607712 - Attachment is obsolete: true
Attachment #607712 - Flags: review?(mak77)
Attachment #607712 - Flags: feedback?(paolo.mozmail)
Attachment #607935 - Flags: review?(mak77)
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
> 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?
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.
(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 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
Attachment #607943 - Flags: review?(mak77) → review+
Whiteboard: [snappy]
https://hg.mozilla.org/mozilla-central/rev/7839dd4bcdb5
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.