168831 - PR_Poll() ignoring data buffered in io layers

Status: VERIFIED FIXED

(NSPR :: NSPR, defect, P1)

Description

[John G. Myers]

Comment 1

[John G. Myers]

The poll implementation in macsocotpt.c is effectively ignoring out_flags
reported by the poll methods of i/o layers.  Any out_flags set by the function
CheckPollDescMethods() are always overwritten by CheckPollDescEndpoints().

Comment 2

[John G. Myers]

Attachment: proposed fix

Here's how I recommend fixing the bug.	I don't have a mac build environment,
so am not able to test this.

Comment 3

[John G. Myers]

Attachment: proposed fix

Optimize out the GetState() when all in_flags satisfied by the layer.

Comment 4

[Wan-Teh Chang]

Simon, could you review the proposed patch for the Mac
implementation of PR_Poll?  Thanks.

Comment 5

[Simon Fraser [no longer active]]

Those changes look right in spirit, but this is senstive code that will need
thorough testing with the patch.

Comment 6

[Simon Fraser [no longer active]]

BTW, can I have a simple testcase?

Comment 7

[John G. Myers]

Blocked bug 130359 has some testcases.  Basically, read mail with IMAP/SSL and
find a message of about 7k or so.  In my experience, messages that trigger bug
130359 tend to do so fairly reliably.

Another possibility would be to modify DoRecv() in sslsecur.c to randomly return
short packets.

Comment 8

[John G. Myers]

Attachment: patch to exercise bug.

Comment 9

[Kai Engert (:KaiE:)]

This patch fixes the bug 130359 on my Mac OS X machine. (I tested only a debug
build yet.)

Comment 10

[Kai Engert (:KaiE:)]

This fix seems to work. There is a test build referenced in bug 130359, which
fixes that problem for a couple of people. No regressions have been reported. In
addition to Mac OS X, I also tested on OS 9, and SSL seemed to work, too.

Simon, Wan-Teh, do you think more review/testing is needed?

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 99368 [details] [diff] [review]
proposed fix

This looks like the right thing to do for me.

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 99368 [details] [diff] [review]
proposed fix

This looks like the right thing to do to me.

Comment 13

[Simon Fraser [no longer active]]

Do you want me to land the patch? wtc, care to review?

Comment 14

[Wan-Teh Chang]

Comment on attachment 99368 [details] [diff] [review]
proposed fix

>@@ -1823,14 +1825,17 @@
> 
>         if (NULL == pd->fd || pd->in_flags == 0) continue;
> 
>+        if ((pd->in_flags & ~pd->out_flags) == 0) {
>+            ready++;
>+            continue;
>+        }

On other platforms, the test is

	  if ((pd->in_flags & pd->out_flags) != 0) {

so this will be a disparity between Mac and other
platforms.

Your test means that all the bits specified in the
in_flags must be set in the out_flags by the poll
method for a fd to be considered ready.  Is this
really what you want?

>         bottomFD = PR_GetIdentitiesLayer(pd->fd, PR_NSPR_IO_LAYER);
>         /* bottomFD can be NULL for pollable sockets */
>         if (bottomFD)
>         {
>             if (_PR_FILEDESC_OPEN == bottomFD->secret->state)
>             {
>-                pd->out_flags = 0;  /* pre-condition */
>-
>                 if (GetState(bottomFD, &readReady, &writeReady, &exceptReady))
>                 {
>                     if (readReady)
>@@ -1851,8 +1856,8 @@
>                     {
>                         pd->out_flags |= PR_POLL_EXCEPT;
>                     }
>-                    if (0 != pd->out_flags) ready++;
>                 }
>+                if (0 != pd->out_flags) ready++;
>             }
>             else    /* bad state */
>             {

I am not sure if the changes in this "hunk" are really
necessary.  Also, these changes may cause the out_flags
to be set by both the poll method and the GetState call,
which is another disparity from the other platforms.

Another question: can we replace this call in the critical
region
    (void)CheckPollDescEndpoints(pds, npds, readFlags, writeFlags);
by
    ready = CheckPollDescEndpoints(pds, npds, readFlags, writeFlags);
and remove the CountReadyPollDescs function?

Comment 15

[John G. Myers]

The first test is a shortcut.  If all of the interest bits have been set by the
layer, then immediately mark the fd as ready and skip testing the OS-level file.
 Otherwise, we need to check the OS-level file to see if there are additional
out_flags to report.  That later code will then mark the fd as ready.

The second hunk is necessary so that the code that calls GetState() doesn't
clear any out_flags set by the layer.  Consider the case where the caller has
specified interest in both read and write, the layer has indicated only read in
the out_flags, and the underlying socket is not readable.

We could remove CountReadyPollDescs() as you mention.  I suggest that be a
different bug.

Comment 16

[Wan-Teh Chang]

Comment on attachment 99368 [details] [diff] [review]
proposed fix

r=wtc.

I just wanted to point out that this patch
makes PR_Poll behave differently on the Mac.
On the other platforms, if the layer's poll
method sets any flag in the out_flags, PR_Poll
does not consult the OS (the equivalent of
calling GetState) to see if any other flag
should also be set.  With this patch, PR_Poll
will always do both on the Mac.  This is
different from the other platforms but still
correct.

I will write another bug for the removal of
CountReadyPollDescs so that this patch can
be checked in right away.

Simon, could you check this patch in on the
tip and NSPRPUB_PRE_4_2_CLIENT_BRANCH of NSPR?

Comment 17

[Simon Fraser [no longer active]]

drivers approval has been sought.

Comment 18

[John G. Myers]

I believe you can check this into the trunk of NSPR without drivers approval. 
Drivers approval should only be needed to check it into
NSPRPUB_PRE_4_2_CLIENT_BRANCH.

Comment 19

[Simon Fraser [no longer active]]

Right, but it doesn't benefit anyone being on the trunk (the mozilla trunk uses
NSPRPUB_PRE_4_2_CLIENT_BRANCH).

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 99368 [details] [diff] [review]
proposed fix

a=dbaron for Mozilla trunk (i.e., NSPRPUB_PRE_4_2_CLIENT_BRANCH) checkin.

Comment 21

[Wan-Teh Chang]

Comment on attachment 99368 [details] [diff] [review]
proposed fix

I just checked in this patch on the tip and
NSPRPUB_PRE_4_2_CLIENT_BRANCH of NSPR.

I opened bug 176686 for the removal of the
CountReadyPollDescs function.  Simon, I think
you can mark this bug fixed now.

We should seek mozilla 1.0 branch approval for
this patch.

Comment 22

[Simon Fraser [no longer active]]

wtc: many thanks for doing that. I've been distracted by other stuff.
I agree on getting 1.0 branch approval.

Comment 23

[Daniel Veditz [:dveditz]]

adt+ for the branch. please get drivers approval before landing.

Comment 24

[Simon Fraser [no longer active]]

Need some QA on the trunk here. Marking fixed to get on radar.

Comment 25

[lchiang]

No Netscape QA at this point to focus on the trunk for this patch.  But, I'll
ping Gayatri.

Any mozilla.org volunteers to ping on testing this patch thoroughly on the
trunk?  Asa is out.

Comment 26

[Gayatri Rimola]

Added James Hsieh to the cc list.  He is going to help verify this bug on the Mac. 
Adding comments from his email as well: 
James Hsieh said:

> Yes, I have a Mac.  In fact, I can give you test coverage for
> OSX 10.2.1 and OS 9.2 to IMAP servers using UW IMAP servers
> and SunOne Message Servers (I believe it's 5.1.1) from the
> IMAP side, and I can certainly test https as well.


Oh, and yes, I'll obviously test IMAP SSL with these servers.
:-)  This is actually a bug that has been bothering me since
Netscape 7 released.

--James
--
James Hsieh                                     O
Stickman Computing                            --+--
email: [e-mail]            |
http://www.stickman-computing.org/~jhsieh      / \

Comment 27

[James Hsieh]

Attachment: Comments on initial QA testing for OS X 10.2.1

Comments on initial testing of fix

Comment 28

[Simon Fraser [no longer active]]

FTP needs to be tested as well.

Comment 29

[James Hsieh]

Good point.  Tested the following URL:

ftp://ftp.netscape.com/pub/netscape7/english/7.0/mac/macosx/sea/Netscape-macosX.smi.bin

No problems.  Speed the same as downloading with Netscape 7.0 

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 99368 [details] [diff] [review]
proposed fix

a=[e-mail] on behalf of drivers for 1.0 branch checkin.  Sorry for
the delay,

/be

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Don't forget to change mozilla1.0.2+ to fixed1.0.2 when checked in

Comment 32

[Simon Fraser [no longer active]]

Checked into the 1.0 branch.

Comment 33

[Charles Rosendahl]

I've tested the IMAP/SSL connectivity on the trunk as well.  All is good.

Comment 34

[Charles Rosendahl]

verified on the 20021031 branch build

Comment 35

[James Hsieh]

Tested Mozilla 1.2b Nightly Build 2002103113 under MacOS 9.1 on an iMac DV400.
Similar results to my OS X testing.  4452 messages downloaded in about 22 
seconds, all other functionality seems fine.

Been using the OS X version for 2 days now and it's been working great.