364741 - nsISocketTransport.isAlive() returns false immediately after successful connect

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[:Gijs (he/him)]

As in summary and URL. Because the event is sent before the socket state is changed, and the networking code runs on a different thread, by the time code started from the event breaks in some cases because it checks isAlive() and then fails.

In ChatZilla's case, this is biting some patches of us because on some older mozilla versions, the socket doesn't throw an error if you go into offline mode, so we tried to check isAlive, which broke the connection sequence completely for the reason stated.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Note that there's an assertion when running the testcase:
###!!! ASSERTION: nsExceptionManager not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /home/chb/mozilla/xpcom/base/nsExceptionService.cpp, line 97

I'd like to fix that before checking the patch in, even though it doesn't actually cause the test to fail.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 249541 [details] [diff] [review]
patch

something seems to be wrong with this testcase...

Comment 4

[:Gijs (he/him)]

Biesi: what exactly is wrong with the testcase? :-)

Comment 5

[:Gijs (he/him)]

Alright, biesi, are you any further with this? :-)

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 249541 [details] [diff] [review]
patch

I think we should get this in without tests for now, there are thread-related issues with the testcase

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 249541 [details] [diff] [review]
patch

r+sr=me, but please make sure to get the test fixed and in...

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 249541 [details] [diff] [review]
patch

I'd like to land the code fix now, because it's a pretty obvious fix. I'll land the test as soon as I can get it fixed.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 249541 [details] [diff] [review]
patch

a=beltzner, but if the test doesn't land, I'm gonna sic shaver and sayrer on you ;)

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: just the code change

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in nsSocketTransport2.cpp;
/cvsroot/mozilla/netwerk/base/src/nsSocketTransport2.cpp,v  <--  nsSocketTransport2.cpp
new revision: 1.55; previous revision: 1.54
done

leaving open to get a unit test checked in

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

so the unit test problem is:
###!!! ASSERTION: nsExceptionManager not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /home/chb/mozilla/xpcom/base/nsExceptionService.cpp, line 97

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

also PrincipalHolder

and then, nsSimpleURI (held by the null principal which gets cycle collected on the main thread during shutdown. did the null principal create that URI on a background thread??)

Comment 14

[Mike Schroepfer]

Comment on attachment 249541 [details] [diff] [review]
patch

Clearing approval flag since this bug is still open for unit tests.  This patch has landed.

Comment 15

[Mike Schroepfer]

Comment on attachment 249541 [details] [diff] [review]
patch

adding back an approval.