711786 - nsISocketTransport::IsAlive is documented as being thread safe, but nsSocketTransport2::IsAlive() is not thread-safe

Status: NEW

(Core :: Networking, defect, P5)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The IDL for nsISocketTransport has this comment:

"* NOTE: This is a free-threaded interface, meaning that the methods on
 * this interface may be called from any thread."

However, the IsAlive() methods calls PR_Recv, and calling PR_Recv (at least on an SSL socket) is NOT thread-safe. (See bug 427948, at least.)

I think we should enforce that IsAlive is only called on the socket transport thread, and change the documentation of nsISocketTransport.

Comment 1

[Honza Bambas (:mayhemer)]

For a quick fix of bug 711786 I think we could just search for the TCP NSPR layer of the socket to call PR_Recv on it.  Maybe do that only off the main thread.

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> off the main thread.

Should be: off the socket thread, of course.

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> For a quick fix of bug 711786 I think we could just search for the TCP NSPR
> layer of the socket to call PR_Recv on it.  Maybe do that only off the main
> thread.

I'll make an attempt at this right now. no guarantees :)

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #3)

> 
> I'll make an attempt at this right now. no guarantees :)

honza says in IM he's got it.

Comment 5

[Florian Quèze [:florian]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> (In reply to Patrick McManus [:mcmanus] from comment #3)
> 
> > 
> > I'll make an attempt at this right now. no guarantees :)
> 
> honza says in IM he's got it.

I assume this comment was related to https://bug711787.bugzilla.mozilla.org/attachment.cgi?id=582828

Any reason to not request review on this?

FYI, we are running into this bug for the IRC code that's used for both Instantbird and Thunderbird (It's been reported in https://bug711787.bugzilla.mozilla.org/attachment.cgi?id=582828).

Comment 6

[Florian Quèze [:florian]]

(In reply to Florian Quèze from comment #5)
> (It's been reported in
> https://bug711787.bugzilla.mozilla.org/attachment.cgi?id=582828).

I meant https://bugzilla.instantbird.org/show_bug.cgi?id=1455

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 8

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 9

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, untouched DOM/Storage bug's priority.

If you have reason to believe, this is wrong, please write a comment and ni :jstutte.