Last Comment Bug 655658 - NetUtil.readInputStreamToString should have aCharset argument as optional
: NetUtil.readInputStreamToString should have aCharset argument as optional
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla11
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on:
Blocks: 505192
  Show dependency treegraph
 
Reported: 2011-05-09 00:03 PDT by Makoto Kato [:m_kato]
Modified: 2012-02-10 12:23 PST (History)
10 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (4.50 KB, patch)
2011-05-19 02:06 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Review
fix (6.49 KB, patch)
2011-05-26 00:10 PDT, Makoto Kato [:m_kato]
philipp: feedback+
Details | Diff | Review
fix v2 (8.12 KB, patch)
2011-05-27 03:11 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Review
fix v3 (8.14 KB, patch)
2011-06-13 00:11 PDT, Makoto Kato [:m_kato]
sdwilsh: review+
Details | Diff | Review
v4 (8.43 KB, patch)
2011-07-20 03:53 PDT, Makoto Kato [:m_kato]
bzbarsky: superreview+
Details | Diff | Review

Description Makoto Kato [:m_kato] 2011-05-09 00:03:25 PDT
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.
Comment 1 Makoto Kato [:m_kato] 2011-05-19 02:06:12 PDT
Created attachment 533565 [details] [diff] [review]
fix
Comment 2 Philipp von Weitershausen [:philikon] 2011-05-19 09:08:06 PDT
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.
Comment 3 Makoto Kato [:m_kato] 2011-05-26 00:10:02 PDT
Created attachment 535275 [details] [diff] [review]
fix
Comment 4 Makoto Kato [:m_kato] 2011-05-26 00:11:06 PDT
Comment on attachment 535275 [details] [diff] [review]
fix

Should I define NS_ERROR_ILLEGAL_INPUT in xpc.msg?
Comment 5 Philipp von Weitershausen [:philikon] 2011-05-26 08:37:57 PDT
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
Comment 6 Makoto Kato [:m_kato] 2011-05-27 03:11:29 PDT
Created attachment 535597 [details] [diff] [review]
fix v2
Comment 7 Shawn Wilsher :sdwilsh 2011-05-31 09:43:26 PDT
I'm not sure I like adding two optional arguments here.  What about using an object here for the optional arguments?
Comment 8 Boris Zbarsky [:bz] 2011-05-31 09:57:25 PDT
That seems fine to me.
Comment 9 Shawn Wilsher :sdwilsh 2011-06-01 12:00:35 PDT
Comment on attachment 535597 [details] [diff] [review]
fix v2

Clearing request until comment 7 is addressed.
Comment 10 Makoto Kato [:m_kato] 2011-06-08 03:31:06 PDT
(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.
Comment 11 Shawn Wilsher :sdwilsh 2011-06-11 14:49:00 PDT
(In reply to comment #10)
> Should I define NetUtil_readInputStreamToString(aInputStream, aCount, aOptions)
> instead of?
Yes
Comment 12 Makoto Kato [:m_kato] 2011-06-13 00:11:51 PDT
Created attachment 538819 [details] [diff] [review]
fix v3
Comment 13 Shawn Wilsher :sdwilsh 2011-06-22 21:13:16 PDT
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?
Comment 14 Makoto Kato [:m_kato] 2011-07-20 03:53:06 PDT
Created attachment 547026 [details] [diff] [review]
v4
Comment 15 Makoto Kato [:m_kato] 2011-07-20 03:56:00 PDT
(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 16 Boris Zbarsky [:bz] 2011-07-20 12:32:19 PDT
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.
Comment 17 Shawn Wilsher :sdwilsh 2011-08-07 15:06:54 PDT
(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.
Comment 19 Mounir Lamouri (:mounir) 2011-11-25 02:29:25 PST
https://hg.mozilla.org/mozilla-central/rev/37f51b143fad
Comment 20 Eric Shepherd [:sheppy] 2012-02-10 12:23:35 PST
Documentation updated:

en/JavaScript_code_modules/NetUtil.jsm#readInputStreamToString()

And mentioned on Firefox 11 for developers.

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