Let JavaScript read embedded nulls from input streams

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
Networking
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla2.0b5
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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).
Attachment #469145 - Flags: superreview?(cbiesinger)
Attachment #469145 - Flags: review?(bzbarsky)
(Assignee)

Comment 1

7 years ago
This blocks bug 568634, which is a beta 5 blocker, which makes this a beta 5 blocker.
blocking2.0: --- → beta5+
Whiteboard: [needs review bz][needs sr biesi]
(Assignee)

Comment 2

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

Comment 5

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

Comment 6

7 years ago
Created attachment 469180 [details] [diff] [review]
v1.1

Per comments
Attachment #469145 - Attachment is obsolete: true
Attachment #469180 - Flags: superreview?(cbiesinger)
Attachment #469180 - Flags: review?(bzbarsky)
Attachment #469145 - Flags: superreview?(cbiesinger)
Attachment #469145 - Flags: review?(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?
(Assignee)

Comment 8

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

Comment 10

7 years ago
(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.
That seems fine to me.
Comment on attachment 469180 [details] [diff] [review]
v1.1

r=me with that comment added and an apostrophe added in "caller's location".
Attachment #469180 - Flags: review?(bzbarsky) → review+
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);
Attachment #469180 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Updated

7 years ago
Whiteboard: [needs review bz][needs sr biesi] → [needs new patch]
(Assignee)

Comment 14

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

Comment 15

7 years ago
Created attachment 469611 [details] [diff] [review]
v1.2

per comments, ready for checkin
Attachment #469180 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [needs new patch] → [can land]
(Assignee)

Comment 16

7 years ago
http://hg.mozilla.org/mozilla-central/rev/b6f61845c3dd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla2.0b5
(Assignee)

Updated

7 years ago
Keywords: dev-doc-needed
Documented:

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

And listed on Firefox 4 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Also mentioned on the doc page for nsIInputStream.
(Assignee)

Comment 19

7 years ago
(In reply to comment #18)
> Also mentioned on the doc page for nsIInputStream.
Do you mean nsIScriptableInputStream?
Um. Yeah, that's what I mean. Hey, look over there! *pointing*
Depends on: 632609
You need to log in before you can comment on or make changes to this bug.