Closed Bug 1259089 Opened 4 years ago Closed 4 years ago

Set a socket nonblocking in sts, just to be sure

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 + fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 4 obsolete files)

Set a socket nonblocking in sts, just to be sure.
Attached patch bug_nonblocking.patch (obsolete) — Splinter Review
Attached patch bug_nonblocking.patch (obsolete) — Splinter Review
Attachment #8733944 - Attachment is obsolete: true
Attachment #8733954 - Flags: review?(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
Attachment #8733954 - Flags: review?(mcmanus) → review+
Whiteboard: [necko-active]
Attached patch bug_nonblocking.patch (obsolete) — Splinter Review
Attachment #8733954 - Attachment is obsolete: true
Attachment #8734259 - Flags: review+
Keywords: checkin-needed
Blocks: 1158189
I will nominate this for uplift to beta as soon as it lands on m-c
Tracking since this may affect crashes in 46
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
Flags: needinfo?(dd.mozilla)
Keywords: checkin-needed
rebased
Attachment #8734259 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8734421 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1fc001760e73
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Dragana--it seems to have stuck.  Request uplift?
Flags: needinfo?(dd.mozilla)
Attached patch bug_nonblocking.patch (obsolete) — Splinter Review
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
Flags: needinfo?(dd.mozilla)
Attachment #8738065 - Flags: review+
Attachment #8738065 - Flags: approval-mozilla-beta?
Attachment #8738065 - Flags: approval-mozilla-aurora?
Comment on attachment 8738065 [details] [diff] [review]
bug_nonblocking.patch

This may help with our #3 beta topcrash. 
Please uplift to beta and aurora.
Attachment #8738065 - Flags: approval-mozilla-beta?
Attachment #8738065 - Flags: approval-mozilla-beta+
Attachment #8738065 - Flags: approval-mozilla-aurora?
Attachment #8738065 - Flags: approval-mozilla-aurora+
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.
Flags: qe-verify+
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
Flags: needinfo?(lhenry)
Flags: needinfo?(dd.mozilla)
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]:
Flags: needinfo?(dd.mozilla)
Attachment #8738940 - Flags: review+
Attachment #8738940 - Flags: approval-mozilla-beta?
Attachment #8738940 - Flags: approval-mozilla-aurora?
Attachment #8738065 - Attachment is obsolete: true
(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!
Flags: needinfo?(lhenry)
Comment on attachment 8738940 [details] [diff] [review]
bug_nonblocking_aurora_beta.patch

May help with shutdown crashes, please uplift to aurora and beta.
Attachment #8738940 - Flags: approval-mozilla-beta?
Attachment #8738940 - Flags: approval-mozilla-beta+
Attachment #8738940 - Flags: approval-mozilla-aurora?
Attachment #8738940 - Flags: approval-mozilla-aurora+
(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
You need to log in before you can comment on or make changes to this bug.