1259089 - Set a socket nonblocking in sts, just to be sure

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dragana Damjanovic [:dragana]]

Set a socket nonblocking in sts, just to be sure.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_nonblocking.patch

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: bug_nonblocking.patch

Comment 3

[Dragana Damjanovic [:dragana]]

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

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8733954 [details] [diff] [review]
bug_nonblocking.patch

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

::: netwerk/base/nsSocketTransport2.cpp
@@ +1419,5 @@
>  
>      NetAddrToPRNetAddr(&mNetAddr, &prAddr);
>  
> +#ifdef XP_WIN
> +    // Find the real tcp socket and set non-blocking once again!

go ahead and reference the shutdown hang bug number

@@ +1422,5 @@
> +#ifdef XP_WIN
> +    // Find the real tcp socket and set non-blocking once again!
> +    PRFileDesc *bottom = PR_GetIdentitiesLayer(fd, PR_NSPR_IO_LAYER);
> +    if (bottom) {
> +      PROsfd osfd = PR_FileDesc2NativeHandle(bottom); 

whitespace

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_nonblocking.patch

Comment 6

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30730011125f

Comment 7

[Dragana Damjanovic [:dragana]]

I will nominate this for uplift to beta as soon as it lands on m-c

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this may affect crashes in 46

Comment 9

[Carsten Book [:Tomcat]]

has conflicts

renamed 1259089 -> bug_nonblocking.patch
applying bug_nonblocking.patch
patching file netwerk/base/nsSocketTransport2.cpp
Hunk #1 FAILED at 1413
1 out of 1 hunks FAILED -- saving rejects to file netwerk/base/nsSocketTransport2.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug_nonblocking.patch

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: bug_nonblocking.patch

rebased

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc001760e73

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/1fc001760e73

Comment 13

[Jason Duell]

Dragana--it seems to have stuck.  Request uplift?

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: bug_nonblocking.patch

rebased


Approval Request Comment
[Feature/regressing bug #]: Set socket to non-blocking once again
[User impact if declined]: This maybe will help with shutdown crashes
[Describe test coverage new/current, TreeHerder]:It is in Nightly for a week and this code path is used for establishing each connection.
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8738065 [details] [diff] [review]
bug_nonblocking.patch

This may help with our #3 beta topcrash. 
Please uplift to beta and aurora.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

After beta 9 releases (this Friday) let's check back in bug 1158189 to see if the crash rate goes down for network related shutdown issues. Hopefully we'll be able to tell.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/d3d6344cc744

Comment 18

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

https://hg.mozilla.org/releases/mozilla-beta/rev/315ab4282331

Comment 19

[Carsten Book [:Tomcat]]

backed out from beta and aurora for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=980326&repo=mozilla-beta

lizzard: fyi, since this was meant to fix a topcrash

Comment 20

[Dragana Damjanovic [:dragana]]

Attachment: bug_nonblocking_aurora_beta.patch

So sorry, I over seen something.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 21

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3785e74d9a5b

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Dragana Damjanovic [:dragana] from comment #20)
> Created attachment 8738940 [details] [diff] [review]
> bug_nonblocking_aurora_beta.patch
> 
> So sorry, I over seen something.
> 
> Approval Request Comment
> [Feature/regressing bug #]:
> [User impact if declined]:
> [Describe test coverage new/current, TreeHerder]:
> [Risks and why]: 
> [String/UUID change made/needed]:

this had approval before so relanding:
https://hg.mozilla.org/releases/mozilla-aurora/rev/c774432aac6d
https://hg.mozilla.org/releases/mozilla-beta/rev/5a7dbc94f9eb

thanks Dragana for the updated patch!

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8738940 [details] [diff] [review]
bug_nonblocking_aurora_beta.patch

May help with shutdown crashes, please uplift to aurora and beta.

Comment 24

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> After beta 9 releases (this Friday) let's check back in bug 1158189 to see
> if the crash rate goes down for network related shutdown issues. Hopefully
> we'll be able to tell.

Things are overall looking better as far as I can tell, the only remaining point of concern
is the following crash signature:
* http://tinyurl.com/zkcv9nc
  - status: 2058 crashes with 46.0b9, 1311 crashes with 46.0b10, 779 crashes 
    with 46.0b11, all in the last 28 days

Here's an overview for the rest of the signatures associated to this bug, per Comment 16:
* http://tinyurl.com/j8xagfp
  - status: 0 crashes with this signature in the last 28 days
* http://tinyurl.com/hg8thnf
  - status: 0 crashes with this signature in the last 28 days
* http://tinyurl.com/j8bc7fl
  - status: 0 crashes with this signature in the last 28 days
* http://tinyurl.com/jjwwo4r
  - status: 0 crashes on Fx46 in the last 28 days
* http://tinyurl.com/gmplkd8
  - status: 0 crashes on Fx46 in the last 28 days
* http://tinyurl.com/hkeky7e
  - status: 0 crashes on Fx46 in the last 28 days
* http://tinyurl.com/j7ehf9c
  - status: 0 crashes with Fx46 in the last 28 days
* http://preview.tinyurl.com/j3nvl2q
  - status: 4634 crashes with 46.0b5, 3 crashes with 46.0b4, all in the last 28 days