Closed
Bug 1359938
Opened 7 years ago
Closed 7 years ago
Socket status events and tcp fast open
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: dragana, Assigned: dragana)
References
Details
(Whiteboard: [necko-active])
Attachments
(1 file, 2 obsolete files)
14.78 KB,
patch
|
mcmanus
:
review+
valentin
:
review+
|
Details | Diff | Splinter Review |
During TFO we buffer data in TCPFastOpenLayer before we call sendto. This is needed because we can only call sendto once and we need to have all data that we want to send in one buffer. This breaks timing of socket status events. When we call PR_Write during TFO we do not send data so we should not send NS_NET_STATUS_SENDING_TO event either. The data will be send when we call TCPFastOpenFinish or after the socket is connected. When socket is connected we need to send the rest of buffered data before we call OnSocketReady, because OnSocketReady can trigger NS_NET_STATUS_WAITING_FOR event. We need to move call of nsHttpTransaction::SetRequestStart from nsHttpTransaction::ReadRequestSegment because this can be called during TFO as well.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8862075 -
Flags: review?(valentin.gosu)
Attachment #8862075 -
Flags: review?(mcmanus)
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c93401ac878dd1faece1748375e8d8698321477f
Updated•7 years ago
|
Attachment #8862075 -
Flags: review?(mcmanus)
Comment 3•7 years ago
|
||
thanks for including the try report unfortunately its lit up all orange with Assertion failure: false, at /home/worker/workspace/build/src/security/manager/ssl/nsNSSIOLayer.cpp:1181 so I'll drop the review for now..
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8862075 -
Attachment is obsolete: true
Attachment #8862075 -
Flags: review?(valentin.gosu)
Attachment #8862193 -
Flags: review?(valentin.gosu)
Attachment #8862193 -
Flags: review?(mcmanus)
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21105e0581d4e7c5f0775972e85446414fa7a3cc
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8862193 [details] [diff] [review] bug_tfo_and_socket_status_event.patch I am dropping review flags. The problem is not 1359847. I do not know what it is. I am investigating. (I cannot reproduce it locally so it is fun to debug.)
Attachment #8862193 -
Flags: review?(valentin.gosu)
Attachment #8862193 -
Flags: review?(mcmanus)
Assignee | ||
Comment 7•7 years ago
|
||
The problem is bug 1340854. If we do PR_Write with 0 bytes to be written assertions can be hit. I will check if we can call PR_Write(nullptr,0,...) or we are not allowed anymore. I assume that is a regression from bug 1340854. If nss wants it that way I will change my patch.
Assignee | ||
Comment 8•7 years ago
|
||
So this try failure are a regression from bug 1340854. This is not a really important regression, just a minor thing. But from ekr's comment, we can not call PR_Write because this will not always call send or recv on a layer under NSS. And we can end up in a deadlock :) - NSS think it has sent data and does not want to call send/recv, but that data sits in a buffer in the layer below. I will update the patch to call TCPFAstOpenLayer:Send directly not utilizing PR_Write.
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29c6e4d15948b8a2269b32898681c22583018f03
Attachment #8862193 -
Attachment is obsolete: true
Attachment #8862619 -
Flags: review?(valentin.gosu)
Attachment #8862619 -
Flags: review?(mcmanus)
Comment 10•7 years ago
|
||
Comment on attachment 8862619 [details] [diff] [review] bug_tfo_and_socket_status_event.patch Review of attachment 8862619 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/TCPFastOpenLayer.cpp @@ +64,5 @@ > CONNECTED, > WAITING_FOR_CONNECTCONTINUE, > COLLECT_DATA_FOR_FIRST_PACKET, > + WAITING_FOR_CONNECT, > + SOCKET_ERROR_STATE i like this
Attachment #8862619 -
Flags: review?(mcmanus) → review+
Updated•7 years ago
|
Attachment #8862619 -
Flags: review?(valentin.gosu) → review+
Assignee | ||
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3896a7e29f605d0bc509ca98095c1960aa70cf Bug 1359938 - Fix socket status events for TCP Fast Open. r=mcmanus
Comment 12•7 years ago
|
||
Pushed by dd.mozilla@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3896a7e29f Fix socket status events for TCP Fast Open. r=mcmanus
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a3896a7e29f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•