1313612 - enable named pipe proxy support in NSPR

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Gary Chen [:xeonchen]]

This is a follow-up of bug 1288308.

Bug 1288308 adds proxy support via named pipe, some modifications are NSPR related:

1) Need path attribute in |PRNetAddr|
2) Named pipe IO layer is a replacement of |PR_NSPR_IO_LAYER|

Comment 1

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1313612-support-named-pipe-proxy-in-NSPR-r-wtc.patch

Comment 2

[Wan-Teh Chang]

Comment on attachment 8805504 [details] [diff] [review]
0001-Bug-1313612-support-named-pipe-proxy-in-NSPR-r-wtc.patch

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

Kai, Ted: please review this patch. Thanks.

Comment 3

[Wan-Teh Chang]

Comment on attachment 8805504 [details] [diff] [review]
0001-Bug-1313612-support-named-pipe-proxy-in-NSPR-r-wtc.patch

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

Hi Gary,

I'm sorry I forgot to review this patch for several months.
Please add the current NSPR owners Ted (:ted.mielczarek)) and
Kai (:kaie) as the reviewers for your NSPR patches in the future.

I have two review comments.

1. This patch seems wrong. It seems that we should not pass this
new type of PRFileDesc to PR_Poll() if PR_Poll() doesn't know
how to handle it.

2. If we decide to use the approach of this patch, please make
the same change to the equivalent code in nspr/pr/src/pthreads/ptio.c.
Please search for "PR_GetIdentitiesLayer" in ptio.c to find the two
PR_ASSERT(NULL != bottom) statements that need to be removed.

Comment 4

[Gary Chen [:xeonchen]]

(In reply to Wan-Teh Chang from comment #3)
> Comment on attachment 8805504 [details] [diff] [review]
> 0001-Bug-1313612-support-named-pipe-proxy-in-NSPR-r-wtc.patch
> 
> Review of attachment 8805504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Gary,
> 
> I'm sorry I forgot to review this patch for several months.
> Please add the current NSPR owners Ted (:ted.mielczarek)) and
> Kai (:kaie) as the reviewers for your NSPR patches in the future.

No problem.

> I have two review comments.
> 
> 1. This patch seems wrong. It seems that we should not pass this
> new type of PRFileDesc to PR_Poll() if PR_Poll() doesn't know
> how to handle it.

When I was working on the named pipe bug, I couldn't find a good way to support Win32 HANDLE type
in PR_Read and PR_Write, so I've created a special PRFileDesc that has its own poll() ([1]).

> 2. If we decide to use the approach of this patch, please make
> the same change to the equivalent code in nspr/pr/src/pthreads/ptio.c.
> Please search for "PR_GetIdentitiesLayer" in ptio.c to find the two
> PR_ASSERT(NULL != bottom) statements that need to be removed.

I'll update the patch later.

[1] https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/netwerk/socket/nsNamedPipeIOLayer.cpp#833

Comment 5

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1313612-support-named-pipe-proxy-in-NSPR-r-wtc-r.patch

Comment 6

[Kai Engert (:KaiE:)]

I see that parts of this patch has already been landed with bug 1288308 into mozilla (it shouldn't have) and is part of FF 52:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da9997b39928
https://hg.mozilla.org/integration/mozilla-inbound/rev/b70e2870aced

Comment 7

[Kai Engert (:KaiE:)]

Because this has already been released with Firefox 52, I think we don't have much choice, but to accept this patch.

Let's check in to NSPR trunk, and see if it passes the tests. We should still try to review the code.

Comment 8

[Kai Engert (:KaiE:)]

checked in without review:
https://hg.mozilla.org/projects/nspr/rev/076df342de92

Comment 9

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #8)
> checked in without review:
> https://hg.mozilla.org/projects/nspr/rev/076df342de92

To clarify:
- a part of this patch has been reviewed by bagder/mayhemer in bug 1288308,
  but they aren't NSPR peers, so it might not have been sufficiently reviewed, 
  if these changes are appropriate for NSPR in general, outside of the context of Firefox

- the remaining portions have been recommended to be added by Wan-Teh, for completeness

Comment 10

[Kai Engert (:KaiE:)]

4.14 branch:
https://hg.mozilla.org/projects/nspr/rev/5c664c77bf57

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 8837067 [details] [diff] [review]
0001-Bug-1313612-support-named-pipe-proxy-in-NSPR-r-wtc-r.patch

r=kaie, because:
- nobody has objected to changing the size of the struct on windows
- it had been released already, so we need it
- the patch implements the completeness suggestions from Wan-Teh

I didn't have time to review if the removal of the assertions has any negative consequences, I'm trusting that others have checked that.