Last Comment Bug 590654 - Let JavaScript read embedded nulls from input streams
: Let JavaScript read embedded nulls from input streams
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b5
Assigned To: Shawn Wilsher :sdwilsh
:
Mentors:
Depends on: 632609
Blocks: 568634
  Show dependency treegraph
 
Reported: 2010-08-25 12:53 PDT by Shawn Wilsher :sdwilsh
Modified: 2011-02-08 16:16 PST (History)
6 users (show)
sdwilsh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5+


Attachments
v1.0 (8.48 KB, patch)
2010-08-25 12:53 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Review
v1.1 (8.78 KB, patch)
2010-08-25 13:57 PDT, Shawn Wilsher :sdwilsh
bzbarsky: review+
cbiesinger: superreview+
Details | Diff | Review
v1.2 (8.77 KB, patch)
2010-08-26 14:17 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Review

Description Shawn Wilsher :sdwilsh 2010-08-25 12:53:44 PDT
Created attachment 469145 [details] [diff] [review]
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 2010-08-25 12:54:44 PDT
This blocks bug 568634, which is a beta 5 blocker, which makes this a beta 5 blocker.
Comment 2 Shawn Wilsher :sdwilsh 2010-08-25 12:56:32 PDT
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) 2010-08-25 13:07:38 PDT
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) 2010-08-25 13:13:13 PDT
(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 2010-08-25 13:46:47 PDT
(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 2010-08-25 13:57:15 PDT
Created attachment 469180 [details] [diff] [review]
v1.1

Per comments
Comment 7 Boris Zbarsky [:bz] 2010-08-25 14:46:24 PDT
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 2010-08-25 14:51:16 PDT
(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 [:bz] 2010-08-25 15:03:00 PDT
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 2010-08-25 15:10:02 PDT
(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 [:bz] 2010-08-25 20:38:56 PDT
That seems fine to me.
Comment 12 Boris Zbarsky [:bz] 2010-08-25 20:44:29 PDT
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) 2010-08-26 00:37:21 PDT
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 2010-08-26 11:28:53 PDT
(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 2010-08-26 14:17:38 PDT
Created attachment 469611 [details] [diff] [review]
v1.2

per comments, ready for checkin
Comment 16 Shawn Wilsher :sdwilsh 2010-08-27 14:34:39 PDT
http://hg.mozilla.org/mozilla-central/rev/b6f61845c3dd
Comment 17 Eric Shepherd [:sheppy] 2010-08-30 13:12:31 PDT
Documented:

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

And listed on Firefox 4 for developers.
Comment 18 Eric Shepherd [:sheppy] 2010-08-30 13:14:22 PDT
Also mentioned on the doc page for nsIInputStream.
Comment 19 Shawn Wilsher :sdwilsh 2010-08-30 13:34:49 PDT
(In reply to comment #18)
> Also mentioned on the doc page for nsIInputStream.
Do you mean nsIScriptableInputStream?
Comment 20 Eric Shepherd [:sheppy] 2010-08-30 13:35:36 PDT
Um. Yeah, that's what I mean. Hey, look over there! *pointing*

Note You need to log in before you can comment on or make changes to this bug.