Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

other
4.16

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch bug_tfo_nspr_minor.patch (obsolete) — Splinter Review
I made minor changes to TFO nspr code. I do not belive that this could influence crashes that we experience with TFO, but better try everything.

1) Use consistent TRUE/FALSE for alreadyConnected and overlappedActive.
2) use windows function signature for LPFN_CONNECTEX in stead of one copied from https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/third_party/libevent/iocp-internal.h#59
Attachment #8886120 - Flags: review?(honzab.moz)
Assignee

Comment 1

2 years ago
I will ping Kai for a review as well as soon as Honza approve it.
Assignee

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26c90da0f4c9a30ffbdb76fc4e389ed8d75dfb38

Try run looks good. Talos test q1 should actually really use TFO. It is contacting servers that have it enabled.
Comment on attachment 8886120 [details] [diff] [review]
bug_tfo_nspr_minor.patch

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

r+ with the comments fixed.

::: nsprpub/pr/src/io/prio.c
@@ +120,5 @@
>          fd->secret->state = _PR_FILEDESC_OPEN;
>  	fd->secret->md.osfd = osfd;
>  #if defined(_WIN64)
> +        fd->secret->alreadyConnected = FALSE;
> +        fd->secret->overlappedActive = FALSE;

PR_TRUE/PR_FALSE variant was the correct one here, those are both defined as PRBool.

there are two places assigning FALSE to overlappedActive member:

https://dxr.mozilla.org/mozilla-central/search?q=overlappedActive&redirect=false

both should update to PR_FALSE.


alreadyConnected doesn't need any changes:

https://dxr.mozilla.org/mozilla-central/search?q=alreadyConnected&redirect=false

::: nsprpub/pr/src/md/windows/w95sock.c
@@ +351,5 @@
>  #ifndef SO_UPDATE_CONNECT_CONTEXT
>  #define SO_UPDATE_CONNECT_CONTEXT 0x7010
>  #endif
>  
> +static _pr_win_connectex_ptr _pr_win_connectex = NULL;

c++ contracts this to be nullified, but this doesn't hurt :)

@@ +469,5 @@
>               * them during Fast Open or after connect. Therefore we can assumed
>               * this data already send. */
> +            if (amount) {
> +              return amount;
> +            } else {

no else after return
Attachment #8886120 - Flags: review?(honzab.moz) → review+
Assignee

Comment 4

2 years ago
Posted patch bug_tfo_nspr_minor.patch (obsolete) — Splinter Review
addressed Honza's comments
Attachment #8886120 - Attachment is obsolete: true
Attachment #8886196 - Flags: review?(kaie)
Comment on attachment 8886196 [details] [diff] [review]
bug_tfo_nspr_minor.patch

>             fd->secret->overlappedActive = PR_TRUE;
>-            _PR_MD_MAP_CONNECT_ERROR(WSAEWOULDBLOCK);
>+
>             /* ConnectEx will copy supplied data to a internal buffer and send
>              * them during Fast Open or after connect. Therefore we can assumed
>              * this data already send. */
>-            return amount;
>+            if (amount) {

Do you think it might be better to limit the non-error return to positive results like this?
  if (amount > 0)

r=kaie with or without this change, my suggestion might be wrong, 
I rely on you to double check if this makes sense,
please let me know your decision.


>+              return amount;
>+            }
>+
>+            _PR_MD_MAP_CONNECT_ERROR(WSAEWOULDBLOCK);
>+            return -1;
Attachment #8886196 - Flags: review?(kaie) → review+
Assignee

Comment 6

2 years ago
(In reply to Kai Engert (:kaie:) from comment #5)
> Comment on attachment 8886196 [details] [diff] [review]
> bug_tfo_nspr_minor.patch
> 
> >             fd->secret->overlappedActive = PR_TRUE;
> >-            _PR_MD_MAP_CONNECT_ERROR(WSAEWOULDBLOCK);
> >+
> >             /* ConnectEx will copy supplied data to a internal buffer and send
> >              * them during Fast Open or after connect. Therefore we can assumed
> >              * this data already send. */
> >-            return amount;
> >+            if (amount) {
> 
> Do you think it might be better to limit the non-error return to positive
> results like this?
>   if (amount > 0)
> 
> r=kaie with or without this change, my suggestion might be wrong, 
> I rely on you to double check if this makes sense,
> please let me know your decision.
> 

You suggestion is valid. I will change it. It is possible to call _PR_MD_TCPSENDTO with amount=0 and it is more logical to return -1 in that case. Thanks! 

> 
> >+              return amount;
> >+            }
> >+
> >+            _PR_MD_MAP_CONNECT_ERROR(WSAEWOULDBLOCK);
> >+            return -1;
Assignee

Comment 7

2 years ago
actually amount=0 is covered by my version. I do not know what the windows connectEx implementation will do if amount is a negative number.

I will make a change to be on the safe side.
Assignee

Comment 8

2 years ago
Attachment #8886196 - Attachment is obsolete: true
Attachment #8886539 - Flags: review+
Assignee

Comment 10

2 years ago
If the try run from comment 9 is green, I will push this patch to mozilla-inbound. I am currently running some tests with TFO turned on so I could test this patch immediately as well.
(In reply to Dragana Damjanovic [:dragana] from comment #10)
> If the try run from comment 9 is green, I will push this patch to
> mozilla-inbound.

The patch is in NSPR, so it needs to be pushed to NSPR (not m-i).
Assignee

Comment 12

2 years ago
(In reply to Kai Engert (:kaie:) from comment #11)
> (In reply to Dragana Damjanovic [:dragana] from comment #10)
> > If the try run from comment 9 is green, I will push this patch to
> > mozilla-inbound.
> 
> The patch is in NSPR, so it needs to be pushed to NSPR (not m-i).

yes, Iknow but i can push it to m.-i. anyway and it is goung to be overwritten at some point
There's now a commit hook in place that will reject all attempted changes to the NSPR directory that aren't an upgrade to a newer NSPR snapshot.
Once the try build from comment 9 is green, I can help to land into NSPR and uplift a new NSPR beta snapshot into m-i, as part of bug 1372579.
Assignee

Comment 15

2 years ago
(In reply to Kai Engert (:kaie:) from comment #13)
> There's now a commit hook in place that will reject all attempted changes to
> the NSPR directory that aren't an upgrade to a newer NSPR snapshot.

Thanks. I did not know that :)
Should it work to land the NSPR uplift first, prior to the other changes from your try build?
Assignee

Comment 17

2 years ago
(In reply to Kai Engert (:kaie:) from comment #16)
> Should it work to land the NSPR uplift first, prior to the other changes
> from your try build?

Yes. The other change is just a pref to turn on TFO.
https://hg.mozilla.org/projects/nspr/rev/6bbe01dd9325
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.16
You need to log in before you can comment on or make changes to this bug.