Check for WSA_IO_INCOMPLETE error after WSAGetOverlappedResult

RESOLVED FIXED in Firefox 57

Status

()

Core
Networking: HTTP
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

57 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [necko-active], crash signature)

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
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
(Assignee)

Comment 1

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

each has one crash on address 0.
(Assignee)

Updated

10 months ago
Version: 56 Branch → 57 Branch
(Assignee)

Comment 2

10 months ago
Created attachment 8897343 [details] [diff] [review]
bug_1390447.patch
Attachment #8897343 - Flags: review?(honzab.moz)
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.
(Assignee)

Comment 5

10 months ago
(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 on attachment 8897343 [details] [diff] [review]
bug_1390447.patch

Thanks!
Attachment #8897343 - Flags: review?(honzab.moz) → review+

Comment 7

10 months ago
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1619307e154a
Check for WSA_IO_INCOMPLETE error after GetOverlappedResult. r=mayhemer

Comment 8

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1619307e154a
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.