590654 - Let JavaScript read embedded nulls from input streams

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

It would be really handy if JS could read from input streams with embedded nulls.  This patch includes a helper method on NetUtil (readInputStreamToString) to make this even easier for JS consumers (no xpcom).

Comment 1

[Shawn Wilsher :sdwilsh]

This blocks bug 568634, which is a beta 5 blocker, which makes this a beta 5 blocker.

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 469145 [details] [diff] [review]
v1.0

>+     * @param aBytes
>+     *        The number of bytes to read from the stream.
I was debating on making this optional and just get whatever is available() if it's not specified.  Would like to hear thoughts on this.

Comment 3

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

Comment on attachment 469145 [details] [diff] [review]
v1.0

+++ b/netwerk/base/src/NetUtil.jsm
+     * Reads aBytes bytes from aInputStream into a string.

Can you rename aBytes to aCount? I think that naming would be more intuitive.

+++ b/netwerk/test/unit/test_NetUtil.js
+  const TEST_DATA = "this is a test string";

Shouldn't you use a string with null bytes? To set that into a string stream, do:

  let istream = Cc["@mozilla.org/io/string-input-stream;1"].
                createInstance(Ci.nsISupportsCString);
  istream.data = TEST_DATA;

+    do_execute_soon(function() {

Why execute_soon?

Comment 4

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

(In reply to comment #2)
> >+     * @param aBytes
> >+     *        The number of bytes to read from the stream.
> I was debating on making this optional and just get whatever is available() if
> it's not specified.  Would like to hear thoughts on this.

Seems simple enough for the caller to specify stream.available()

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> Can you rename aBytes to aCount? I think that naming would be more intuitive.
I did this originally, but aBytes sounded better in the comments.  I don't care either way though, so I can flip it back.

> Shouldn't you use a string with null bytes? To set that into a string stream,
> do:
Ah, yes I should.  Good catch.

> Why execute_soon?
smaller stacks when failures happen

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Per comments

Comment 7

[Boris Zbarsky [:bzbarsky]]

Seems like the WOULD_BLOCK case will first read all the data in the stream, then throw it away and raise WOULD_BLOCK.  That seems broken, no?

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Seems like the WOULD_BLOCK case will first read all the data in the stream,
> then throw it away and raise WOULD_BLOCK.  That seems broken, no?
If we haven't read all the data we are supposed to (which, AFAICT is the only time we would get WOULD_BLOCK), the contract says we throw anyway, right?  This is also the same behavior as NS_ReadInputStreamToString, which I really just wanted to call, but it throws NS_ERROR_UNEXPECTED (which I could just change this new API to do).

Comment 9

[Boris Zbarsky [:bzbarsky]]

The usual nsIInputStream contract is that if you can return any data at all, you return and and don't throw.

NS_ReadInputStreaToString does in fact fail if you ask it to read more bytes than the stream is willing to hand out..... I suppose it's ok to do the same here, as long as the interface very clearly documents that this does NOT have the usual input stream contract.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> The usual nsIInputStream contract is that if you can return any data at all,
> you return and and don't throw.
> 
> NS_ReadInputStreaToString does in fact fail if you ask it to read more bytes
> than the stream is willing to hand out..... I suppose it's ok to do the same
> here, as long as the interface very clearly documents that this does NOT have
> the usual input stream contract.
I'm not sure on what the right behavior is here to be honest.  At least with JS, you can't throw and return something, so I'm not sure how to convey that you have some data still without having an awkward API.

But, if we go with the currently implemented behavior, ss this sufficient, or do you want more (suggestions welcome):
>+     * @throws NS_ERROR_FAILURE if there are not enough bytes available to read
>+     *         aCount amount of data.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That seems fine to me.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 469180 [details] [diff] [review]
v1.1

r=me with that comment added and an apostrophe added in "caller's location".

Comment 13

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

Comment on attachment 469180 [details] [diff] [review]
v1.1

+++ b/xpcom/io/nsIScriptableInputStream.idl
+                   [size_is(aCount), retval] out ACString aString);

Interesting, I didn't know size_is worked with ACString. Why not just make this:
    ACString readBytes(in unsigned long aCount);

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> r=me with that comment added and an apostrophe added in "caller's location".
This was documented at both locations, but is now using the wording in comment 10, which was how it was documented in NetUtil.jsm.

Also fixed grammar error.

(In reply to comment #13)
> Interesting, I didn't know size_is worked with ACString. Why not just make
> this:
>     ACString readBytes(in unsigned long aCount);
I don't know why I did it that way in the first place.  Updated to make it work this way instead.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

per comments, ready for checkin

Comment 16

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/b6f61845c3dd

Comment 17

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/JavaScript/Code_modules/NetUtil.jsm#readInputStreamToString()

And listed on Firefox 4 for developers.

Comment 18

[Eric Shepherd [:sheppy]]

Also mentioned on the doc page for nsIInputStream.

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> Also mentioned on the doc page for nsIInputStream.
Do you mean nsIScriptableInputStream?

Comment 20

[Eric Shepherd [:sheppy]]

Um. Yeah, that's what I mean. Hey, look over there! *pointing*