Last Comment Bug 702344 - (PermissionManagerIO) Squash main thread PermissionManager sqlite outputs (moz_hosts table)
(PermissionManagerIO)
: Squash main thread PermissionManager sqlite outputs (moz_hosts table)
Status: RESOLVED FIXED
[Snappy:P1]
: main-thread-io, perf
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
: 711238 (view as bug list)
Depends on: 683808
Blocks: StorageJank 750312 724878
  Show dependency treegraph
 
Reported: 2011-11-14 10:41 PST by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-09-11 04:01 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First item on the list. Waiting for feedback to be sure that it could not have unwanted side-effects. (2.19 KB, patch)
2011-11-14 10:44 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (8.13 KB, patch)
2012-02-08 01:34 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dwitte: review-
Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (8.40 KB, patch)
2012-03-26 06:13 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (11.47 KB, patch)
2012-03-26 14:39 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (10.71 KB, patch)
2012-04-04 06:43 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
mak77: feedback+
Details | Diff | Review
Complete patch (34.20 KB, patch)
2012-04-04 08:13 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (10.23 KB, patch)
2012-04-30 09:24 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (10.31 KB, patch)
2012-05-03 02:25 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (4.62 KB, patch)
2012-05-27 03:38 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (9.81 KB, patch)
2012-05-27 04:12 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
mak77: review-
Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (10.59 KB, patch)
2012-06-07 09:55 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (10.29 KB, patch)
2012-06-08 02:37 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (10.19 KB, patch)
2012-06-12 07:27 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
mak77: review+
Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (9.99 KB, patch)
2012-06-15 02:47 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
mak77: review+
Details | Diff | Review
Async requests for nsPermissionManager::RemoveAll() (10.00 KB, patch)
2012-06-15 03:44 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-11-14 10:41:56 PST
Track every single places SQL IO on the main thread, get rid of them.

As soon as data from 699051 is available, use it to choose highest effect IO.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-11-14 10:44:01 PST
Created attachment 574342 [details] [diff] [review]
First item on the list. Waiting for feedback to be sure that it could not have unwanted side-effects.
Comment 2 Marco Bonardo [::mak] 2011-11-14 10:58:38 PST
First, we already have a tracking bug, that is bug 699820.
Second, the patch here is not Places at all, and please stop confusing any storage users by places. places is places, sqlite is sqlite, storage is storage, cookies are cookies.
This should be filed as a cookies bug and block bug 699820.

This bug, as it is, is invalid.
Comment 3 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-11-14 11:02:58 PST
Please accept my apologies for the misfiling. I freely admit that, when I find a main thread sql IO, I have difficulties finding out tracing it back to.
Comment 4 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-11-14 11:03:46 PST
I meant "finding out from which component it depends".
Comment 5 Marco Bonardo [::mak] 2011-11-14 11:19:20 PST
It's easy, if you read history, bookmarks or favicons, it's places, otherwise it's not :)
Comment 6 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-11-16 00:38:21 PST
Adding Daniel Witte, Michiel van Leeuwen.
Comment 7 dwitte@gmail.com 2011-11-17 12:41:43 PST
This is the right approach, but doing it properly takes a little more work.

In cookies, for instance, we have async error handlers for each operation; if they fail, it will rebuild the db from memory with whatever data it has. Currently, the permission manager doesn't do any of that, so we don't need to go that far here.

Reading of the db on startup will still be synchronous, which is a problem. This patch only addresses the O part. We should have a followup for the I part. :)

Making reading asynchronous is a bit trickier. Take a look at the cookie service if you want to know the details. It involves creating an index on base domain, and reading in domains as required. (A simpler approach is to kick off an async read, and cancel it in favor of a sync read if the getter is called; but that only makes sense if the permission manager is instantiated immediately at startup.)

Anyway, for this patch, there are a few simple optimizations you can make:
1. Set the growth increment to be larger; http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#771
2. Create async statements (for write operations only) instead of sync ones; http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1025
3. Make removing all an async operation as well; http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#630
4. Delete the code directly following that, which resets the statements and reopens the db; http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#635 (I'm not sure why that's there, perhaps a holdover from a time when we didn't secure delete, or perhaps to shrink the db after a delete)

Another thing I noticed is that we don't delete the permissions database if reading it in fails. We really should do that. Want to take a stab at it? It involves splitting out InitDB() into two methods; InitDB() and TryInitDB(). If the latter returns an error code, InitDB() deletes the database and calls TryInitDB() again. You'll have to reset the statements and db connection like so: http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#632 and then delete the database like so: http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#262

Let me know if you need any help!
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-11-17 13:32:05 PST
Looks like a bigger refactoring than expected. While we are at it, I wonder whether cookies would not be a good candidate for porting to LevelDB.
Comment 9 dwitte@gmail.com 2011-11-17 13:37:29 PST
It's not that hard, just a few lines of code here and there. The functional changes are small.
Comment 10 dwitte@gmail.com 2011-11-18 11:23:52 PST
Note: to be clear, a bunch of the stuff in comment 7 is definitely not scoped for this bug. Just the bits I numbered. Deleting the database if reading fails could also be split into another bug, but it's a simple change that you could also just roll in here.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-12-15 13:38:54 PST
*** Bug 711238 has been marked as a duplicate of this bug. ***
Comment 12 dwitte@gmail.com 2011-12-21 12:50:39 PST
David, still interested in working on this? Let me know if you need any help.
Comment 13 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-02 01:53:07 PST
(In reply to Dan Witte (:dwitte) (not reading bugmail, email to contact) from comment #12)
> David, still interested in working on this? Let me know if you need any help.

Still, interested, only the priority of OS.File has increased.
Comment 14 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-03 10:05:39 PST
(In reply to David Rajchenbach Teller [:Yoric] from comment #13)
> (In reply to Dan Witte (:dwitte) (not reading bugmail, email to contact)
> from comment #12)
> > David, still interested in working on this? Let me know if you need any help.
> 
> Still interested, only the priority of OS.File has increased.

I hope to be able to resume work on this bug soon. I will be sure to contact you, Dan.
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-07 07:59:50 PST
Ok, back on it.
(In reply to Dan Witte (:dwitte) (not reading bugmail, email to contact) from comment #7)

Ok, back to work.

> Anyway, for this patch, there are a few simple optimizations you can make:
> 1. Set the growth increment to be larger;
> http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.
> cpp#771

How large?

> 2. Create async statements (for write operations only) instead of sync ones;
> http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.
> cpp#1025

mak beat me to this.

> 3. Make removing all an async operation as well;
> http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> nsPermissionManager.cpp#630

Code seems to have changed. Are you talking about the implementation of http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIPermissionManager.idl#140 (removeAll)?

If so, what do you want me to do in case of error? I can introduce a callback, if necessary.

> 4. Delete the code directly following that, which resets the statements and
> reopens the db;
> http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> nsPermissionManager.cpp#635 (I'm not sure why that's there, perhaps a
> holdover from a time when we didn't secure delete, or perhaps to shrink the
> db after a delete)

> Another thing I noticed is that we don't delete the permissions database if
> reading it in fails. We really should do that. Want to take a stab at it? It
> involves splitting out InitDB() into two methods; InitDB() and TryInitDB().
> If the latter returns an error code, InitDB() deletes the database and calls
> TryInitDB() again. You'll have to reset the statements and db connection
> like so:
> http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> nsPermissionManager.cpp#632 and then delete the database like so:
> http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> nsPermissionManager.cpp#262

Sure. Do you want me to do this in this bug or in a followup?
Comment 16 Marco Bonardo [::mak] 2012-02-07 08:14:55 PST
Rather than caching statements the old way, I suggest using a StatementCache. One day we will modify it to drop stmts unused by a certain amount of time and save memory.
Comment 17 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-08 01:34:59 PST
Created attachment 595347 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()
Comment 18 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-08 01:44:14 PST
Comment on attachment 595347 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Attaching a first version of point 3.
Comment 19 dwitte@gmail.com 2012-03-14 14:52:54 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> > Anyway, for this patch, there are a few simple optimizations you can make:
> > 1. Set the growth increment to be larger;
> > http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.
> > cpp#771
> 
> How large?

128k would be decent. This table shouldn't be large in the common case, so it doesn't need to be as big as cookies.

> > 2. Create async statements (for write operations only) instead of sync ones;
> > http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.
> > cpp#1025
> 
> mak beat me to this.

Great!

> > 3. Make removing all an async operation as well;
> > http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> > nsPermissionManager.cpp#630
> 
> Code seems to have changed. Are you talking about the implementation of
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> nsIPermissionManager.idl#140 (removeAll)?

Yep, this part here: http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#644

> If so, what do you want me to do in case of error? I can introduce a
> callback, if necessary.

Yeah, the current code closes, deletes, and recreates the db on error. You can move that code into an error handler callback. (I said in comment 7 that we don't need it, but... it's probably a good idea to have.)

> > Another thing I noticed is that we don't delete the permissions database if
> > reading it in fails. We really should do that. Want to take a stab at it? It
> > involves splitting out InitDB() into two methods; InitDB() and TryInitDB().
> > If the latter returns an error code, InitDB() deletes the database and calls
> > TryInitDB() again. You'll have to reset the statements and db connection
> > like so:
> > http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> > nsPermissionManager.cpp#632 and then delete the database like so:
> > http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> > nsPermissionManager.cpp#262
> 
> Sure. Do you want me to do this in this bug or in a followup?

Followup for sure. :)
Comment 20 dwitte@gmail.com 2012-03-14 15:16:36 PDT
Comment on attachment 595347 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

>diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp

>+/* void handleCompletion (in unsigned short aReason); */
>+NS_IMETHODIMP RemoveAllListener::HandleCompletion(PRUint16 aReason)
>+{
>+  nsresult rv;
>+  if (aReason & REASON_ERROR) {
>+    this -> mManager -> CloseDB();

Since this is async, the profile may have been changed in between the RemoveAll() call and this code executing. So instead of calling CloseDB(), there has to be a check here that the DB connection the permission manager currently has is the same as the one the errorhandler was called on.

So you need a check that mDBState hasn't changed. For example:
http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#365
http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1214

>+    rv = this -> mManager -> InitDB(true);
>+    if (!NS_SUCCEEDED(rv)) {
>+      this -> mManager.forget(); // Help breaking cycles

Don't think you need the 'this', and prevailing style is for no spacing around '->'.

> NS_IMETHODIMP
> nsPermissionManager::RemoveAll()
> {
>   ENSURE_NOT_CHILD_PROCESS;
> 
>-  nsresult rv = RemoveAllInternal();
>-  NotifyObservers(nsnull, NS_LITERAL_STRING("cleared").get());
>-  return rv;
>+  return RemoveAllInternal(true);

No need to make the notify call async. The in-memory database is authoritative; the sqlite one just backs it on disk. We won't shut down until the async remove operation is complete and the errorhandler is called if necessary, and if we still can't delete the db then there's not much we can do anyway.

That should simplify things a bit!
Comment 21 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-03-26 06:13:11 PDT
Created attachment 609297 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()
Comment 22 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-03-26 14:39:53 PDT
Created attachment 609496 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Turns out that closing a database that uses asynchronous statements is a little more complicated than I first understood. I _think_ I got it now. Dan, Marco, what do you think?
Comment 23 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-03-26 14:41:53 PDT
Note to self: need to restore the order of tests before landing this patch.
Comment 24 Marco Bonardo [::mak] 2012-03-28 09:32:33 PDT
Comment on attachment 609496 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +198,5 @@
> +/* void handleResult (in mozIStorageResultSet aResultSet); */
> +NS_IMETHODIMP
> +CloseDatabaseListener::Complete()
> +{
> +  if (mManager->mDBConn != mDBConn) {

this should be a fatal assertion, should not really be possible we end in this situation

@@ +205,5 @@
> +  PRInt32 lastError;
> +  nsresult rv = mManager->mDBConn->GetLastError(&lastError);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));
> +  if (lastError == 0 /*success*/) {
> +    return mManager->InitDB(true);

what happens otherwise? if it's undefined we probably don't care, and we should not check lastError either.

@@ +277,5 @@
> +NS_IMETHODIMP RemoveAllListener::HandleCompletion(PRUint16 aReason)
> +{
> +  if (aReason & REASON_ERROR) {
> +    // If the connection has somehow changed during the request,
> +    // it is too late to close the database, bail out.

this should not happen, if it happens we are doing it wrong (not waiting properly)

@@ +281,5 @@
> +    // it is too late to close the database, bail out.
> +    if (mManager->mDBConn != mDBConn) {
> +      return NS_OK;
> +    }
> +    mManager->mDBConn->AsyncClose(new CloseDatabaseListener(mManager, true));

So, actually you don't need all of this complication, all async operations are serialized, so if you execute an async query and immediately call asyncClose, those will just happen in that order.

@@ +283,5 @@
> +      return NS_OK;
> +    }
> +    mManager->mDBConn->AsyncClose(new CloseDatabaseListener(mManager, true));
> +  }
> +  mManager.forget(); // Help breaking cycles

just assign 0

@@ +778,5 @@
>    mStmtDelete = nsnull;
>    mStmtUpdate = nsnull;
>    if (mDBConn) {
> +    mozIStorageCompletionCallback* cb(new CloseDatabaseListener(this, false));
> +    mozilla::DebugOnly<nsresult> rv = mDBConn->AsyncClose(cb);

assigning but not using rv

@@ +793,2 @@
>    RemoveAllFromMemory();
> +  NotifyObservers(nsnull, NS_LITERAL_STRING("cleared").get());

shouldn't you check aShouldNotifyObservers? Otherwise what it is for?

@@ +804,2 @@
>      if (NS_FAILED(rv)) {
> +      goto error;

I'm not a big fan of goto, I would personally prefer a temporary macro.

@@ +817,5 @@
>    return NS_OK;
> +
> + error:
> +  CloseDB();
> +  rv = InitDB(true);

heh, I don't think we can do this.
CloseDB is async, so may not have completed when you invoke InitDB(true), that at that point will fail removing the database and thus creating a new one. Unless we spin the events loop after asyncClose :(
Btw, apart this, what's the point of creating a new file if this is invoked cause we are shutting down?
Comment 25 Dietrich Ayala (:dietrich) 2012-04-03 14:53:53 PDT
Dwitte - do you have an idea of when you can look at this patch?

Actually - is a new patch required to address Marco's comments first?
Comment 26 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-04 06:36:08 PDT
(In reply to Marco Bonardo [:mak] from comment #24)
> Comment on attachment 609496 [details] [diff] [review]
> Async requests for nsPermissionManager::RemoveAll()
> 
> Review of attachment 609496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: extensions/cookie/nsPermissionManager.cpp
> @@ +198,5 @@
> > +/* void handleResult (in mozIStorageResultSet aResultSet); */
> > +NS_IMETHODIMP
> > +CloseDatabaseListener::Complete()
> > +{
> > +  if (mManager->mDBConn != mDBConn) {
> 
> this should be a fatal assertion, should not really be possible we end in
> this situation

Ok. What about the other check on the same condition?


> @@ +205,5 @@
> > +  PRInt32 lastError;
> > +  nsresult rv = mManager->mDBConn->GetLastError(&lastError);
> > +  MOZ_ASSERT(NS_SUCCEEDED(rv));
> > +  if (lastError == 0 /*success*/) {
> > +    return mManager->InitDB(true);
> 
> what happens otherwise? if it's undefined we probably don't care, and we
> should not check lastError either.

Ah, you are right, I mistranslated the original algorithm.


> @@ +277,5 @@
> > +NS_IMETHODIMP RemoveAllListener::HandleCompletion(PRUint16 aReason)
> > +{
> > +  if (aReason & REASON_ERROR) {
> > +    // If the connection has somehow changed during the request,
> > +    // it is too late to close the database, bail out.
> 
> this should not happen, if it happens we are doing it wrong (not waiting
> properly)

What should we do, then? MOZ_ASSERT? Or LOG in case of error?


> @@ +281,5 @@
> > +    // it is too late to close the database, bail out.
> > +    if (mManager->mDBConn != mDBConn) {
> > +      return NS_OK;
> > +    }
> > +    mManager->mDBConn->AsyncClose(new CloseDatabaseListener(mManager, true));
> 
> So, actually you don't need all of this complication, all async operations
> are serialized, so if you execute an async query and immediately call
> asyncClose, those will just happen in that order.

Well, the original synchronous algorithm was the following:
- delete the contents of the database;
- in case of failure (and only in case of failure), close the database then recreate it from scratch.

I cannot see any way to do it without all this complication.


> 
> @@ +283,5 @@
> > +      return NS_OK;
> > +    }
> > +    mManager->mDBConn->AsyncClose(new CloseDatabaseListener(mManager, true));
> > +  }
> > +  mManager.forget(); // Help breaking cycles
> 
> just assign 0

Ok. Is it considered a better policy than calling |forget|?

> @@ +778,5 @@
> >    mStmtDelete = nsnull;
> >    mStmtUpdate = nsnull;
> >    if (mDBConn) {
> > +    mozIStorageCompletionCallback* cb(new CloseDatabaseListener(this, false));
> > +    mozilla::DebugOnly<nsresult> rv = mDBConn->AsyncClose(cb);
> 
> assigning but not using rv

Fixed.

> @@ +793,2 @@
> >    RemoveAllFromMemory();
> > +  NotifyObservers(nsnull, NS_LITERAL_STRING("cleared").get());
> 
> shouldn't you check aShouldNotifyObservers? Otherwise what it is for?

Fixed, thanks.

> @@ +804,2 @@
> >      if (NS_FAILED(rv)) {
> > +      goto error;
> 
> I'm not a big fan of goto, I would personally prefer a temporary macro.

I personally dislike either just equally so I will follow your preference and hope that Rust will one day save us :)

Actually, I just realized that
- the only error case here is a programmer error (wrong thread or SQL syntax error);
- catching that error is most likely pointless.

So I will replace this by an assertion, unless anybody has an objection.

> 
> @@ +817,5 @@
> >    return NS_OK;
> > +
> > + error:
> > +  CloseDB();
> > +  rv = InitDB(true);
> 
> heh, I don't think we can do this.
> CloseDB is async, so may not have completed when you invoke InitDB(true),
> that at that point will fail removing the database and thus creating a new
> one. Unless we spin the events loop after asyncClose :(

No, you are right, that was me missing a little bit of rewriting to asynchronous style.

> Btw, apart this, what's the point of creating a new file if this is invoked
> cause we are shutting down?

Probably not very useful, but if I understand correctly, this is what the algorithm did before I arrived. Dan, do you concur that we should not create the file if we are currently shutting down?
Comment 27 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-04 06:43:00 PDT
Created attachment 612170 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()
Comment 28 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-04 06:48:07 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #25) 
> Actually - is a new patch required to address Marco's comments first?

Patch uploaded.
Comment 29 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-04 08:13:25 PDT
Created attachment 612196 [details] [diff] [review]
Complete patch
Comment 30 Marco Bonardo [::mak] 2012-04-23 12:56:30 PDT
Comment on attachment 612170 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

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

yeah, this one looks better

::: extensions/cookie/nsPermissionManager.cpp
@@ +61,2 @@
>  #include "mozStorageHelper.h"
>  #include "mozStorageCID.h"

would rather include "mozilla/storage.h" and remove some of the  single storage includes, most of them should not be needed anymore then.

@@ +167,5 @@
> +  CloseDatabaseListener(nsPermissionManager* aManager,
> +                        bool aRebuildOnSuccess);
> +
> +private:
> +  ~CloseDatabaseListener();

do you need a non-default destructor?

@@ +176,5 @@
> +   * The connection for which this listener was created.
> +   * Used for sanity check, to ensure that the connection has
> +   * not been replaced during the asynchronous call.
> +   */
> +  nsCOMPtr<mozIStorageConnection> mDBConn;

should be ifdef DEBUG, since MOZ_ASSERT is a no-op in release and  you use this only there

@@ +185,5 @@
> +
> +CloseDatabaseListener::CloseDatabaseListener(nsPermissionManager* aManager,
> +                                             bool aRebuildOnSuccess)
> +  : mManager(aManager)
> +  , mDBConn(aManager->mDBConn)

ifdef debug

@@ +199,5 @@
> +CloseDatabaseListener::Complete()
> +{
> +  MOZ_ASSERT(mManager->mDBConn == mDBConn);
> +  nsresult rv = mManager->InitDB(true);
> +  mManager = nsnull;  // Help breaking cycles

would be cleaner if you'd
nsRefPtr<nsPermissionManager> manager;
mManager.forget(manager);
return manager->InitDB(true);

@@ +235,5 @@
> +   * The connection for which this listener was created.
> +   * Used for sanity check, to ensure that the connection has
> +   * not been replaced during the asynchronous call.
> +   */
> +  nsCOMPtr<mozIStorageConnection> mDBConn;

ditto on debug

@@ +248,5 @@
> +}
> +
> +RemoveAllListener::~RemoveAllListener()
> +{
> +}

do you actually need a destructor at all?

@@ +250,5 @@
> +RemoveAllListener::~RemoveAllListener()
> +{
> +}
> +
> +/* void handleResult (in mozIStorageResultSet aResultSet); */

I suppose these comments where just for you reference and you forgot to remove them

@@ +253,5 @@
> +
> +/* void handleResult (in mozIStorageResultSet aResultSet); */
> +NS_IMETHODIMP RemoveAllListener::HandleResult(mozIStorageResultSet *)
> +{
> +  //Results are ignored

Are we actually supposed to get results at all? this should probably be a MOZ_NOT_REACHED("Not expecting any result");

@@ +260,5 @@
> +
> +/* void handleError (in mozIStorageError aError); */
> +NS_IMETHODIMP RemoveAllListener::HandleError(mozIStorageError *)
> +{
> +  //Errors are handled in |HandleCompletion|

whitespace after //

@@ +267,5 @@
> +
> +/* void handleCompletion (in unsigned short aReason); */
> +NS_IMETHODIMP RemoveAllListener::HandleCompletion(PRUint16 aReason)
> +{
> +  if (aReason & REASON_ERROR) {

nit: works but doesn't help much the readability, I'd prefer a common comparison

@@ +271,5 @@
> +  if (aReason & REASON_ERROR) {
> +    MOZ_ASSERT(mManager->mDBConn == mDBConn);
> +    mManager->mDBConn->AsyncClose(new CloseDatabaseListener(mManager, true));
> +  }
> +  mManager = nsnull; // Help breaking cycles

ditto

@@ +762,5 @@
>    mStmtInsert = nsnull;
>    mStmtDelete = nsnull;
>    mStmtUpdate = nsnull;
>    if (mDBConn) {
> +    mozIStorageCompletionCallback* cb(new CloseDatabaseListener(this, false));

nit: "= new" is more common I think

@@ +768,1 @@
>      MOZ_ASSERT(NS_SUCCEEDED(rv));

nit: you may even use MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mDBConn->AsyncClose(cb)));

@@ +775,4 @@
>  {
> +  // Remove from memory and notify immediately
> +  // (the in-memory database is authoritative, so we do not need
> +  // confirmation from the on-disk database to notify observers).

nit:
// Remove from memory and notify immediately.  Since the ...

@@ +780,5 @@
> +  if (aShouldNotifyObservers) {
> +    NotifyObservers(nsnull, NS_LITERAL_STRING("cleared").get());
> +  }
> +
> +  nsresult rv;

you don't need to declare this here, just declare on first use.

@@ +785,4 @@
>  
>    // clear the db
>    if (mDBConn) {
> +    nsCOMPtr<mozIStorageAsyncStatement> removeOp;

nit: removeStmt may be a clearer name

@@ +788,5 @@
> +    nsCOMPtr<mozIStorageAsyncStatement> removeOp;
> +    rv = mDBConn->
> +      CreateAsyncStatement(NS_LITERAL_CSTRING("DELETE FROM moz_hosts"),
> +                           getter_AddRefs(removeOp));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

well I suppose this may fail in some rare cases, like if the connection is in an inconsistent case.  Apart the fatal assertion that is fine, you should probably still handle the null removeOp case for safety, Even by just wrapping the next stuff in an if (removeOp)

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
@@ +1,5 @@
>  [DEFAULT]
>  head = head_addons.js
>  tail = 
>  
> +[test_permissions.js]

this should still be undone
Comment 31 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-30 08:18:29 PDT
(In reply to Marco Bonardo [:mak] from comment #30)
> would rather include "mozilla/storage.h" and remove some of the  single
> storage includes, most of them should not be needed anymore then.

ok.

> do you need a non-default destructor?

Probably not.

> 
> @@ +176,5 @@
> > +   * The connection for which this listener was created.
> > +   * Used for sanity check, to ensure that the connection has
> > +   * not been replaced during the asynchronous call.
> > +   */
> > +  nsCOMPtr<mozIStorageConnection> mDBConn;
> 
> should be ifdef DEBUG, since MOZ_ASSERT is a no-op in release and  you use
> this only there

Good point.
> 
> @@ +185,5 @@
> > +
> > +CloseDatabaseListener::CloseDatabaseListener(nsPermissionManager* aManager,
> > +                                             bool aRebuildOnSuccess)
> > +  : mManager(aManager)
> > +  , mDBConn(aManager->mDBConn)
> 
> ifdef debug
> 
> @@ +199,5 @@
> > +CloseDatabaseListener::Complete()
> > +{
> > +  MOZ_ASSERT(mManager->mDBConn == mDBConn);
> > +  nsresult rv = mManager->InitDB(true);
> > +  mManager = nsnull;  // Help breaking cycles
> 
> would be cleaner if you'd
> nsRefPtr<nsPermissionManager> manager;
> mManager.forget(manager);
> return manager->InitDB(true);

Would the following work?

nsRefPtr<nsPermissionManager> manager(mManager.forget()); // Help breaking cycles
return manager->InitDB(true);

> @@ +248,5 @@
> > +}
> > +
> > +RemoveAllListener::~RemoveAllListener()
> > +{
> > +}
> 
> do you actually need a destructor at all?

Force of habit.

> > +/* void handleResult (in mozIStorageResultSet aResultSet); */
> 
> I suppose these comments where just for you reference and you forgot to
> remove them

Well, in some parts of our tree, these comments stay, in some part they disappear. Not sure where and why. I will just remove it. 

> 
> @@ +253,5 @@
> > +
> > +/* void handleResult (in mozIStorageResultSet aResultSet); */
> > +NS_IMETHODIMP RemoveAllListener::HandleResult(mozIStorageResultSet *)
> > +{
> > +  //Results are ignored
> 
> Are we actually supposed to get results at all? this should probably be a
> MOZ_NOT_REACHED("Not expecting any result");

I may have misunderstood something about mozStorage, but I assumed that there is always a result, isn't it?

> @@ +267,5 @@
> > +
> > +/* void handleCompletion (in unsigned short aReason); */
> > +NS_IMETHODIMP RemoveAllListener::HandleCompletion(PRUint16 aReason)
> > +{
> > +  if (aReason & REASON_ERROR) {
> 
> nit: works but doesn't help much the readability, I'd prefer a common
> comparison

My bad, for some reason, I thought this was an or-able flag.

> 
> @@ +271,5 @@
> > +  if (aReason & REASON_ERROR) {
> > +    MOZ_ASSERT(mManager->mDBConn == mDBConn);
> > +    mManager->mDBConn->AsyncClose(new CloseDatabaseListener(mManager, true));
> > +  }
> > +  mManager = nsnull; // Help breaking cycles
> 
> ditto
> 
> @@ +762,5 @@
> >    mStmtInsert = nsnull;
> >    mStmtDelete = nsnull;
> >    mStmtUpdate = nsnull;
> >    if (mDBConn) {
> > +    mozIStorageCompletionCallback* cb(new CloseDatabaseListener(this, false));
> 
> nit: "= new" is more common I think

ok

> @@ +768,1 @@
> >      MOZ_ASSERT(NS_SUCCEEDED(rv));
> 
> nit: you may even use MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mDBConn->AsyncClose(cb)));

ok

> 
> @@ +775,4 @@
> >  {
> > +  // Remove from memory and notify immediately
> > +  // (the in-memory database is authoritative, so we do not need
> > +  // confirmation from the on-disk database to notify observers).
> 
> nit:
> // Remove from memory and notify immediately.  Since the ...

ok

> 
> @@ +780,5 @@
> > +  if (aShouldNotifyObservers) {
> > +    NotifyObservers(nsnull, NS_LITERAL_STRING("cleared").get());
> > +  }
> > +
> > +  nsresult rv;
> 
> you don't need to declare this here, just declare on first use.

Good point.

> @@ +785,4 @@
> >  
> >    // clear the db
> >    if (mDBConn) {
> > +    nsCOMPtr<mozIStorageAsyncStatement> removeOp;
> 
> nit: removeStmt may be a clearer name

ok

> @@ +788,5 @@
> > +    nsCOMPtr<mozIStorageAsyncStatement> removeOp;
> > +    rv = mDBConn->
> > +      CreateAsyncStatement(NS_LITERAL_CSTRING("DELETE FROM moz_hosts"),
> > +                           getter_AddRefs(removeOp));
> > +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> 
> well I suppose this may fail in some rare cases, like if the connection is
> in an inconsistent case.  Apart the fatal assertion that is fine, you should
> probably still handle the null removeOp case for safety, Even by just
> wrapping the next stuff in an if (removeOp)

So, basically, I should defend against me writing a typo?
Just a little bit scary, there :)

> ::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
> @@ +1,5 @@
> >  [DEFAULT]
> >  head = head_addons.js
> >  tail = 
> >  
> > +[test_permissions.js]
> 
> this should still be undone
Comment 32 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-30 09:24:20 PDT
Created attachment 619591 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Attaching the updated patch.
Running some more tests, it turns out that all these MOZ_ASSERT I had nicely added can indeed fail if the function is called during shutdown, so I had to remove them.

I will run further tests.
Comment 33 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-03 02:25:12 PDT
Created attachment 620626 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Ok, I had to rework slightly the |forget| part to avoid a few leaks, but the tests now pass.

Dwitte, I would really need your feedback to know if this is what you wanted me to do.
Comment 34 Marco Bonardo [::mak] 2012-05-08 09:57:58 PDT
Comment on attachment 620626 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +180,5 @@
> +  : mManager(aManager)
> +#if defined(DEBUG)
> +  , mDBConn(aManager->mDBConn)
> +#endif // defined(DEBUG)
> +  , mRebuildOnSuccess(aRebuildOnSuccess)

Looks like you are not using this anywhere?

@@ +254,5 @@
> +}
> +
> +NS_IMETHODIMP RemoveAllListener::HandleCompletion(PRUint16 aReason)
> +{
> +  nsRefPtr<nsPermissionManager> manager(mManager.forget()); // Help breaking cycles

hm, maybe you are forgetting before assigning to a new owner, I think you should mManager.forget(manager);

@@ +259,5 @@
> +
> +  // Handle errors, except during shutdown
> +  if (aReason == REASON_ERROR && manager->mDBConn) {
> +    MOZ_ASSERT(manager->mDBConn == mDBConn);
> +    manager->mDBConn->AsyncClose(new CloseDatabaseListener(manager, true));

Sorry I got a bit lost here, why are we closing the connection on error? Don't we already invoke CloseDB() where needed (thus after RemoveAllInternal)?

@@ +752,5 @@
>    mStmtDelete = nsnull;
>    mStmtUpdate = nsnull;
>    if (mDBConn) {
> +    mozIStorageCompletionCallback* cb = new CloseDatabaseListener(this, false);
> +    mDBConn->AsyncClose(cb); // Note: Can fail if we are in the process of shutting down

Strange, we should be closing at profile-before-change, so the comment looks bogus, do you have a stack where this failed?

@@ +773,5 @@
>    if (mDBConn) {
> +    nsCOMPtr<mozIStorageAsyncStatement> removeStmt;
> +    nsresult rv = mDBConn->
> +      CreateAsyncStatement(NS_LITERAL_CSTRING("DELETE FROM moz_hosts"),
> +                           getter_AddRefs(removeStmt));

nit: may indent as

nsresult rv = mDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
  "DELETE FROM moz_hosts"
), getter_AddRefs(removeStmt));

@@ +775,5 @@
> +    nsresult rv = mDBConn->
> +      CreateAsyncStatement(NS_LITERAL_CSTRING("DELETE FROM moz_hosts"),
> +                           getter_AddRefs(removeStmt));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +    if (!removeStmt) { // Defend against typoes?

comment is not needed, mostly defend against unexpected :)

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
@@ +1,5 @@
>  [DEFAULT]
>  head = head_addons.js
>  tail = 
>  
> +[test_permissions.js]

is this still a debug change?
Comment 35 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-16 08:22:16 PDT
Here's the minidump. Crash takes place in CloseDatabaseListener::Complete()


Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  XUL!CloseDatabaseListener::Complete [nsPermissionManager.cpp:fd8b17e549ad : 192 + 0x41]
    rbx = 0x09ef2a20   r12 = 0x019090c6   r13 = 0x0107bc0e   r14 = 0x5fbff0f0
    r15 = 0x00000000   rip = 0x01482874   rsp = 0x5fbfecc0   rbp = 0x5fbfed00
    Found by: given as instruction pointer in context
 1  XUL!mozilla::storage::::CallbackEvent::Run [mozStoragePrivateHelpers.cpp:fd8b17e549ad : 200 + 0x1b]
    rbx = 0x072f6240   r12 = 0x019090c6   r13 = 0x0107bc0e   r14 = 0x5fbff0f0
    r15 = 0x00000000   rip = 0x013d8090   rsp = 0x5fbfed10   rbp = 0x5fbfed20
    Found by: call frame info
 2  XUL!nsThread::ProcessNextEvent [nsThread.cpp:fd8b17e549ad : 656 + 0x17]
    rbx = 0x072f6240   r12 = 0x019090c6   r13 = 0x0107bc0e   r14 = 0x5fbff0f0
    r15 = 0x00000000   rip = 0x01953688   rsp = 0x5fbfed30   rbp = 0x5fbfee10
    Found by: call frame info
 3  XUL!NS_ProcessPendingEvents_P [nsThreadUtils.cpp:fd8b17e549ad : 195 + 0x1c]
    rbx = 0x072f6240   r12 = 0x019090c6   r13 = 0x0107bc0e   r14 = 0x5fbff0f0
    r15 = 0x00000000   rip = 0x018e33c0   rsp = 0x5fbfee20   rbp = 0x5fbfee40
    Found by: call frame info
 4  XUL!mozilla::ShutdownXPCOM [nsXPComInit.cpp:fd8b17e549ad : 613 + 0x15]
    rbx = 0x072f6240   r12 = 0x019090c6   r13 = 0x0107bc0e   r14 = 0x5fbff0f0
    r15 = 0x00000000   rip = 0x018eca11   rsp = 0x5fbfee50   rbp = 0x5fbfef30
    Found by: call frame info
 5  XUL!NS_ShutdownXPCOM_P [nsXPComInit.cpp:fd8b17e549ad : 569 + 0x8]
    rbx = 0x081d5c00   r12 = 0x072825c0   r13 = 0x0107bc0e   r14 = 0x5fbff0f0
    r15 = 0x00000000   rip = 0x018ecf9d   rsp = 0x5fbfef40   rbp = 0x5fbfef50
    Found by: call frame info
 6  libxpcom.dylib!NS_ShutdownXPCOM [nsXPComStub.cpp:fd8b17e549ad : 167 + 0x8]
    rbx = 0x081d5c00   r12 = 0x072825c0   r13 = 0x0107bc0e   r14 = 0x5fbff0f0
    r15 = 0x00000000   rip = 0x0004b112   rsp = 0x5fbfef60   rbp = 0x5fbfef70
    Found by: call frame info
 7  xpcshell!main [xpcshell.cpp:fd8b17e549ad : 2036 + 0x9]
    rbx = 0x081d5c00   r12 = 0x072825c0   r13 = 0x0107bc0e   r14 = 0x5fbff0f0
    r15 = 0x00000000   rip = 0x00006ce5   rsp = 0x5fbfef80   rbp = 0x5fbff270
    Found by: call frame info
 8  xpcshell + 0x16bf
    rbx = 0x00000000   r12 = 0x00000000   r13 = 0x00000000   r14 = 0x00000000
    r15 = 0x00000000   rip = 0x000016c0   rsp = 0x5fbff280   rbp = 0x5fbff280
    Found by: call frame info
Comment 36 dwitte@gmail.com 2012-05-16 11:01:28 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #33)
> Dwitte, I would really need your feedback to know if this is what you wanted
> me to do.

Approach looks good to me! If mak wants to continue doing reviews, that'd be good; otherwise I can take a look at the next one.
Comment 37 Marco Bonardo [::mak] 2012-05-24 03:23:43 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #35)
> Here's the minidump. Crash takes place in CloseDatabaseListener::Complete()

Hm, I thought the failure was coming out from the call to asyncClose, how did you generate this stack? Is this something else?
Maybe would be better if you'd attach a latest version addressing my comments, then we can debug on a common codebase.
Comment 38 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-25 08:24:58 PDT
(In reply to Marco Bonardo [:mak] from comment #34)
> Comment on attachment 620626 [details] [diff] [review]
> Async requests for nsPermissionManager::RemoveAll()
> 
> Review of attachment 620626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: extensions/cookie/nsPermissionManager.cpp
> @@ +180,5 @@
> > +  : mManager(aManager)
> > +#if defined(DEBUG)
> > +  , mDBConn(aManager->mDBConn)
> > +#endif // defined(DEBUG)
> > +  , mRebuildOnSuccess(aRebuildOnSuccess)
> 
> Looks like you are not using this anywhere?

Thanks for the catch, I had indeed forgotten to use it. It is easy to get lost in this (short) code.

> @@ +254,5 @@
> > +}
> > +
> > +NS_IMETHODIMP RemoveAllListener::HandleCompletion(PRUint16 aReason)
> > +{
> > +  nsRefPtr<nsPermissionManager> manager(mManager.forget()); // Help breaking cycles
> 
> hm, maybe you are forgetting before assigning to a new owner, I think you
> should mManager.forget(manager);

The wisdom of crowds seems to hint that I was right :)

> @@ +259,5 @@
> > +
> > +  // Handle errors, except during shutdown
> > +  if (aReason == REASON_ERROR && manager->mDBConn) {
> > +    MOZ_ASSERT(manager->mDBConn == mDBConn);
> > +    manager->mDBConn->AsyncClose(new CloseDatabaseListener(manager, true));
> 
> Sorry I got a bit lost here, why are we closing the connection on error?
> Don't we already invoke CloseDB() where needed (thus after
> RemoveAllInternal)?

Double-checking... yes, there is an issue here, thanks for detecting it.

> 
> @@ +752,5 @@
> >    mStmtDelete = nsnull;
> >    mStmtUpdate = nsnull;
> >    if (mDBConn) {
> > +    mozIStorageCompletionCallback* cb = new CloseDatabaseListener(this, false);
> > +    mDBConn->AsyncClose(cb); // Note: Can fail if we are in the process of shutting down
> 
> Strange, we should be closing at profile-before-change, so the comment looks
> bogus, do you have a stack where this failed?

I think I solved the issue. If so, I was, of course, the guilty party.


> @@ +773,5 @@
> >    if (mDBConn) {
> > +    nsCOMPtr<mozIStorageAsyncStatement> removeStmt;
> > +    nsresult rv = mDBConn->
> > +      CreateAsyncStatement(NS_LITERAL_CSTRING("DELETE FROM moz_hosts"),
> > +                           getter_AddRefs(removeStmt));
> 
> nit: may indent as
> 
> nsresult rv = mDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
>   "DELETE FROM moz_hosts"
> ), getter_AddRefs(removeStmt));

Try telling that to my emacs mode :)
(done)

> @@ +775,5 @@
> > +    nsresult rv = mDBConn->
> > +      CreateAsyncStatement(NS_LITERAL_CSTRING("DELETE FROM moz_hosts"),
> > +                           getter_AddRefs(removeStmt));
> > +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> > +    if (!removeStmt) { // Defend against typoes?
> 
> comment is not needed, mostly defend against unexpected :)

Sure :)

> ::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
> @@ +1,5 @@
> >  [DEFAULT]
> >  head = head_addons.js
> >  tail = 
> >  
> > +[test_permissions.js]
> 
> is this still a debug change?

As I can't remember, removing that change.
Comment 39 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-27 03:38:39 PDT
Created attachment 627549 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

I believe that I have finally got it.
Comment 40 Josh Matthews [:jdm] 2012-05-27 03:49:36 PDT
Comment on attachment 627549 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Wrong patch.
Comment 41 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-27 04:12:51 PDT
Created attachment 627552 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Thanks for the catch.
Comment 42 Marco Bonardo [::mak] 2012-06-05 09:26:42 PDT
Comment on attachment 627552 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

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

looks almost done, there is still a glitch in the error handler though

::: extensions/cookie/nsPermissionManager.cpp
@@ +205,5 @@
> +
> +NS_IMETHODIMP DeleteFromMozHostListener::HandleResult(mozIStorageResultSet *)
> +{
> +  // Results are ignored
> +  return NS_OK;

I'd say more, we don't expect any result, so put a MOZ_ASSERT(true, "Should not get any result"); here, instead of the comment

@@ +223,5 @@
> +
> +  // Handle errors, except during shutdown
> +  if (aReason == REASON_ERROR) {
> +    manager->CloseDB(true);
> +  }

doesn't look like we are handling shutdown here, what does the comment point at?
Looks like in case of error we are still trying to reinit the database, even if we are shutting down.

@@ +713,5 @@
>    mStmtDelete = nsnull;
>    mStmtUpdate = nsnull;
>    if (mDBConn) {
> +    mozIStorageCompletionCallback* cb = new CloseDatabaseListener(this, aRebuildOnSuccess);
> +    mDBConn->AsyncClose(cb);

not sure why you removed the MOZ_ASSERT(rv) from here, I thought you had solved the thing causing it to trigger?
Comment 43 Gregory Szorc [:gps] 2012-06-07 09:00:40 PDT
Latest Try build had crashes in Sync's 4 xpcshell tests related to addon sync. And, I have no clue why. It boggles me that Sync is crashing but extensions xpcshell tests aren't. I suspect Sync isn't doing something properly when loading/unloading the addons manager.
Comment 44 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-07 09:03:10 PDT
For reference, the crashes are here: https://tbpl.mozilla.org/php/getParsedLog.php?id=12453965&tree=Try
Comment 45 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-07 09:53:13 PDT
(In reply to Marco Bonardo [:mak] from comment #42)
> Comment on attachment 627552 [details] [diff] [review]
> Async requests for nsPermissionManager::RemoveAll()
> 
> Review of attachment 627552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks almost done, there is still a glitch in the error handler though
> 
> ::: extensions/cookie/nsPermissionManager.cpp
> @@ +205,5 @@
> > +
> > +NS_IMETHODIMP DeleteFromMozHostListener::HandleResult(mozIStorageResultSet *)
> > +{
> > +  // Results are ignored
> > +  return NS_OK;
> 
> I'd say more, we don't expect any result, so put a MOZ_ASSERT(true, "Should
> not get any result"); here, instead of the comment

|MOZ_ASSERT(true, ...)|?
That is a bit surprising. Shouldn't it be |false| or |MOZ_NOT_REACHED|?

> 
> @@ +223,5 @@
> > +
> > +  // Handle errors, except during shutdown
> > +  if (aReason == REASON_ERROR) {
> > +    manager->CloseDB(true);
> > +  }
> 
> doesn't look like we are handling shutdown here, what does the comment point
> at?
> Looks like in case of error we are still trying to reinit the database, even
> if we are shutting down.

You are correct, the comment was obsolete.

> @@ +713,5 @@
> >    mStmtDelete = nsnull;
> >    mStmtUpdate = nsnull;
> >    if (mDBConn) {
> > +    mozIStorageCompletionCallback* cb = new CloseDatabaseListener(this, aRebuildOnSuccess);
> > +    mDBConn->AsyncClose(cb);
> 
> not sure why you removed the MOZ_ASSERT(rv) from here, I thought you had
> solved the thing causing it to trigger?

Actually, I had solved a different error – thanks for spotting this one, it hid something nasty.
Comment 46 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-07 09:55:08 PDT
Created attachment 631018 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

I may have fixed the issue that kills the Sync tests. Not quite sure what happened between Sync and my code, but the reference to the database connection was improperly protected against race conditions.
Comment 47 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-08 02:37:37 PDT
Created attachment 631319 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()
Comment 48 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-12 07:27:12 PDT
Created attachment 632242 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Fixed more race conditions and a leak.
Comment 49 Marco Bonardo [::mak] 2012-06-14 07:47:58 PDT
Comment on attachment 632242 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

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

r=me, once you clarify that crazy error condition :)

::: extensions/cookie/nsPermissionManager.cpp
@@ +221,5 @@
> +  // Note that the race condition is single-threaded, so introducing a
> +  // mutex would really solve nothing.
> +  if ((aReason == REASON_ERROR) && (manager->mDBConn != dbconn)) {
> +    manager->CloseDB(true);
> +  }

hm, the comment says "rebuild if error, unless mDBConn has changed", but the condition rebuilds if (error && mDBConn changed)... so one of the 2 is wrong, I assume the condition should check "==" ?

nit: the added parenthesis are useless here

::: extensions/cookie/nsPermissionManager.h
@@ +190,5 @@
>    void     NotifyObservers(nsIPermission *aPermission, const PRUnichar *aData);
>  
>    // Finalize all statements, close the DB and null it.
> +  // if aRebuildOnSuccess, reinitialize database
> +  void     CloseDB(bool aRebuildOnSuccess = false);

nit: could you please make this a javadoc?
Comment 50 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-15 02:46:17 PDT
(In reply to Marco Bonardo [:mak] from comment #49)
> Comment on attachment 632242 [details] [diff] [review]
> Async requests for nsPermissionManager::RemoveAll()
> 
> Review of attachment 632242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, once you clarify that crazy error condition :)
> 
> ::: extensions/cookie/nsPermissionManager.cpp
> @@ +221,5 @@
> > +  // Note that the race condition is single-threaded, so introducing a
> > +  // mutex would really solve nothing.
> > +  if ((aReason == REASON_ERROR) && (manager->mDBConn != dbconn)) {
> > +    manager->CloseDB(true);
> > +  }
> 
> hm, the comment says "rebuild if error, unless mDBConn has changed", but the
> condition rebuilds if (error && mDBConn changed)... so one of the 2 is
> wrong, I assume the condition should check "==" ?

Headwall. Sorry about that.

> nit: the added parenthesis are useless here

Is it an issue? I personally find that the presence of these parentheses makes it easier to be sure that the reader has correctly parsed this condition. Anyway, this code has been rewritten in the new patch.
 
> ::: extensions/cookie/nsPermissionManager.h
> >    // Finalize all statements, close the DB and null it.
> > +  // if aRebuildOnSuccess, reinitialize database
> > +  void     CloseDB(bool aRebuildOnSuccess = false);
> 
> nit: could you please make this a javadoc?

Are you sure? In this file, nothing else is a javadoc.
Comment 51 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-15 02:47:51 PDT
Created attachment 633448 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Thanks for the synchronization on IRC yesterday. I had mostly guessed right what the assertion failure was all about, but having confirmation was very useful.
Comment 52 Marco Bonardo [::mak] 2012-06-15 03:06:42 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #50)
> > nit: the added parenthesis are useless here
> 
> Is it an issue?

No, it's a nit :) In some cases parenthesis are useful, but for a single && condition I don't think they are, btw the review is not conditioned on nits.

> > nit: could you please make this a javadoc?
> 
> Are you sure? In this file, nothing else is a javadoc.

Generally when rewriting old code, if possible, we try to make it more properly documented. Btw, as I said, nits don't condition the review.
Comment 53 Marco Bonardo [::mak] 2012-06-15 03:15:51 PDT
Comment on attachment 633448 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

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

apart some minor glitch it still looks fine.

::: extensions/cookie/nsPermissionManager.cpp
@@ +147,5 @@
> +  // Help breaking cycles
> +  nsRefPtr<nsPermissionManager> manager = mManager.forget();
> +  if (mRebuildOnSuccess && !manager->mIsShuttingDown) {
> +    nsresult rv = manager->InitDB(true);
> +    return rv;

why the temp var?

@@ +704,5 @@
>    }
>  }
>  
>  nsresult
> +nsPermissionManager::RemoveAllInternal(bool aInformObservers)

nit: aNotifyObservers (we usually use Notify rather than Inform)

@@ +886,3 @@
>      if (!nsCRT::strcmp(someData, NS_LITERAL_STRING("shutdown-cleanse").get())) {
>        // clear the permissions file
> +      // (this will asynchronously close the db)

// Clear the permissions file and asynchronously close the database.

@@ +886,4 @@
>      if (!nsCRT::strcmp(someData, NS_LITERAL_STRING("shutdown-cleanse").get())) {
>        // clear the permissions file
> +      // (this will asynchronously close the db)
> +      RemoveAllInternal(false);

you called the argument to RemoveAllInternal aIsShuttingDown in the header file, now here you set shuttingDown to true and pass false!
Likely you just forgot to rename the argument in the header.

::: extensions/cookie/nsPermissionManager.h
@@ +221,5 @@
>    // An array to store the strings identifying the different types.
>    nsTArray<nsCString>          mTypeArray;
> +
> +  // Initially, |false|. Set to |true| once shutdown has started, to avoid
> +  // reopening the databa

"the databa" :)
Comment 54 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-15 03:35:46 PDT
(In reply to Marco Bonardo [:mak] from comment #53)
> > +  // Help breaking cycles
> > +  nsRefPtr<nsPermissionManager> manager = mManager.forget();
> > +  if (mRebuildOnSuccess && !manager->mIsShuttingDown) {
> > +    nsresult rv = manager->InitDB(true);
> > +    return rv;
> 
> why the temp var?

Sorry, leftovers from debugging code.

> >        // clear the permissions file
> > +      // (this will asynchronously close the db)
> > +      RemoveAllInternal(false);
> 
> you called the argument to RemoveAllInternal aIsShuttingDown in the header
> file, now here you set shuttingDown to true and pass false!
> Likely you just forgot to rename the argument in the header.

Well-spotted, thanks.

> ::: extensions/cookie/nsPermissionManager.h
> @@ +221,5 @@
> >    // An array to store the strings identifying the different types.
> >    nsTArray<nsCString>          mTypeArray;
> > +
> > +  // Initially, |false|. Set to |true| once shutdown has started, to avoid
> > +  // reopening the databa
> 
> "the databa" :)

Oops :)
Comment 55 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-15 03:44:36 PDT
Created attachment 633470 [details] [diff] [review]
Async requests for nsPermissionManager::RemoveAll()

Fixed. Thanks for the reviews!
Comment 56 Geoff Lankow (:darktrojan) 2012-06-15 22:56:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/290c7dcdeac3
Comment 57 Ryan VanderMeulen [:RyanVM] 2012-06-16 06:49:55 PDT
https://hg.mozilla.org/mozilla-central/rev/290c7dcdeac3

Note You need to log in before you can comment on or make changes to this bug.