29074 - Reading a file from js doesn't work

Status: VERIFIED INVALID

(Core :: XPCOM, defect, P3)

Description

[Pete Collins (MDG)]

Test script atached:

To test, just call: readFile(path);

Where path is the path to an existing file.

DUMP OUT (UNIX)
*** Prepare File ***



**** Checking to see if the file exists ****

File "/home/petejc/IO/foo.dap" exists = true

PATH i am trying to read = /home/petejc/IO/foo.dap
available = 0
fileContents = 



DUMP OUT(WIN)

*** Prepare File ***



**** Checking to see if the file exists ****

File "C:\foo.dap" exists = true

PATH i am trying to read = C:\foo.dap
available = 0
fileContents =

Comment 1

[Pete Collins (MDG)]

Attachment: js test case for reading a file

Comment 2

[Doug Turner (:dougt)]

At first look, it appears to be a sync problem.  One one thread, we are opening 
the nsIFile and on the other we are calling nsScriptableInputStream::Available() 
which assumes the file is opened.  ccing warren and jud.

Comment 3

[Judson Valeski]

scriptable input stream is just a wrapper for the input stream underneath.

Comment 4

[Pete Collins (MDG)]

I think i found the problem with this I just need some help fixing it.

go to file nsScriptableInputStream.cpp

then go to the function: nsScriptableInputStream::Read

then go down to: 
rv = mInputStream->Read(buffer, count, &amtRead);

count is wrong.

What i did is hard code this:

rv = mInputStream->Read(buffer, 4/*count*/, &amtRead);

Because i know my file is 4 chars long.

After i did this it worked fine.

Can anybody help me figure out why the arg aCount is not correct??

I get a value of 0 for aCount and it should be 4.

I am guessing that aCount's value comes from Available()

So is there something wrong in the Available() function??


pete

Comment 5

[Warren Harris]

Sounds like a problem with nsBufferedInputStream. I'll take a look.

Comment 6

[Pete Collins (MDG)]

Also do note guys that i think tis is wrong in function Read:

rv = mInputStream->Read(buffer, count, &amtRead);

it should be:

rv = mInputStream->Read(buffer, aCount, &amtRead);

aCount not count.

If i use aCount i can then pass a size from javascript.


pete

Comment 7

[Warren Harris]

Pete: I don't think your suggestion is right. You can overrun the buffer with 
that.

The real problem is nsBufferedInputStream::Available which doesn't try and go 
get more data if there is currently nothing in the buffer. The following patch 
fixes that:

Index: nsBufferedStreams.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/base/src/nsBufferedStreams.cpp,v
retrieving revision 1.4
diff -c -r1.4 nsBufferedStreams.cpp
*** nsBufferedStreams.cpp	2000/02/21 05:22:43	1.4
--- nsBufferedStreams.cpp	2000/02/29 23:58:23
***************
*** 180,186 ****
  NS_IMETHODIMP
  nsBufferedInputStream::Available(PRUint32 *result)
  {
!     *result = mFillPoint - mCursor;
      return NS_OK;
  }
  
--- 180,194 ----
  NS_IMETHODIMP
  nsBufferedInputStream::Available(PRUint32 *result)
  {
!     nsresult rv;
!     PRUint32 amt = mFillPoint - mCursor;
!     if (amt == 0) {
!         rv = Fill();
!         if (NS_SUCCEEDED(rv)) {
!             amt = mFillPoint - mCursor;
!         }
!     }
!     *result = amt;
      return NS_OK;
  }
  

Give it a try and let me know if it fixes the problem.

Warren

Comment 8

[Judson Valeski]

I think we've needed this patch for a long time. Note that this now turns 
Available() into a blocking call though. hmm.

Comment 9

[Warren Harris]

Good point Jud. Here's attempt number 2:

Index: nsBufferedStreams.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/base/src/nsBufferedStreams.cpp,v
retrieving revision 1.4
diff -c -r1.4 nsBufferedStreams.cpp
*** nsBufferedStreams.cpp	2000/02/21 05:22:43	1.4
--- nsBufferedStreams.cpp	2000/03/01 00:26:46
***************
*** 180,186 ****
  NS_IMETHODIMP
  nsBufferedInputStream::Available(PRUint32 *result)
  {
!     *result = mFillPoint - mCursor;
      return NS_OK;
  }
  
--- 180,199 ----
  NS_IMETHODIMP
  nsBufferedInputStream::Available(PRUint32 *result)
  {
!     nsresult rv;
!     PRUint32 amt = mFillPoint - mCursor;
!     if (amt == 0) {
!         PRUint32 sourceAmt;
!         rv = Source()->Available(&sourceAmt);
!         if (NS_FAILED(rv)) return rv;
!         if (sourceAmt > 0) {
!             // Fill calls Read on the source, and shouldn't block if it
!             // already declared that some data was available:
!             (void)Fill();
!             amt = mFillPoint - mCursor;
!         }
!     }
!     *result = amt;
      return NS_OK;
  }
  

Comment 10

[pete collins]

Warren, the patch doesn't work.

I think i just found something interesting.

I don't think 

nsBufferedInputStream::Available is ever called.

I think nsPipeInputStream::Available instead.

I came to this conclusion by putting printf's in each function.
I got the output back from nsPipeInputStream::Available only.

what do you think??

pete

Comment 11

[Warren Harris]

I see. Then it's like Doug said -- one end is trying to read while the other 
end hasn't even opened the file yet.

So as I think about this, there are 2 possibilities:

1. nsScriptableInputStream::Read should not be calling Available at all since 
you really want the underlying Read call to block and/or return less than the 
requested amount -- at least for scriptable streams anyway. This causes the 
problem that you don't know how much space to allocate for the returned string. 
You could allocated the maximum size for the requested amount, but that might 
be wasteful. This solution would probably work for now.

2. Maybe the real problem here is the semantics of Available. For a blocking 
stream, maybe it should block itself, and never return 0 except for EOF. We 
don't do that now. Cc'ing jband, brendan and rpotts on this.

I still think the nsBufferedInputStream fix is right though, even if not needed 
in this case.

Comment 12

[Judson Valeski]

1. yup. tough call. picking any size out of a hat will hurt.
2. agreed. I like the second of your two patches.

Comment 13

[Doug Turner (:dougt)]

assign to warren to figure out which way this should work.

Comment 14

[Warren Harris]

Moving non-essential, non-beta2 and performance-related bugs to M17.

Comment 15

[Warren Harris]

Not needed for beta2.

Comment 16

[Brendan Eich [:brendan]]

I believe folks can use nsILocalFile via XPConnect to read a file without 
benefit of a stream.  Pete, is that a workaround for you?

It seems to me that Available should "block" if it must ask another thread to do 
something (stat or open a file) in order to give a true answer.  That may not be 
the same as making Available block for all "blocking streams" -- we don't want 
Available to generate HTTP or TCP/IP traffic on a single-threaded stream stack 
that's talking to a remote server, it seems to me.  Warren, is this distinction 
between local and remote blocking right?

/be

Comment 17

[Pete Collins (MDG)]

Brendan, I am going to look back into all this stuff next week. I may have
already tried your suggestion but i can't remember.

Warren said he landed a bunch of changes so i need reexamine all of the
js/xpconnect IO stuff and see where we stand now.


pete

Comment 18

[Pete Collins (MDG)]

Yep, i understand the problem is some syncing of some sort between the
classes/streams.
When Available is called:


NS_IMETHODIMP
nsScriptableInputStream::Available(PRUint32 *_retval) {
    printf("calling Available ****\n\n");
    if (!mInputStream) return NS_ERROR_NOT_INITIALIZED;
    return mInputStream->Available(_retval);
}

it is returning 0

so when you do this:

count = PR_MIN(count, aCount);

you make count 0 also.

I see what Warren says about passing aCount raw in from js that you can overflow
the buffer.

So what about if for available we used:

nsLocalFile::GetFileSize(PRInt64 *aFileSize)

Then we would have the size of the file to compare with the raw js being passed
in.

I am not sure if this would be a good solution. First i guess it assumes that
Available is only comparing local files??

It this could work, how would i call nsLocalFile::GetFileSize from this
Available function??

If solving this problem is not a *huge* task perhaps i can take a stab at it?

Ideas??

pete

Comment 19

[Pete Collins (MDG)]

Warren i just tried you patch below again to  nsBufferedStreams.cpp
and it looks like it solves the problem now. I have read working here!!

Yahoo!!!! Warren be da man . . .


NS_IMETHODIMP
nsBufferedInputStream::Available(PRUint32 *result)
  {
    printf("inside nsBufferedInputStream Available ****\n\n");
     nsresult rv;
     PRUint32 amt = mFillPoint - mCursor;
     if (amt == 0) {
         rv = Fill();
         if (NS_SUCCEEDED(rv)) {
             amt = mFillPoint - mCursor;
         }
     }
     *result = amt;
      return NS_OK;
}




pete

Comment 20

[Pete Collins (MDG)]

Attachment: Warrens diff which seems to fix the Read problem

Comment 21

[Warren Harris]

Hold on there... I'm not sure that we really want to make Available be a 
blocking call. Seems to defeat the purpose. So I'm not sure what the best thing 
to do is. Maybe we need to distinguish Available (how much is in the 
pipe) from a GetContentLength (the logical size) (which we don't have yet). 
Brendan?

Comment 22

[Brendan Eich [:brendan]]

Warren, thanks for asking: we should distinguish "how much can I read without
blocking, or doing even any kind of protocol operation such as a TCP window
probe" from "how much is there to read form this stream, in total (if known)."

We should have both.  Available() is an ok name.  GetContentLength() is better. 
Why not use these for the bytes-available-in-local-buffer and bytes-in-total
methods?  I'm for it.  What's the bug number?

/be

Comment 23

[Warren Harris]

Do we need a new bug for that change (adding nsIInputStream::GetContentLength) 
or can we just use this one?

Comment 24

[Pete Collins (MDG)]

Use this one please.

Because you are still solving the read problem by adding:

nsIInputStream::GetContentLength

function.


pete

Comment 25

[Warren Harris]

We discussed this in the api review meeting last week, and decided the 
following:

1. Available is correctly defined to return the amount of data that may be read 
without blocking. Our implementations are also correct in this regard (it is a 
coincidence that Available = GetContentLength for nsFileInputStream).

2. We are not going to add GetContentLength to nsIInputStream because there are 
streams that just can't know their content length. 

Consequently, this bug is invalid, and the js code in the attachment is wrong. 
It needs a loop to read until Read returns 0.

Comment 26

[leger]

marking verified as Invalid.