97866 - Async file I/O problems in Mac NSPR

Status: VERIFIED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Simon Fraser [no longer active]]

In working on bug 71718, I discovered some issues with the async file I/O code on 
Mac.

It currently looks like this:

if (op == READ_ASYNC)
{
  /*
  **  Skanky optimization so that reads < 20K are actually done synchronously
  **  to optimize performance on small reads (e.g. registry reads on startup)
  */
  if ( bytes > 20480L )
  {
    err = PBReadAsync(&pbAsync.pb);
    if (err != noErr && err != eofErr)
      goto ErrorExit;
    
    me->io_pending = PR_TRUE; /* Only mark thread io pending if async call worked 
*/
  }
  else
  {
    (void) PBReadSync(&pbAsync.pb);
    /*
    ** This is probbaly redundant but want to make sure we indicate the read
    ** is complete so we don't wander off into the Sargasso Sea of Mac
    ** threading
    */
    pbAsync.pb.ioParam.ioResult = 0;
  }
}
else
{
  /* writes are currently always async */
  err = PBWriteAsync(&pbAsync.pb);
    if (err != noErr)
      goto ErrorExit;
  
  /* Didn't get an error on the asyn call so mark thread io pending */
  me->io_pending = PR_TRUE;
}

/* See if the i/o call is still pending before we actually yield */
if (pbAsync.pb.ioParam.ioResult == 1)
  WaitOnThisThread(me, PR_INTERVAL_NO_TIMEOUT);
else
  me->io_pending = PR_FALSE; /* io completed so don't mark thread io pending */

There are two problems here.

First, here:

    err = PBReadAsync(&pbAsync.pb);
...
    me->io_pending = PR_TRUE;

You risk a race between the notifier and this code in setting me->io_pending. 
It's quite possible for the notifer to hit before you come out of PBReadAsync(), 
set io_pending to PR_FALSE, causing you to wait forever lower down (on Mac OS X, 
the 'ioResult == 1' trick does not seem to work, and anyway, io_pending and 
ioResult are not strictly synchronized).

Second, if you make an async call, then detect pbAsync.pb.ioParam.ioResult == 1 
so skip the WaitOnThisThread(), it's possible to leave this routine before the 
notifier fires. This was happening on Mac OS X. The notifier then gets some 
random stack garbage in its param, which is bad. *If* you make an async call, 
then you *must* call WaitOnThisThread(), even if you think the notifer has fired 
already.

Comment 1

[Jussi-Pekka Mantere]

Marking this to block Bug 98405; spreading keyword love.

Comment 2

[Jussi-Pekka Mantere]

Setting platform OS X, P1, critical.

Needs an appropriate NSPR milestone to ship with 0.9.5.

Comment 3

[Simon Fraser [no longer active]]

Note that the patch in bug 71718 will fix this.

Comment 4

[Simon Fraser [no longer active]]

Attachment: patch to macio.c

Comment 5

[Simon Fraser [no longer active]]

The patch ensures that we always call WaitOnThisThread() if we are doing an asyc
read or write, and that thread->io_pending flag is properly maintained.

Reviews, please (wtc, sdagley/gordon).

Comment 6

[Steve Dagley]

r=sdagley

One comment/suggestion though - the 20K threshold for sync vs. async reads was 
POOMA based on the size of file reads during startup 2 years ago.  We might want 
to change the that to 32K so we're aligned on an 8K boundary (and maybe re-check 
the size of file reads during startup)

Comment 7

[Simon Fraser [no longer active]]

Attachment: Patch to macio.c

Comment 8

[Simon Fraser [no longer active]]

New patch based on discussion with Gordon: Changes:
* It turns out that PB{Read,Write}Async *never* return errors directly (see
  <http://developer.apple.com/technotes/fl/fl_515.html>), so I removed the
  error handling from those calls.
* I added error handling for the PBReadSync call, which does need it.
* I removed an unneccessary setting of pbAsync.pb.ioParam.ioResult in
  the sync read case.

Comment 9

[gordon]

r=gordon

Comment 10

[Jaime Rodriguez, Jr.]

This is not on Tian Tian Must Fix list for this release. Should we onsider
minusing it, and changing the priority for this release?

Comment 11

[Steve Dagley]

A P1 critical w/nsbranch+ and nsenterrise+ and you don't think it's a must fix?  
It is.  I don't think tiantian is querying for NSPR bugs and/or the 4.2 milestone 
is throwing the query off

Comment 12

[Jaime Rodriguez, Jr.]

Get with Tian Tian, and make sure here query captures ALL the STOP SHIPS!!!

Comment 13

[Jaime Rodriguez, Jr.]

Check it into the trunk. Get people to test it, and let us know when you fill
comfonfident, and we'll give you a PDT+.

Comment 14

[Wan-Teh Chang]

a=wtc.

Comment 15

[Simon Fraser [no longer active]]

Fix checked in to NSPR tip and NSPR_PRE_4_2_CLIENT_BRANCH. Keeping bug open for 
checkin to 0.9.4 branch.

Comment 16

[Jaime Rodriguez, Jr.]

Simon - Let's talk about this one @ tomorrow's PDT meeting.

Comment 17

[Jaime Rodriguez, Jr.]

Lisa - We need to know how well this fix works on all Mac OS. Let's let it bake
fora while and get some more testing.

Comment 18

[lchiang]

Please advise on what items to test on the trunk.  I may need to get Mozilla QA
to help out on this testing on the trunk.

Comment 19

[Simon Fraser [no longer active]]

This is low-level code that handles just about all disk file I/O that mozilla 
does: disk cache, file reading on startup, downloading, mail summary files etc 
etc.

Comment 20

[lchiang]

I will coordinate with Asa to try some tests.  Thanks.   I assume this is on all
the Mac OS's.

Comment 21

[Jaime Rodriguez, Jr.]

check it in - PDT+.

Comment 22

[grega]

marking nsenterprise- to get off the enterprise stop ship list.

Comment 23

[Jaime Rodriguez, Jr.]

Pls check this one in today.

Comment 24

[Brian Nesse (gone)]

*** Bug 98405 has been marked as a duplicate of this bug. ***

Comment 25

[Simon Fraser [no longer active]]

This is already on the NSPR tip.

Comment 26

[lchiang]

I am marking verified.  This affects many basic operations on the Mac.  Since
the landing of this fix, there haven't been major problems with the disk cache,
file reading on startup, downloading, mail summary files etc, as Simon suggested
we try.