343332 - thread-safety assertion and crash with nsWeakRefPtr and nsLDAPConnection

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[David :Bienvenu]

The LDAP thread nsIRunnable object holds a weak reference to an nsILDAPConnection object. That weak reference is created on the ui thread, but released on the ldap thread, when the mRunnable object goes away.

Allocated here:

http://lxr.mozilla.org/mozilla/source/directory/xpcom/base/src/nsLDAPConnection.cpp#1021

Defined here:

http://lxr.mozilla.org/mozilla/source/directory/xpcom/base/src/nsLDAPConnection.h#160

released here on the ldap thread:

http://lxr.mozilla.org/mozilla/source/directory/xpcom/base/src/nsLDAPConnection.cpp#829

And finally the runnable object is destroyed here, from an ldap thread:

NSGlue_Assertion(const char * 0x00367650, const char * 0x0036761c, const char * 0x003675e8, int 0x0000006e) line 107
nsWeakReference::Release(nsWeakReference * const 0x044ba9f8) line 110 + 98 bytes
nsCOMPtr<nsIWeakReference>::~nsCOMPtr<nsIWeakReference>() line 584
nsLDAPConnectionLoop::~nsLDAPConnectionLoop() line 575 + 11 bytes
nsLDAPConnectionLoop::`scalar deleting destructor'(unsigned int 0x00000001) + 15 bytes
nsLDAPConnectionLoop::Release(nsLDAPConnectionLoop * const 0x06950898) line 577 + 147 bytes
nsCOMPtr<nsIRunnable>::assign_assuming_AddRef(nsIRunnable * 0x00000000) line 569
nsCOMPtr<nsIRunnable>::assign_with_AddRef(nsISupports * 0x00000000) line 1225
nsCOMPtr<nsIRunnable>::operator=(nsIRunnable * 0x00000000) line 714
nsThread::Main(void * 0x0676ef80) line 135
_PR_NativeRunThread(void * 0x06f68538) line 436 + 13 bytes
pr_root(void * 0x06f68538) line 112 + 13 bytes
_threadstartex(void * 0x06f686c0) line 227 + 13 bytes

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP v1

I originally wrote most of this a while ago, but I've now got around to looking at it again.

The basic plan of this patch is to:

- drop nsLDAPConnectionLoop, this is the loop that currently continuously runs until told to shutdown.
- replace the loop with nsLDAPConnectionRunnable. These are objects that we create, one for each operation.
- When nsLDAPConnection gets a pending operation, it will now add it to its list, create an nsLDAPConnectionRunnable, and dispatch it to the connection thread.
- Once that Run is called on the Runnable, it will get the result, and process it accordingly, either notifying the callers and/or re-dispatching itself. Once the operation finishes, it doesn't re-dispatch itself and just dies (the thread code keeps a reference to the runnable whilst operations are going on).

It still doesn't fix the one case of SSL shutdown error that I can repeat (do a search, quit immediately), I'm starting to think we should be doing an xpcom-shutdown observer to fix that fully.

Comment 2

[Mark Banner (:standard8)]

I've not got time to work on this at the moment. I think the patch basically works, its just the SSL shutdown error that is the problem.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)

This sounds like it wouldn't miss much to be completed.
Would you have time to do it (now)?
Otherwise, would you have an example/instructions to do so?
(and a testcase or an SSL LDAP url to reproduce.)

Comment 4

[Mark Banner (:standard8)]

Not blocking on this at the moment - afaict the crash is rarely hit as the code currently stands. The thread-safety assertion also isn't a major issue.

(In reply to comment #3)
> (In reply to comment #2)
> Otherwise, would you have an example/instructions to do so?
> (and a testcase or an SSL LDAP url to reproduce.)

The end of comment 0 has what needs doing - fix the patch so that we don't crash when the user does a search and quit against an ssl server. The solution would potentially be to hook up an xpcom-shutdown observer.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 355986 [details] [diff] [review]
WIP v1

For patchlove query cleanup purposes, patch has obsoleted:

$ patch -p1 --dry-run < ~/Desktop/p343332.diff 
patching file directory/xpcom/base/src/nsLDAPConnection.cpp
Hunk #1 succeeded at 69 (offset 1 line).
Hunk #2 FAILED at 80.
Hunk #3 FAILED at 93.
Hunk #4 succeeded at 137 (offset 5 lines).
Hunk #5 succeeded at 201 (offset 5 lines).
Hunk #6 succeeded at 221 (offset 5 lines).
Hunk #7 FAILED at 312.
Hunk #8 FAILED at 352.
Hunk #9 succeeded at 569 (offset 25 lines).
Hunk #10 succeeded at 584 (offset 25 lines).
4 out of 10 hunks FAILED -- saving rejects to file directory/xpcom/base/src/nsLDAPConnection.cpp.rej
patching file directory/xpcom/base/src/nsLDAPConnection.h
patching file directory/xpcom/base/src/nsLDAPMessage.h
patching file directory/xpcom/base/src/nsLDAPOperation.cpp
Hunk #1 succeeded at 323 (offset 98 lines).
Hunk #2 succeeded at 336 (offset 98 lines).
Hunk #3 succeeded at 520 (offset 98 lines).
Hunk #4 succeeded at 585 (offset 98 lines).
Hunk #5 succeeded at 732 (offset 98 lines).
Hunk #6 succeeded at 784 (offset 98 lines).
Hunk #7 succeeded at 899 (offset 98 lines).
Hunk #8 succeeded at 964 (offset 98 lines).

Comment 6

[Takanori MATSUURA]

Attachment: Fit attachment 355986 to the latest comm-central

for activating this bug.

I've generated the patch before thinking. This means some new features implemented in the original source file after generating attachment 355986 [details] [diff] [review] may be lost.

I'm only a patch fitter. I hope guru will complete this bug.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #6)
> Created attachment 468590 [details] [diff] [review]
> Fit attachment 355986 [details] [diff] [review] to the latest comm-central
> 
> for activating this bug.
> 
> I've generated the patch before thinking. This means some new features
> implemented in the original source file after generating attachment 355986 [details] [diff] [review] may
> be lost.
> 
> I'm only a patch fitter. I hope guru will complete this bug.

Perhaps feedback should be requested anyway?

Comment 8

[Jan Horak [:jhorak]]

Any progress lately? Seems to be related to #590494.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> Any progress lately? Seems to be related to #590494.

I've not had time recently, hence the helpwanted. Basically this patch needs updating, and then some sort of xpcom-shutdown hook observer to kill the LDAP connection before the app gets shut down.

Comment 10

[Jan Horak [:jhorak]]

Attachment: WIP patch based on 468590

Fixed patch that can be applied on latest comm-central. Looking into xpcom-shutdown hook observer.

Comment 11

[Jan Horak [:jhorak]]

Attachment: WIP patch including test case.

I've added to patch xpcom-shutdown observer to abandon ldap operation. There's no crash during shutdown. However there are leaking urls:
  ldap://ldap.*****.com/dc=redhat,dc=com??sub?(objectclass=*)
  ldap://ldap.*****.com/dc=*****,dc=com?birthday,o,company,mail,mozillaCustom3,custom3,mozillaUseHtmlMail,xmozillausehtmlmail,mozillaCustom2,custom2,mozillaHomeCountryName,ou,department,departmentnumber,orgunit,mobile,cellphone,carphone,telephoneNumber,title,mozillaCustom1,custom1,sn,surname,mozillaNickname,xmozillanickname,mozillaWorkUrl,workurl,labeledURI,facsimiletelephonenumber,fax,mozillaSecondEmail,xmozillasecondemail,mozillaCustom4,custom4,nsAIMid,nscpaimscreenname,street,streetaddress,postOfficeBox,givenName,l,locality,homePhone,mozillaHomeUrl,homeurl,mozillaHomeStreet,st,region,mozillaHomePostalCode,mozillaHomeLocalityName,mozillaWorkStreet2,mozillaHomeStreet2,postalCode,zip,birthmonth,c,countryname,pager,pagerphone,mozillaHomeState,description,notes,birthyear,modifytimestamp,cn,commonname,objectClass?sub?(|(mail=*s*)(cn=*s*)(givenName=*s*)(sn=*s*))

I'm not sure if this is serious issue when leak occurs only when ldap operation is proceeding during shutdown. I've added PR_Sleep to ldap searching thread to be able to test shutting down app during search. I would like to know how far we are from accepting patch. TIA.

Comment 12

[Jan Horak [:jhorak]]

Ouch. I forgot to mention that I set LDAP *mConnectionHandle as a public member just for testing. This have to be solved differently. I'm aware of it.

Comment 13

[Jan Horak [:jhorak]]

I've hit an synchronization problem:
In AbortPendingOperations, which is an enumeration function over opened connections called during shutdown, I would like to use nsILDAPOperation::AbandonExt() to cancel LDAP operation. The problem is that AbandonExt try to remove from hashtable which I'm currently enumerating by AbortPendingOperations. It is waiting until lock is freed, which is after finishing enumerating so this seems to be deadlock.

One solution which comes to my mind is to change:
AbandonExt(void) to:
AbandonExt(PRBool remove_from_hashtable = PR_TRUE)
when remove_from_hashtable is called by PR_FALSE it wouldn't try to remove entry from hash table and it will be removed by AbortPendingOperation by returning PL_DHASH_REMOVE. I try to add a patch with my proposed solution.

Comment 14

[Mark Banner (:standard8)]

The patch in bug 609585 may help you here.

If you stick the patch up next week and request feedback from me I'll try and remember to take a look at it.

Comment 15

[Jan Horak [:jhorak]]

So making copy of the hashtable is more preferred?

Comment 16

[Jan Horak [:jhorak]]

Um, mPendingOperations is nsInterfaceHashtableMT which does not have Clone() method (unlike nsHashtable). I'd rather keep it that way because nsInterfaceHashtable can be used in frozen linkage. 
We will have to create the copy manually or change AbandonExt function (which is not generic solution for this kind of problem). Could you please help me out?

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 490834 [details] [diff] [review]
WIP patch including test case.

see previous 2 comments

Comment 18

[Jan Horak [:jhorak]]

My patch is based on: https://bugzilla.mozilla.org/attachment.cgi?id=468590&action=diff
and there is the change from nsSupportsHashtable to nsInterfaceHashtableMT. I still have problem with AbandonExt and solution proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=609585 as long as nsInterfaceHashtableMT does not have clone method is unfortunately unusable. I would have to clone it manually, is this a proposed solution?

Comment 19

[Jan Horak [:jhorak]]

Attachment: proposed patch 1

- sync to comm-central
- pointers to pending operations copied to array to avoid lockup
Please have a look and let me know.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 501686 [details] [diff] [review]
proposed patch 1

I've reviewed this using the review board which you can find here:

http://reviews.visophyte.org/r/501686/

Here's the comments for bugzilla records:

Sorry for the delay in reviewing this, I had issues getting a LDAP server
connected to my Thunderbird. The patch looks really good, and I've been
testing it and with a couple of tweaks it works really well and solves this
bug and bug 544939 both of which will be really useful.

As I wrote some of the original patch, I'd just like David to take a quick
look as well to verify that the changes look reasonable.

Thanks for your work on getting this completed.


on file: ldap/xpcom/src/nsLDAPConnection.h line 108
>   nsresult AddPendingOperation(PRInt32 aOperationID, nsILDAPOperation *aOperation);

I think its would probably better to use PRUint32 here. Especially as the
mPendingOperation expects a nsUint32HashKey. This also means you can remove
the cast within in the function.


on file: ldap/xpcom/src/nsLDAPConnection.h line 119
>     nsresult RemovePendingOperation(PRInt32 aOperationID);

Ditto with using PRUint32.


on file: ldap/xpcom/src/nsLDAPConnection.cpp line 82
>   obsServ->AddObserver(this, "xpcom-shutdown", PR_FALSE);

Ok, contrary to what I originally though, "xpcom-shutdown" isn't quite early
enough, this actually needs to be "profile-change-net-teardown".

This combined with one other change will fix bug 544939 for us.


on file: ldap/xpcom/src/nsLDAPConnection.cpp line 105
> NS_IMPL_CI_INTERFACE_GETTER3(nsLDAPConnection, nsILDAPConnection,

You need to add nsIObserver in here (and increment the number), and add
nsIObserver as an NS_INTERFACE_MAP_ENTRY.

Its probably not strictly necessarily to do this, but just in case someone
does try to QI to an nsIObserver, it'd probably be good to have.


on file: ldap/xpcom/src/nsLDAPConnection.cpp line 259
>   if (!nsCRT::strcmp(aTopic, "xpcom-shutdown")) {

Please use "profile-change-net-teardown" as mentioned earlier.


on file: ldap/xpcom/src/nsLDAPConnection.cpp line 272
>     }

After checking that we've abandoned the pending operations, please can you add
a call to Close(). This will then mean we'll close the connection early enough
at shutdown and properly fix bug 544939 in the process.

Comment 21

[[e-mail]]

(In reply to comment #20)
> in case someone does try to QI to an nsIObserver, it'd probably be good to have.
nsCOMPtr<nsIObserver> automatically warns about this in debug builds.

Comment 22

[[e-mail]]

Comment on attachment 501686 [details] [diff] [review]
proposed patch 1

>+  nsIRunnable* runnable = new nsLDAPConnectionRunnable(aOperationID, aOperation,
>+                                                       this);
>+  nsresult rv;
>+  if (!mThread)
>+  {
>+    rv = NS_NewThread(getter_AddRefs(mThread), runnable);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+  else
>+  {
>+    rv = mThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
> 
>+  mPendingOperations.Put((PRUint32)aOperationID, aOperation);
[Is this a potential race condition?]

>+  if (listener)
>+      listener->OnLDAPMessage(aMsg);
Nit: I guess this should be 2 space indent now.

>+  nsIThread* thread = NS_GetCurrentThread();
...
>+              nsIThread* thread = NS_GetCurrentThread();
...
>+        nsIThread* thread = NS_GetCurrentThread();
I don't think this compiles under the external API! Also, aren't these are all in the same method, so only the first is necessary?

>      */
>-    nsresult AddPendingOperation(nsILDAPOperation *aOperation);
>+  nsresult AddPendingOperation(PRInt32 aOperationID, nsILDAPOperation *aOperation);
Nit: probably wrong to change the indentation here.

> NS_IMETHODIMP
> nsLDAPOperation::Init(nsILDAPConnection *aConnection,
>@@ -141,7 +141,7 @@
> 
>     // cache the connection handle
>     //
>-    mConnectionHandle = 
>+    mConnectionHandle =
>         static_cast<nsLDAPConnection *>(aConnection)->mConnectionHandle;
On an unrelated note, I see that nsLDAPOperation casts its nsILDAPConnection to nsLDAPConnection all over (well, all except three places). Does anyone think it might be an idea to change mConnection to an nsRefPtr<nsLDAPConnection>?

>-  
>+
[Unrelated whitespace changes make parts of this patch harder to read.]

Comment 23

[Mark Banner (:standard8)]

(In reply to comment #22)
> Comment on attachment 501686 [details] [diff] [review]
> proposed patch 1
> 
> >+  nsIRunnable* runnable = new nsLDAPConnectionRunnable(aOperationID, aOperation,
> >+                                                       this);
> >+  nsresult rv;
> >+  if (!mThread)
> >+  {
> >+    rv = NS_NewThread(getter_AddRefs(mThread), runnable);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+  }
> >+  else
> >+  {
> >+    rv = mThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+  }
> > 
> >+  mPendingOperations.Put((PRUint32)aOperationID, aOperation);
> [Is this a potential race condition?]

If you mean that we should do the Put before the create thread/dispatch, that's probably a good idea.

> >+  nsIThread* thread = NS_GetCurrentThread();
> ...
> >+              nsIThread* thread = NS_GetCurrentThread();
> ...
> >+        nsIThread* thread = NS_GetCurrentThread();
> I don't think this compiles under the external API! Also, aren't these are all
> in the same method, so only the first is necessary?

Just a note that I think I said to jhorak that we should probably not care about external API yet, but get this patch in without too much extra complexity (there's other bits which I believe won't compile yet), and then we can get the external API sorted later, hopefully in an easier way ;-)

> >-    mConnectionHandle = 
> >+    mConnectionHandle =
> >         static_cast<nsLDAPConnection *>(aConnection)->mConnectionHandle;
> On an unrelated note, I see that nsLDAPOperation casts its nsILDAPConnection to
> nsLDAPConnection all over (well, all except three places). Does anyone think it
> might be an idea to change mConnection to an nsRefPtr<nsLDAPConnection>?

That would be reasonable.

> >-  
> >+
> [Unrelated whitespace changes make parts of this patch harder to read.]

Using review board its much easier to filter these out of the reading ;-)

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 501686 [details] [diff] [review]
proposed patch 1

And thinking about it, I'll switch the additional review to Neil as he's pretty much just looked through it anyway ;-)

Comment 25

[[e-mail]]

(In reply to comment #23)
> (In reply to comment #22)
> > I don't think this compiles under the external API!
> Just a note that I think I said to jhorak that we should probably not care
> about external API yet
Well, this was the only line that I spotted. But now you've requested my review, I might as well try this on my experimental external API build too :-P

Comment 26

[[e-mail]]

(In reply to comment #25)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > I don't think this compiles under the external API!
> > Just a note that I think I said to jhorak that we should probably not care
> > about external API yet
> Well, this was the only line that I spotted. But now you've requested my
> review, I might as well try this on my experimental external API build too :-P
Oh, LDAP isn't counted as part of the experimental external API build...

Comment 27

[Jan Horak [:jhorak]]

Attachment: proposed patch 2

> On an unrelated note, I see that nsLDAPOperation casts its nsILDAPConnection to
> nsLDAPConnection all over (well, all except three places). Does anyone think it
> might be an idea to change mConnection to an nsRefPtr<nsLDAPConnection>?
I'm not sure how to interpret this, could you show me some example of how to remove static_cast<> or solve this issue?

Comment 28

[Josh Matthews [:jdm]]

A nsRefPtr is a refcounted concrete implementation class, instead of having nsCOMPtr<nsILDAPConnection> mConnection and continuously casting mConnection.get() to its concrete type, you could just use nsRefPtr<nsLDAPConnection> mConnection.

Comment 29

[[e-mail]]

(In reply to comment #27)
> > On an unrelated note, I see that nsLDAPOperation casts its nsILDAPConnection to
> > nsLDAPConnection all over (well, all except three places). Does anyone think it
> > might be an idea to change mConnection to an nsRefPtr<nsLDAPConnection>?
> I'm not sure how to interpret this, could you show me some example of how to
> remove static_cast<> or solve this issue?
It was an unrelated note, so I hope your change of nsLDAPConnectionRunnable's mConnection to an nsRefPtr<nsLDAPConnection> was unrelated too ;-)

Comment 30

[[e-mail]]

Comment on attachment 509405 [details] [diff] [review]
proposed patch 2

>@@ -136,17 +136,17 @@ nsLDAPOperation::Init(nsILDAPConnection *aConnection,
>     // set the member vars
>     //
>     mConnection = aConnection;
>     mMessageListener = aMessageListener;
>     mClosure = aClosure;
> 
>     // cache the connection handle
>     //
>-    mConnectionHandle = 
>+    mConnectionHandle =
>         static_cast<nsLDAPConnection *>(aConnection)->mConnectionHandle;
Here's an example, by the way. Because aConnection is an nsILDAPConnection rather than an nsLDAPConnection the cast has been added to access the mConnectionHandle. (I don't know if we can change the way the nsLDAPOperation is constructed to avoid passing the interface type instead of the concrete type.) mConnection is currently an nsCOMPtr<nsILDAPConnection> to match aConnection, but a number of times throughout the file we cast it to nsLDAPConnection* (which is even uglier, because .get() used to return an nsDerivedSafe<nsILDAPConnection>). If we change mConnection to an nsRefPtr<nsLDAPConnection> then we eliminate all the casts on the way out. (We still have to cast aConnection in the initial assignment of course.)

Comment 31

[[e-mail]]

Comment on attachment 509405 [details] [diff] [review]
proposed patch 2

There's one thing that worries me after reading this patch. The old code used to work by polling mPendingOperations and running CheckLDAPOperationResult. This meant that simply removing the operation from mPendingOperations was enough to effectively cancel the operation (assuming the thread wasn't checking it at the time). This doesn't seem to happen in this code. Everything seems to work when I try an address book search, so am I overlooking something?

> NS_INTERFACE_MAP_BEGIN(nsLDAPConnection)
>   NS_INTERFACE_MAP_ENTRY(nsILDAPConnection)
>   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>   NS_INTERFACE_MAP_ENTRY(nsIDNSListener)
>+NS_INTERFACE_MAP_ENTRY(nsIObserver)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsILDAPConnection)
>   NS_IMPL_QUERY_CLASSINFO(nsLDAPConnection)
> NS_INTERFACE_MAP_END_THREADSAFE
Nit: wrong indentation on the new map entry.
In fact I think this can be written
NS_IMPL_QUERY_INTERFACE4_CI(nsLDAPConnection, nsILDAPConnection,
                            nsISupportsWeakReference, nsIDNSListener, nsIObserver)
Sadly there's no NS_IMPL_THREADSAFE_QUERY_INTERFACE4_CI macro ;-)

>-  // allocate a hashtable to keep track of pending operations.
>-  // 10 buckets seems like a reasonable size, and we do want it to 
>-  // be threadsafe
>-  mPendingOperations = new nsSupportsHashtable(10, PR_TRUE);
>-  if (!mPendingOperations) {
>-    NS_ERROR("failure initializing mPendingOperations hashtable");
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>+  // Initialise the hashtable to keep track of pending operations.
>+  // 10 buckets seems like a reasonable size.
>+  mPendingOperations.Init(10);
Does this use "infallible" malloc or do we need to keep the OOM check?

>+    return NS_ERROR_UNEXPECTED;    
Nit: trailing whitespace (this is the last of 5 occurrences).

>+  nsCOMPtr<nsILDAPMessage> msg;
Could make this an nsRefPtr<nsLDAPMessage> to avoid having to have rawMsg.

>+  nsIThread* thread = NS_GetCurrentThread();
I notice that existing ldap/xpcom/src code also uses
nsCOMPtr<nsIThread> thread = do_GetCurrentThread();

Comment 32

[Jan Horak [:jhorak]]

Attachment: proposed patch 3

Ah, I see. Without static_casts code looks much nicer. Please have a look.

Comment 33

[[e-mail]]

Comment on attachment 509721 [details] [diff] [review]
proposed patch 3

Very nice, except somehow the trailing whitespace count went up...

>+  nsCOMPtr<nsIObserverService> obsServ = 
>+      mozilla::services::GetObserverService();
>+  obsServ->AddObserver(this, "profile-change-net-teardown", PR_FALSE);
>+  
(first and last of these lines)

>+NS_IMPL_CI_INTERFACE_GETTER4(nsLDAPConnection, nsILDAPConnection,                             

>+  nsTArray<nsILDAPOperation*>* pending_operations = static_cast<nsTArray<nsILDAPOperation*>* >(userArg);  

>+      /* We cannot use enumerate function to abort operations because 

>+  mPendingOperations.Put((PRUint32)aOperationID, aOperation);
>+  
(latter line)

Comment 34

[Jan Horak [:jhorak]]

Attachment: Final patch 4

Can I set checkin-needed or does this need superreview?

Comment 35

[Ludovic Hirlimann [:Usul]]

Comment on attachment 510285 [details] [diff] [review]
Final patch 4

Carrying Neil's r+ over. There seems to be no API change, so no need for sr.

Comment 36

[[e-mail]]

jhorak's attached quite enough patches as it is but whomever checks in please check as I think there are a few trailing whitespaces left still. Thanks.

Comment 37

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/ee3e25817685

Note: I fixed Neil's whitespaces issues, plus I also moved the added Close() call slightly so that it took account of the pending operations = 0 case (we still need to close in that case because we keep the connection open), and I removed a redundant nsresult rv; that we hadn't spotted.

Comment 38

[Wayne Mery (:wsmwk)]

is this unlikely to be accepted to land on branch?  (being such a large patch)

Comment 39

[Wayne Mery (:wsmwk)]

(In reply to comment #38)
> is this unlikely to be accepted to land on branch?  (being such a large patch)

to be clear, I'm thinking of this in terms of the hang bug 524315 that this fixes

Comment 40

[Mark Banner (:standard8)]

(In reply to comment #38)
> is this unlikely to be accepted to land on branch?  (being such a large patch)

Yes, this is way too much for a branch release.