NetUtil.readInputStreamToString should have aCharset argument as optional

RESOLVED FIXED in mozilla11

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

({dev-doc-complete})

Trunk
mozilla11
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
From Bug 505192 comment #28

(In reply to comment #26)
> > Can you use NetUtil.readInputStreamToString here?
> 
> NetUtil.readInputStreamToString doesn't set character encoding.  It is no
> good when we need character conversion.

We could fix the NetUtil with an optional argument, I'm sure sdwilsh could review that change.
(Assignee)

Updated

6 years ago
Assignee: nobody → m_kato
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 533565 [details] [diff] [review]
fix
Comment on attachment 533565 [details] [diff] [review]
fix

Drive-by review:

>+        if (aCharset) {
>+          let cis = Cc["@mozilla.org/intl/converter-input-stream;1"].
>+                    createInstance(Ci.nsIConverterInputStream);
>+          try {
>+            cis.init(aInputStream, "UTF-8", aCount, 0);

I think you want to use aCharset here instead of "UTF-8". I would also suggest to test at least one other charset in the unit test to ensure the charset is actually configurable.
(Assignee)

Comment 3

6 years ago
Created attachment 535275 [details] [diff] [review]
fix
Attachment #533565 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
Comment on attachment 535275 [details] [diff] [review]
fix

Should I define NS_ERROR_ILLEGAL_INPUT in xpc.msg?
Attachment #535275 - Flags: review?(philipp)
Comment on attachment 535275 [details] [diff] [review]
fix

Thanks for patch! I'm not a toolkit peer, 302ing the review to sdwilsh. That said, here's some feedback:

>+     * @throws NS_ERROR_ILLEGAL_INPUT if aInputStream has invalid sequenses

(typo in "sequences")

>+    do_check_eq(e.result, 0x8050000E); // NS_ERROR_ILLEGAL_INPUT

(In reply to comment #4)
> Should I define NS_ERROR_ILLEGAL_INPUT in xpc.msg?

Yes, that's probably a good idea, assuming this would expose it in Components.results. And then change the above line to use Cr.NS_ERROR_ILLEGAL_INPUT.

>+  do_check_eq(NetUtil.readInputStreamToString(istream,
>+                                              TEST_DATA_UTF8.length,
>+                                              "UTF-8",
>+                                              Ci.nsIConverterInputStream.
>+                                                DEFAULT_REPLACEMENT_CHARACTER),

nit: indention
Attachment #535275 - Flags: review?(sdwilsh)
Attachment #535275 - Flags: review?(philipp)
Attachment #535275 - Flags: feedback+
(Assignee)

Comment 6

6 years ago
Created attachment 535597 [details] [diff] [review]
fix v2
(Assignee)

Updated

6 years ago
Attachment #535275 - Flags: review?(sdwilsh)
(Assignee)

Updated

6 years ago
Attachment #535597 - Flags: review?(sdwilsh)
I'm not sure I like adding two optional arguments here.  What about using an object here for the optional arguments?
That seems fine to me.
Comment on attachment 535597 [details] [diff] [review]
fix v2

Clearing request until comment 7 is addressed.
Attachment #535597 - Flags: review?(sdwilsh)
(Assignee)

Comment 10

6 years ago
(In reply to comment #7)
> I'm not sure I like adding two optional arguments here.  What about using an
> object here for the optional arguments?

Should I define NetUtil_readInputStreamToString(aInputStream, aCount, aOptions) instead of?

aReplacementChar is for replacing some code with this.  On bug 505192, we should trow invalid UTF-8 sequence, but other usages uses replacement character such as http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStartup.js#318.
(In reply to comment #10)
> Should I define NetUtil_readInputStreamToString(aInputStream, aCount, aOptions)
> instead of?
Yes
(Assignee)

Comment 12

6 years ago
Created attachment 538819 [details] [diff] [review]
fix v3
Attachment #535597 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #538819 - Flags: review?(sdwilsh)
Comment on attachment 538819 [details] [diff] [review]
fix v3

Review of attachment 538819 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh with changes address.  You should get sr from bz.

::: netwerk/base/src/NetUtil.jsm
@@ +273,5 @@
>       * @param aInputStream
>       *        The input stream to read from.
>       * @param aCount
>       *        The number of bytes to read from the stream.
> +     * @param aOption [optional]

nit: aOptions

@@ +316,5 @@
> +          let cis = Cc["@mozilla.org/intl/converter-input-stream;1"].
> +                    createInstance(Ci.nsIConverterInputStream);
> +          try {
> +            if (!aOption.replacement) {
> +              // throw NS_ERROR_ILLEGAL_INPUT if invalid sequences

This comment doesn't seem very useful in its current form.  Can you try to rephrase it please?

@@ +325,5 @@
> +            let str = {};
> +            cis.readString(-1, str);
> +            cis.close();
> +            return str.value;
> +          } catch (e) {

nit: local style says the catch starts on the next line after the }

::: netwerk/test/unit/test_NetUtil.js
@@ +573,5 @@
> +    NetUtil.readInputStreamToString(istream,
> +                                    TEST_DATA_UTF8.length,
> +                                    { charset: "UTF-8" });
> +    do_throw("should throw!");
> +  } catch (e) {

nit: catch should be on the line after }

@@ +574,5 @@
> +                                    TEST_DATA_UTF8.length,
> +                                    { charset: "UTF-8" });
> +    do_throw("should throw!");
> +  } catch (e) {
> +    do_check_eq(e.result, Cr.NS_ERROR_ILLEGAL_INPUT);

Can we test that the stack is fixed to the right call site too?
Attachment #538819 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 14

6 years ago
Created attachment 547026 [details] [diff] [review]
v4
(Assignee)

Updated

6 years ago
Attachment #547026 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 15

6 years ago
(In reply to comment #13)
> @@ +574,5 @@
> > +                                    TEST_DATA_UTF8.length,
> > +                                    { charset: "UTF-8" });
> > +    do_throw("should throw!");
> > +  } catch (e) {
> > +    do_check_eq(e.result, Cr.NS_ERROR_ILLEGAL_INPUT);
> 
> Can we test that the stack is fixed to the right call site too?

This exception should throws by uconv.  I believe that stack won't fixed forever.
Comment on attachment 547026 [details] [diff] [review]
v4

>+     * @param aOptions [optional]
>+     *        charset
>+     *        The character encoding of stream data.
>+     *        replacement
>+     *        The character to repelace unknown byte sequences.
>+     *        If unset, it causes an exceptions to be thrown.

s/repelace/replace/ and please indent the descriptions of the options a bit with respect to the option nam.  The existing comment is very hard to read.

>+        if (aOptions && aOptions.charset) {

Shouldn't that test be |aOptions && "charset" in aOptions|?  Or at least do the "in" test before doing the boolean test if you really want to test for nonempty charset.

>+            if (!aOptions.replacement) {
>+              // aOptions.replacement isn't set.
>+              // If input stream has unknown sequences for aOptions.charset,
>+              // throw NS_ERROR_ILLEGAL_INPUT.
>+              aOptions.replacement = 0;

Again, this feels like it should be an "in" test, not a boolean test (e.g. aOptions may already have "replacement: 0").

sr=me with those changes.
Attachment #547026 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

6 years ago
Blocks: 505192
(In reply to Makoto Kato from comment #15)
> This exception should throws by uconv.  I believe that stack won't fixed
> forever.
We can catch it in NetUtil code and then set the stack correctly.
Keywords: dev-doc-needed
Whiteboard: [has reviews, needs new patch]
(Assignee)

Comment 18

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f51b143fad
Whiteboard: [has reviews, needs new patch] → [inbound]
https://hg.mozilla.org/mozilla-central/rev/37f51b143fad
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
Documentation updated:

en/JavaScript_code_modules/NetUtil.jsm#readInputStreamToString()

And mentioned on Firefox 11 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.