sync: take 2 at aborting active HTTP requests.

sync: take 2 at aborting active HTTP requests.

Original issue is codereview.chromium.org/7792022.
Problem was a race between Connection destruction and TerminateAllIO. If TerminateAllIO won the race,
it would call Abort() on a Connection object *after* the BridgedConnection destructor had run, while
~Connection was waiting on the termination lock.  This would cause a purecall exception.

BUG=93829, 19757
TEST=SyncAPIServerConnectionManagerTest, integration tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99874

[tim (not reviewing), 2011-09-06 22:46:39]

Lingesh, please review.

TBR akalin

[lipalani1, 2011-09-06 23:24:24]

LGTM.
But i just reviewed the scopedserverconnectionhelper methods and the methods it
call.

[tim (not reviewing), 2011-09-06 23:29:06]

On 2011/09/06 23:24:24, lipalani1 wrote:
> LGTM.
> But i just reviewed the scopedserverconnectionhelper methods and the methods
it
> call.

Thanks.  The rest was reviewed by Fred earlier, so I'll check this in once try
runs complete.

[tim (not reviewing), 2011-09-06 23:37:16]

On 2011/09/06 23:29:06, timsteele wrote:
> On 2011/09/06 23:24:24, lipalani1 wrote:
> > LGTM.
> > But i just reviewed the scopedserverconnectionhelper methods and the methods
> it
> > call.
> 
> Thanks.  The rest was reviewed by Fred earlier, so I'll check this in once try
> runs complete.

Gah, integration test failures are due to URLBlacklistManager change
crbug.com/95510)... not going to be much use.

[jar (doing other things), 2011-09-07 06:47:41]

Drive by review:

I'm not well educated about the structure of all of this code, but I'm always
concerned about deadlocks (even more so in the current stability push).  

The description of this CL mentioned a "termination lock" which scared me a bit.
 I put a specific comment/question below, and I'm hoping you can give some
details to calm my concerns.

Thanks in advance,

Jim

http://codereview.chromium.org/7841013/diff/1008/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7841013/diff/1008/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:200:
active_connection_ = MakeConnection();
It is usually best practice to use locks to only protect data, and not to
protect code (such as this call to MakeConnection()). 

Calling code (while a lock is held) can often lead to a deadlock.  To prevent
such accidents, when you absolutely have to call code, it is good to note in the
code name that you ahve a lock held.  This prevents accidental efforts to
re-acquire the lock... and makes it hard to call arbitrary code (leading to
deadlocks).

Can you comment on this call, explaining why it is safe to make while the lock
is held, and why it doesn't have any potential for blocking on anything else?

[tim (not reviewing), 2011-09-07 14:46:56]

http://codereview.chromium.org/7841013/diff/1008/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7841013/diff/1008/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/server_connection_manager.cc:200:
active_connection_ = MakeConnection();
On 2011/09/07 06:47:41, jar wrote:
> It is usually best practice to use locks to only protect data, and not to
> protect code (such as this call to MakeConnection()). 
> 
> Calling code (while a lock is held) can often lead to a deadlock.  To prevent
> such accidents, when you absolutely have to call code, it is good to note in
the
> code name that you ahve a lock held.  This prevents accidental efforts to
> re-acquire the lock... and makes it hard to call arbitrary code (leading to
> deadlocks).
> 
> Can you comment on this call, explaining why it is safe to make while the lock
> is held, and why it doesn't have any potential for blocking on anything else?

Thanks for your review Jim.  I get the concerns with deadlocks particularly in
this release.  I'll address some of your fears by saying that this particular
case is quite well understood by this point (it has been around since M5) and is
not the likely culprit for the hangs of doom on m14 (with the history thread /
pref writing etc).  I'll try to address the rest of your fears with more context
-

The lock protects two pieces of data: active_connection_ and terminated_. This
is so that we can safely abort active_connection_, which means assigning to it
when a new connection is created / put in use, and nulling it out when the
connection is done / destroyed.

If you peek at http://codereview.chromium.org/7792022/,  first review comment,
you'll notice that in the original patch the lock was explicitly around setting
/ checking those pieces of data, but the suggestion was to move it into
MakeConnection so that we didn't have to write the same locking logic in two
places.

The only place where we're protecting a call is when using the protected data
(active_connection_) for the actual call to Abort, because that object could
wind up being destroyed if we release the lock.  In this case, the object in
question is the Connection.  That object is designed to have clear and scoped
use by only the ServerConnectionManager, and the public APIs of both of those
objects are in general to be used only from one thread (see thread_checker_ use)
with the exception of aborting.  There is only ever zero or one Connection
object in in existence, and it's lifetime is very clear / well known.  Speaking
as someone who works on this code frequently, adding ref counting and making the
destruction of Connection nondeterministic would have made this code much harder
to follow.