Reading a file from js doesn't work

VERIFIED INVALID

Status

()

Core
XPCOM
P3
normal
VERIFIED INVALID
18 years ago
14 years ago

People

(Reporter: Pete Collins (MDG), Assigned: Warren Harris)

Tracking

Trunk
x86
FreeBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
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 =
(Reporter)

Comment 1

18 years ago
Created attachment 5695 [details]
js test case for reading a file

Comment 2

18 years ago
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

18 years ago
scriptable input stream is just a wrapper for the input stream underneath.
(Reporter)

Comment 4

18 years ago
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



(Assignee)

Comment 5

18 years ago
Sounds like a problem with nsBufferedInputStream. I'll take a look.
(Reporter)

Comment 6

18 years ago
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
(Assignee)

Comment 7

18 years ago
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

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

Comment 9

18 years ago
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

18 years ago
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
(Assignee)

Comment 11

18 years ago
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

18 years ago
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

18 years ago
assign to warren to figure out which way this should work.
Assignee: dougt → warren
(Assignee)

Updated

18 years ago
Target Milestone: M15
(Assignee)

Comment 14

18 years ago
Moving non-essential, non-beta2 and performance-related bugs to M17.
Target Milestone: M15 → M17
(Assignee)

Comment 15

18 years ago
Not needed for beta2.
Target Milestone: M17 → M18
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
(Reporter)

Comment 17

18 years ago
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
(Reporter)

Comment 18

18 years ago
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

(Reporter)

Comment 19

18 years ago
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
(Reporter)

Comment 20

18 years ago
Created attachment 8083 [details] [diff] [review]
Warrens diff which seems to fix the Read problem
(Assignee)

Comment 21

18 years ago
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?
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
(Assignee)

Comment 23

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

Comment 24

18 years ago
Use this one please.

Because you are still solving the read problem by adding:

nsIInputStream::GetContentLength

function.


pete
(Assignee)

Comment 25

18 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → INVALID

Comment 26

18 years ago
marking verified as Invalid.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.