278786 - Scriptable inputstream needs not call available in Read

Status: REOPENED

(Core :: XPCOM, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Since input streams should deal with a count > their available data, this
Available call is not needed. what's more, it breaks reading files from /proc.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Darin Fisher]

Comment on attachment 171587 [details] [diff] [review]
patch

r+sr=darin

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

so, as a note, the reason this fails for /proc files is that PR_Available is
implemented by seeking to the end of the file and then back, which fails for
proc files...
(http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/pthreads/ptio.c#1468)

Comment 4

[Darin Fisher]

I wonder why PR_Available isn't implemented using fstat.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

since there is no disadvantage in not calling read, I checked this patch in
despite comment 4.

Checking in xpcom/io/nsScriptableInputStream.cpp;
/cvsroot/mozilla/xpcom/io/nsScriptableInputStream.cpp,v  <-- 
nsScriptableInputStream.cpp
new revision: 1.8; previous revision: 1.7
done

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

random trivia: this saved 52 bytes of codesize:
  	        -52 (+0/-52)	text (DATA)
  		        -52 (+0/-52)	UNDEF:libxpcom_core.so:text
  			         -2	.nosyms.text
  			        -50	nsScriptableInputStream::Read(unsigned, char**)

(on luna)

Comment 7

[Boris Zbarsky [:bzbarsky]]

So.. some consumers are passing in "-1" as the number of bytes to read.  With
this patch, will we try to allocate PRUint32(-1) bytes?  Or do something else?

Comment 8

[Darin Fisher]

> So.. some consumers are passing in "-1" as the number of bytes to read.  With
> this patch, will we try to allocate PRUint32(-1) bytes?  Or do something else?

Actually, you're right.  This patch shouldn't have gone in.  PR_UINT32_MAX is an
acceptable value for the aCount parameter to nsIInputStream::read, and it should
also be an acceptable value for the aCount parameter to
nsIScriptableInputStream::read, as it says:

  "@param aCount [in] the maximum number of bytes to read"

Christian: Back to the drawing board then?  Want to look into fixing
PR_Available instead?  Or, we could "stat" in nsFileInputStream::Available. 
Thoughts?

REOPENING

Comment 9

[Darin Fisher]

bz: to answer your question.. we'll allocate a 0-length buffer (aCount + 1), but
we'll lie to the underlying stream and tell it the buffer length is aCount bytes
long!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fun.  ;)

I think the right solution may be that if aCount is more than 4096 or something
we read the stream in a loop in 4096-byte chunks, and realloc the buffer as
needed.....

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

urg

implementing Available with fstat might be fun, given:
> LANG=C stat /proc/cpuinfo
  File: `/proc/cpuinfo'
  Size: 0               Blocks: 0          IO Block: 1024   regular empty file


An alternative to bz's suggestion might be to call available iff aCount ==
PR_UINT32_MAX... but that kinda sucks, so I'll probably implement someting like
bz's suggestion

Comment 12

[Wan-Teh Chang]

It seems bad for PR_Available to be implemented
with seek calls that move the file pointer to
the end of file and back.  If two threads call
PR_Available on the same file descriptor at the
same file, the interleaved seek calls can be
problematic.  I guess it's implemented this way
because fstat returns more information than
PR_Available needs.

Regarding the special files in /proc, I'm
wondering if ioctl(fd, FIONREAD, &bytes)
works.

Comment 13

[Samuel Sieb]

This would apply to pipes as well.

Comment 14

[Darin Fisher]

hmm, yeah... i would expect "ioctl(fd, FIONREAD, &bytes)" to be the right
solution here as well.  biesi: can you give that a try?

Comment 15

[Darin Fisher]

nevermind... the proc filesystem seems to return 0 bytes for FIONREAD :(

test prog:

  #include <sys/types.h>
  #include <sys/stat.h>
  #include <sys/ioctl.h>
  #include <fcntl.h>
                                                                                
  int main(int argc, char **argv)
  {
    int fd, bytes;
                                                                                
    fd = open(argv[1], O_RDONLY);
    if (fd == -1)
      return 1;
                                                                                
    ioctl(fd, FIONREAD, &bytes);
    printf("size: %d\n", bytes);
    return 0;
  }

Comment 16

[Wan-Teh Chang]

You should check the return value of ioctl.
Maybe it doesn't work on /proc files.

In any case, we don't seem to have a solution
to make PR_Available work on /proc files.

Comment 17

[Darin Fisher]

> You should check the return value of ioctl.
> Maybe it doesn't work on /proc files.

Unfortunately, it returns a success code :(

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

So... what if I just do:
  aCount = PR_MIN(4096, aCount);
in nsScriptableInputStream::Read? that way the maximum data to read is bounded.
The interface does not guarantee that all data will be read when passing
PR_UINT32_MAX, and in fact for things like sockets and channels, that will not
be the case.

It does break code that assumes passing -1 when reading a file will give the
entire contents. is that a problem?

Comment 19

[Darin Fisher]

> It does break code that assumes passing -1 when reading a file will give the
> entire contents. is that a problem?

I think that's ok, but we should verify that consumers like chatzilla and
venkman are still happy.

Comment 20

[Jon Stritar]

We use read(-1) in Forecastfox. With this patch it breaks the current versions
of the extension. Its not hard at all to fix it for future versions, but it may
be a pain for all the people using it when they upgrade to Firefox 1.1 or a nightly.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

guys, read(-1) will work in firefox 1.1. (in the sense of "will not fail"). do
not worry. it may not read the entire stream though (which was never guaranteed).

Comment 22

[Darin Fisher]

biesi: perhaps we should only do the 4096 thing when Available fails?  (if it
turns out that people were depending on read(-1) reading the entire file.)  i'd
rather not resort to this approach, but we should keep it in mind in case we
find that we are breaking several extensions.

Comment 23

[Nickolay_Ponomarev]

fwiw, you will be breaking quite a few exensions. Those who use this[1] js
wrapper or those who copied their code from [2]. I don't know how many
extensions is that break, but I guess quite a few.

[1] http://gratisdei.com/io.js
[2] http://kb.mozillazine.org/Dev_:_Extensions_:_Example_Code_:_File_IO

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

I backed out the patch for now.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Could we fix that documentation to not give bad advice?  There are plenty of
streams (non-blocking ones, in particular) where that approach would have failed
even before the changes for this bug....

Comment 26

[Darin Fisher]

I agree.  Part of whatever patch we come up with here should include
documentation revision to include a warning of that sort.  Also, it is not just
non-blocking streams where this would be a problem.  Indeed, a blocking input
stream returned from nsHttpChannel::Open would have the same problem since it
only blocks until it can return >0 bytes (or error), and there are cases where
Available would not return the full content length (b/c maybe it is not known in
advance -- think transfer-encoding: chunked).

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

so scriptableinputstream users probably should read until 0 bytes are read
(indicating eof)?

(that's what other streams do; does the scriptable stream follow that?)

Comment 28

[Worcester12345]

(In reply to comment #25)
> Could we fix that documentation to not give bad advice?  There are plenty of
> streams (non-blocking ones, in particular) where that approach would have failed
> even before the changes for this bug....

I'd be interested to see this happen. Does a new bug need to be filed for that?

Comment 29

[Boris Zbarsky [:bzbarsky]]

The documentation in question is not mozilla.org documentation, so if a bug
needs to be filed it needs to be filed with the actual authors of the docs.

Comment 30

[Bill Gianopoulos [:WG9s]]

Not to ask a stupid question (and this probably is) but ...

I realize that this would have the opposite effect of reducing the size of the
code base, but isn't the obvious solution to use the old code calling Available
for the -1 case and use the new code for other values?  This would not break the
-1 case but still allow read to work for the /proc filesystem which I thought
was what was orginally trying to be solved here.

Comment 31

[Boris Zbarsky [:bzbarsky]]

That would just mean that an attempt to read an entire file from /proc (with -1)
would fail.

Comment 32

[Bill Gianopoulos [:WG9s]]

(In reply to comment #31)
> That would just mean that an attempt to read an entire file from /proc (with -1)
> would fail.

But that is something that did not work previously, and the original patch,
which caused the regression, left broken as well.  It was just a suggestion.

Comment 33

[Boris Zbarsky [:bzbarsky]]

That's not a good reason to purposefully write code that doesn't work right when
there are several proposals in this bug on how it could work correctly....