Closed Bug 1313612 Opened 3 years ago Closed 3 years ago

enable named pipe proxy support in NSPR

Categories

(NSPR :: NSPR, defect)

All
Windows
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: xeonchen, Assigned: xeonchen)

References

Details

Attachments

(1 file, 1 obsolete file)

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|
OS: Unspecified → Windows
Hardware: Unspecified → All
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.
Attachment #8805504 - Flags: superreview?(kaie)
Attachment #8805504 - Flags: review?(ted)
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.
(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
Attachment #8805504 - Attachment is obsolete: true
Attachment #8805504 - Flags: superreview?(kaie)
Attachment #8805504 - Flags: review?(wtc)
Attachment #8805504 - Flags: review?(ted)
Attachment #8837067 - Flags: review?(wtc)
Attachment #8837067 - Flags: review?(ted)
Attachment #8837067 - Flags: review?(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
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.
Target Milestone: --- → 4.14
(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
Blocks: 1347932
4.14 branch:
https://hg.mozilla.org/projects/nspr/rev/5c664c77bf57
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
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.
Attachment #8837067 - Flags: review?(kaie) → review+
Attachment #8837067 - Flags: review?(wtc)
You need to log in before you can comment on or make changes to this bug.