946159 - nsSocketTransport::IsAlive does I/O on the main thread

Status: NEW

(Core :: Networking, defect, P5)

Description

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

nsSocketTransport::IsAlive can be called on the main thread. nsSocketTransport::IsAlive calls PR_Recv. PR_Recv on an SSL socket will attempt to drive the SSL handshake forward. If this happens while a Certificate handshake message is pending, then the libssl I/O layer will parse the Certificate message during the PR_Recv call. This may access the NSS certificate database on disk. Thus, main thread I/O.

Comment 1

[Patrick McManus [:mcmanus]]

now that you raise the issue, it seems also bad to do blocking disk i/o on the socket thread in the more general non-isalive case too. hadn't realized that was going on.

Comment 2

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

(In reply to Patrick McManus [:mcmanus] from comment #1)
> now that you raise the issue, it seems also bad to do blocking disk i/o on
> the socket thread in the more general non-isalive case too. hadn't realized
> that was going on.

I agree. I think that we may be able to remove the bad I/O generated from IsAlive, but within libssl there are some access to the certificate database that can cause disk reads. New libssl APIs are needed to fix that. I filed bug 951781 in NSS for this. However, I think we should find a workaround in Gecko if possible, because I suspect that this can cause severe jank due to the possibility of blocking the main thread on OCSP requests as well as disk I/O.

Comment 3

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

My understanding is that the purpose of this IsAlive() call is to determine if the socket has been closed (e.g. connection reset). So, I think one alternative solution is for IsAlive to be changed so that it navigates through the I/O layer stack until it gets to a socket I/O layer, and then doing the recv(PR_MSG_PEEK) directly on that I/O layer. Not only would that resolve this bug, but I think it would also be more correct for the intended purpose.

Comment 4

[Firefox Bug Husbandry Bot]

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

Comment 5

[Firefox Bug Husbandry Bot]

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

Comment 6

[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.