1390447 - Check for WSA_IO_INCOMPLETE error after WSAGetOverlappedResult

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Dragana Damjanovic [:dragana]]

Windows have such a nice documentation.

I found out that WSAGetOverlappedResult will return WSA_IO_INCOMPLETE instead of WSA_IO_PENDING(==ERROR_IO_PENDING).

I think this caused some of this crashes. Thay all have 3e3 in crash address which is error WSA_OPERATION_ABORTED which will happen when we call cancelio

Comment 1

[Dragana Damjanovic [:dragana]]

maybe also responsible for:
mozilla::net::nsSocketTransport::InitiateSocket
mozilla::net::nsIOService::URIChainHasFlags 
RtlEnterCriticalSection | mozilla::net::CacheStorageService::CacheQueueSize  

each has one crash on address 0.

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: bug_1390447.patch

Comment 3

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28def8567a3225347cfa35aef22cd669c51294dc

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8897343 [details] [diff] [review]
bug_1390447.patch

Review of attachment 8897343 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm.... the docs at https://msdn.microsoft.com/cs-cz/library/windows/desktop/ms683209(v=vs.85).aspx are confusing again.  bWait argument description says it may return ERROR_IO_INCOMPLETE but Remarks say it may return ERROR_IO_PENDING.

According https://msdn.microsoft.com/en-us/library/windows/desktop/ms681388(v=vs.85).aspx :

ERROR_IO_INCOMPLETE

    996 (0x3E4)

    Overlapped I/O event is not in a signaled state.

ERROR_IO_PENDING

    997 (0x3E5)

    Overlapped I/O operation is in progress.


Locally looking at how the OL is filled by connectex there apparently is no event assigned (win10, official (non-insider), latest, x64), hence I would not expect IO_INCOMPLETE be returned from GetOverlappedResult ...

I don't know if connectex could ever assign an event on the OL, if that is Win version dependent or even LSA dependent.  If yes, does it mean for us to somehow specifically handle it?  poll the event instead of the socket?  or just handle both these error codes the same way as we already do now?


BTW, looks like there are two possible error codes when connectex returns false:

"Connection-oriented sockets are often unable to complete their connection immediately, and therefore the operation is initiated and the function immediately returns with the ERROR_IO_PENDING or WSA_IO_PENDING error"

Should we check both at https://dxr.mozilla.org/mozilla-central/rev/b95b1638db48fc3d450b95b98da6bcd2f9326d2f/nsprpub/pr/src/md/windows/w95sock.c#458 ?


One more nit: you have a lot of else after return in your code, followup would be good to file.

Comment 5

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Comment on attachment 8897343 [details] [diff] [review]
> bug_1390447.patch
> 
> Review of attachment 8897343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm.... the docs at
> https://msdn.microsoft.com/cs-cz/library/windows/desktop/ms683209(v=vs.85).
> aspx are confusing again.  bWait argument description says it may return
> ERROR_IO_INCOMPLETE but Remarks say it may return ERROR_IO_PENDING.
> 

Remarks are talking about "the function that started the operation". In this case ConnectEx

> According
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms681388(v=vs.85).
> aspx :
> 
> ERROR_IO_INCOMPLETE
> 
>     996 (0x3E4)
> 
>     Overlapped I/O event is not in a signaled state.
> 
> ERROR_IO_PENDING
> 
>     997 (0x3E5)
> 
>     Overlapped I/O operation is in progress.
> 

As I understand this:  ERROR_IO_INCOMPLETE is for GetOverlappedResult and ERROR_IO_PENDING is for ConnectEx.

> 
> Locally looking at how the OL is filled by connectex there apparently is no
> event assigned (win10, official (non-insider), latest, x64), hence I would
> not expect IO_INCOMPLETE be returned from GetOverlappedResult ...

You can use OL without event. If you want an event you need to add that to OVERLAPPED before calling, for example, ConnectEx.
If you use OL without events (I wanted to avoid using a separate thread because we had problems with that) you will need to check OL your self until it is finished, i.e. returns something else than ERROR_IO_INCOMPLETE.

> 
> I don't know if connectex could ever assign an event on the OL, if that is
> Win version dependent or even LSA dependent.  If yes, does it mean for us to
> somehow specifically handle it?  poll the event instead of the socket?  or
> just handle both these error codes the same way as we already do now?
> 
> 
> BTW, looks like there are two possible error codes when connectex returns
> false:
> 
> "Connection-oriented sockets are often unable to complete their connection
> immediately, and therefore the operation is initiated and the function
> immediately returns with the ERROR_IO_PENDING or WSA_IO_PENDING error"
> 
> Should we check both at
> https://dxr.mozilla.org/mozilla-central/rev/
> b95b1638db48fc3d450b95b98da6bcd2f9326d2f/nsprpub/pr/src/md/windows/w95sock.
> c#458 ?
> 

ERROR_IO_PENDING == 997 == WSA_IO_PENDING == 997

> 
> One more nit: you have a lot of else after return in your code, followup
> would be good to file.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8897343 [details] [diff] [review]
bug_1390447.patch

Thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1619307e154a
Check for WSA_IO_INCOMPLETE error after GetOverlappedResult. r=mayhemer

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1619307e154a