1380612 - Minor changes to TFO

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement)

Description

[Dragana Damjanovic [:dragana]]

Attachment: bug_tfo_nspr_minor.patch

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

Comment 1

[Dragana Damjanovic [:dragana]]

I will ping Kai for a review as well as soon as Honza approve it.

Comment 2

[Dragana Damjanovic [:dragana]]

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 3

[Honza Bambas (:mayhemer)]

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

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_tfo_nspr_minor.patch

addressed Honza's comments

Comment 5

[Kai Engert (: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;

Comment 6

[Dragana Damjanovic [:dragana]]

(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;

Comment 7

[Dragana Damjanovic [:dragana]]

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.

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_tfo_nspr_minor.patch

Comment 9

[Dragana Damjanovic [:dragana]]

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

Comment 10

[Dragana Damjanovic [:dragana]]

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.

Comment 11

[Kai Engert (:KaiE:)]

(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).

Comment 12

[Dragana Damjanovic [:dragana]]

(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

Comment 13

[Kai Engert (:KaiE:)]

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.

Comment 14

[Kai Engert (:KaiE:)]

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.

Comment 15

[Dragana Damjanovic [:dragana]]

(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 :)

Comment 16

[Kai Engert (:KaiE:)]

Should it work to land the NSPR uplift first, prior to the other changes from your try build?

Comment 17

[Dragana Damjanovic [:dragana]]

(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.

Comment 18

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nspr/rev/6bbe01dd9325

Comment 19

[Kai Engert (:KaiE:)]

uplifted here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71af7e82694f8d5b7fa547800979aafec665a56a