Closed Bug 1125766 Opened 9 years ago Closed 9 years ago

new TextEncoder('xxx') throws TypeError not RangeError in JavaScript

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jxck, Assigned: diorahman, Mentored)

References

Details

(Keywords: dev-doc-complete, testcase, Whiteboard: [lang=c++])

Attachments

(1 file, 11 obsolete files)

59.75 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150125030202

Steps to reproduce:

in firefox nightly (at 2015/1/26)
run a JavaScript code below in developer console.
'xxx' is every string which is unsupported encoding name.

```
new TextEncoder('xxx');
```


Actual results:

throws TypeError. trace is below

```
// TypeError: The given encoding 'xxx' is not supported.
```


Expected results:

in Encoding Standard spec says that
this case should throw RangeError not TypeError.

https://encoding.spec.whatwg.org/#dom-textencoder

```
2. If encoding is failure, or is none of utf-8, utf-16be, and utf-16le, throw a RangeError. 
```
This was a somewhat recent spec change.  In particular, see https://github.com/whatwg/encoding/commit/811cf733af9aba3bcccc3d82d68298d375caac48 from this last Nov 17.  Before that the spec said to throw TypeError.

I'm happy to mentor someone who wants to work on this; it'll require changing JSAPI to allow producing RangeErrors as desired and adding a ThrowRangeError on ErrorResult similar to the existing ThrowTypeError it has.  This is a good chance to learn more about binding error-reporting codepaths and the JSAPI for someone inclined to do so.  ;)

That's all assuming the spec doesn't plan to flipflop, of course.
Mentor: bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=c++]
Hi Boris,

I would love to work on this. Could you help me out? Thanks!
Is there any other affected tests besides the dom/test_TextEncoder.html?

Thanks!
Attachment #8554957 - Flags: review?(bzbarsky)
No other tests right now, it seems
Comment on attachment 8554957 [details] [diff] [review]
invalid-input-on-text-encoder-ctr-should-throw-range-error.patch

Isn't this losing the message passed to ThrowRangeError?  Did you look at the actual .message of the resulting exceptions?

I suspect you need something like ErrorResult::ReportTypeError but ReportRangeError.  Or maybe give it a more generic name, since it's not actually typerror-specific.  Similarly, factoring out the identical code for ThrowRangeError and ThrowRangeError (into a helper that just takes the mResult value) might be a good idea.

You also need to change all the places that check IsTypeError() and clean up mMessage to check for IsRangeError too, or something.
Attachment #8554957 - Flags: review?(bzbarsky) → review-
Assignee: nobody → diorahman
Hi Boris,

Currently, I got it to spit the desired error message. I also added a test inside the test_TextEncoder to check if the exception has the right message.

However, I'm not really sure especially on some points as follows:

1. The current `ErrorFormatString` is defined to have `JSEXN_TYPEERR` as its type, I feel bad when I have to copy its declaration to `RangeErrorFormatString`. Is there any proper way to "switch" the error type? (making a function is ok?)
2. I tried to take out the common instructions of ThrowTypeError and ThrowRangeError, but it seems not too much.
3. I believe the TextDecoder has the same issue with TextEncoder, it should also throw RangeError? (should I put it in another issue or include it in this patch?)

Thanks
Attachment #8554957 - Attachment is obsolete: true
> Is there any proper way to "switch" the error type?

Yeah.  The proper way, now that we have DOM errors that are not typeerror, is to move the error type into the MSG_DEF bit in Errors.msg.  Compare how the MSG_DEF macros in js/src/js.msg look, for example.  It's a bit of typing up front to get the JSEXN_TYPEERR into all the existing MSG_DEF lines, but then we'll have the flexibility we need.

> 2. I tried to take out the common instructions of ThrowTypeError and ThrowRangeError, but
> it seems not too much.

In this last patch, all the management around IsJSException(), setting up mMessage->mArgs, etc, is still duplicated, no?  Doesn't seem to me like it needs to be.  The only bits that need to be separate are the va_list/va_start/va_end; all the other logic can be shared.  Especially if you don't need to preserve the MOZ_ASSERT message distinction between the two (which you don't; the stack will make it clear which codepath reached the assert).

> I believe the TextDecoder has the same issue with TextEncoder,

Correct.  Just fixing it here should be fine, since I believe it should be about one line of code once the basic bits are in place.

Thanks for working on this!
Attached patch bug-1125766-fix.patch (obsolete) — Splinter Review
Attachment #8556996 - Attachment is obsolete: true
Attachment #8557437 - Flags: review?(bzbarsky)
Comment on attachment 8557437 [details] [diff] [review]
bug-1125766-fix.patch

I'm going to look more carefully on Monday, but one obvious remaining issue is that passing *va_arg(ap, nsString*) is wrong when there are multiple arguments.  Each va_arg call returns a new pointer, for the cases when it happens in a loop...

What you want to do is to pass a va_list down to SetThrowErrorMessage and continue to do va_arg calls in there.
Attached patch bug-1125766-fix.patch (obsolete) — Splinter Review
Attachment #8557437 - Attachment is obsolete: true
Attachment #8557437 - Flags: review?(bzbarsky)
Attachment #8557630 - Flags: review?(bzbarsky)
Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1127450 can be close if you fix everything in this bug.
Attached patch bug-1125766-1127450-fix.patch (obsolete) — Splinter Review
In addition to throwing RangeError at the constructors of TextEncoder and TextDecoder, this patch also put the TextDecoder.encode() to throw TypeError instead of EncodingError. (as reported in https://bugzilla.mozilla.org/show_bug.cgi?id=1127450). Is it still relevant? or Should I create a separate patch for this?

The related tests are also updated. However I couldn't manage dom/encoding/test/reftest/bug863728-1.html to pass the reftest test(the build without this patch can't pass this test either).
Attachment #8557680 - Flags: feedback?(crimsteam)
Comment on attachment 8557680 [details] [diff] [review]
bug-1125766-1127450-fix.patch

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

Please obsolete the previous patch and transfer review flags when you attach a new patch.

::: dom/bindings/BindingUtils.cpp
@@ +132,2 @@
>      MOZ_ASSERT(false,
> +               "Ignoring ThrowError call because we have a JS exception");

"Ignoring SetThrowErrorMessage call ..."

::: dom/bindings/ErrorResult.h
@@ +58,5 @@
>  
>    void Throw(nsresult rv) {
>      MOZ_ASSERT(NS_FAILED(rv), "Please don't try throwing success");
>      MOZ_ASSERT(rv != NS_ERROR_TYPE_ERR, "Use ThrowTypeError()");
> +    MOZ_ASSERT(!IsErrorWithMessage(), "Don't overwite TypeError");

"Don't overwite errors with a message"

@@ +66,5 @@
>      MOZ_ASSERT(!IsNotEnoughArgsError(), "Don't overwrite not enough args error");
>      mResult = rv;
>    }
>  
> +  void ThrowError(const dom::ErrNum errorNumber, ...);

Where is the definition of this function?

@@ +74,2 @@
>    void ClearMessage();
> +  bool IsErrorWithMessage() const { return ErrorCode() == NS_ERROR_TYPE_ERR || ErrorCode() == NS_ERROR_RANGE_ERR; }

Probably you should rename NS_ERROR_TYPE_ERROR to NS_ERROR_WITH_MESSAGE rather than adding a new error code for each JS exception type.
It's a bit ridiculous to remember that what message requires what function.

@@ +124,5 @@
>    // Throw() here because people can easily pass success codes to
>    // this.
>    void operator=(nsresult rv) {
>      MOZ_ASSERT(rv != NS_ERROR_TYPE_ERR, "Use ThrowTypeError()");
> +    MOZ_ASSERT(!IsErrorWithMessage(), "Don't overwite TypeError");

"Don't overwite errors with a message"
Attachment #8557630 - Attachment is obsolete: true
Attachment #8557630 - Flags: review?(bzbarsky)
Comment on attachment 8557680 [details] [diff] [review]
bug-1125766-1127450-fix.patch

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

::: dom/bindings/BindingUtils.cpp
@@ +132,2 @@
>      MOZ_ASSERT(false,
> +               "Ignoring ThrowError call because we have a JS exception");

Thanks I won't miss this again.

::: dom/bindings/ErrorResult.h
@@ +58,5 @@
>  
>    void Throw(nsresult rv) {
>      MOZ_ASSERT(NS_FAILED(rv), "Please don't try throwing success");
>      MOZ_ASSERT(rv != NS_ERROR_TYPE_ERR, "Use ThrowTypeError()");
> +    MOZ_ASSERT(!IsErrorWithMessage(), "Don't overwite TypeError");

Thanks I won't miss this again.

@@ +66,5 @@
>      MOZ_ASSERT(!IsNotEnoughArgsError(), "Don't overwrite not enough args error");
>      mResult = rv;
>    }
>  
> +  void ThrowError(const dom::ErrNum errorNumber, ...);

Oops. I should remove this. Thanks.

@@ +74,2 @@
>    void ClearMessage();
> +  bool IsErrorWithMessage() const { return ErrorCode() == NS_ERROR_TYPE_ERR || ErrorCode() == NS_ERROR_RANGE_ERR; }

Interesting. Should I add a new definition to dom/base/domerr.msg?

@@ +124,5 @@
>    // Throw() here because people can easily pass success codes to
>    // this.
>    void operator=(nsresult rv) {
>      MOZ_ASSERT(rv != NS_ERROR_TYPE_ERR, "Use ThrowTypeError()");
> +    MOZ_ASSERT(!IsErrorWithMessage(), "Don't overwite TypeError");

Thanks I won't miss this again.
Hmm.  So that's a good point: tying the error type to the message does mean that the choice of "message string" influences the type of error thrown in a slightly non-obvious way.  Sadly, that's what JSAPI gives us to work with.

I'd really like to avoid scope-creep in this bug, though.  You can't rename NS_ERROR_TYPE_ERR without touching a whole bunch of code that uses it to produce DOMExceptions with name == TypeError or something.  And I really do prefer the explicit indication of what sort of error is being thrown.

So for now I think it's fine to keep the two separate nsresult values there, file a followup bug to decide what we actually want here, and maybe assert in ThrowTypeError/ThrowRangeError that the message corresponds to the right error type.
Though I guess I'd also be ok with just creating a new NS_DOM_ERROR_WITH_MESSAGE (but leaving NS_ERROR_TYPE_ERROR as-is) and using it here.
Also, comment 12 was not all that helpful.  This patch really is getting big enough without the scope creep...

I just talked to the SpiderMonkey folks, and they want to move away from having the message id control the type of exception.  So we should not be combining into a single API for throwing both TypeError and RangeError; we want to keep both ThrowTypeError and ThrowRangeError.

Given that, I think having the two separate nsresults is ok for now.  Once the JS folks figure out how their API is changing, we can figure out what that means for ErrorResult.
Comment on attachment 8557680 [details] [diff] [review]
bug-1125766-1127450-fix.patch

>+++ b/dom/bindings/BindingUtils.cpp
>+ErrorResult::SetThrowErrorMessage(va_list ap, const dom::ErrNum errorNumber, const nsresult errorType)

How about making this ThrowErrorWithMessage?  

>+ErrorResult::ReportError(JSContext* aCx)

ReportErrorWithMessage, please.

>+++ b/dom/bindings/ErrorResult.h
>+  void ThrowError(const dom::ErrNum errorNumber, ...);

This is unused, right?

>+  void SetThrowErrorMessage(va_list ap, const dom::ErrNum errorNumber, const nsresult errorType = NS_ERROR_TYPE_ERR);

Please make the third argument non-optional.

>- ** Macros for checking results
>+ ** Macros for checking results;

Why this change?

>+++ b/dom/encoding/TextDecoder.cpp
>+  // (https://encoding.spec.whatwg.org/#dom-textencoder).

Clearly not the right link.  Should be textdecoder.

>+        aRv.ThrowTypeError(MSG_DOM_ENCODING_ERROR);

That's an odd name for a decoding error.  Maybe MSG_DOM_DECODING_FAILED or something?  And change the string to "Decoding failed." perhaps.

Also, you should be able to remove NS_ERROR_DOM_ENCODING_NOT_UTF_ERR and NS_ERROR_DOM_ENCODING_DECODE_ERR from both ErrorList.h and domerr.msg, right?

>+++ b/dom/encoding/test/test_TextDecoder.js
>+  var message = "The given encoding '_encoding_' is not supported.";
>+  function errorMessage(encoding) {
>+    encoding = encoding + '';
>+    return message.split('_encoding_').join(encoding.trim());
>+  }

How about:

  function errorMessage(encoding) {
    return `The given encoding '${String(encoding).trim()}' is not supported.`;
  }

?

>+  encoding = "   ";
>+  testSingleString({encoding: "   ", input: aData, error: "RangeError",

{ encoding: encoding }, no?  Otherwise you have to duplicate the value...

Same for a bunch more of these.

Similar comments in dom/encoding/test/test_TextEncoder.js.

r=me with that and the assertion strings fixed per comment 14.  Thank you for sticking with this!
Attachment #8557680 - Flags: review+
Attached patch bug-1125766-1127450-fix.patch (obsolete) — Splinter Review
Hi Boris,

I hope I can carry the r+ to this patch and ask for your help to check the changes I've made to accommodate your notes in comment 19.

Thanks for mentoring me!
Attachment #8558251 - Flags: review+
Attachment #8558251 - Flags: checkin?(bzbarsky)
Attachment #8557680 - Attachment is obsolete: true
Attachment #8557680 - Flags: feedback?(crimsteam)
Try push, with some minor changes to fix whitespace nits and such, and merged to build on tip: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7e4c6ad53f9
(In reply to Boris Zbarsky [:bz] from comment #21)
> Try push, with some minor changes to fix whitespace nits and such, and
> merged to build on tip:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7e4c6ad53f9

hey bz, the try push seems very broken like crashes in  PROCESS-CRASH | file:///C:/slave/test/build/tests/reftest/tests/gfx/tests/crashtests/358732-2.svg | application crashed [@ mozilla::ErrorResult::Throw(nsresult)]  - is this fixed now in the current checkin request ?
Flags: needinfo?(bzbarsky)
Because there are already NS_ERROR_RANGE_ERR consumers without any messages:
https://mxr.mozilla.org/mozilla-central/search?string=NS_ERROR_RANGE_ERR
Do we have to clear mMessage in Throw()?
(In reply to Masatoshi Kimura [:emk] from comment #23)
> Because there are already NS_ERROR_RANGE_ERR consumers without any messages:
> https://mxr.mozilla.org/mozilla-central/search?string=NS_ERROR_RANGE_ERR
> Do we have to clear mMessage in Throw()?

Then the function body of IsErrorWithMessage() could be just |return mMessage != nullptr;| and ThrowRangeError() and ThrowTypeError() could be changed to |ThrowWithMessage(nsresult, ...);|.
(In reply to Masatoshi Kimura [:emk] from comment #24)
> (In reply to Masatoshi Kimura [:emk] from comment #23)
> > Because there are already NS_ERROR_RANGE_ERR consumers without any messages:
> > https://mxr.mozilla.org/mozilla-central/search?string=NS_ERROR_RANGE_ERR
> > Do we have to clear mMessage in Throw()?
> 
> Then the function body of IsErrorWithMessage() could be just |return
> mMessage != nullptr;| and ThrowRangeError() and ThrowTypeError() could be
> changed to |ThrowWithMessage(nsresult, ...);|.

Or we could just provide suitable messages for all the current consumers using NS_ERROR_RANGE_ERR?
Attachment #8558251 - Flags: checkin?(bzbarsky)
> is this fixed now in the current checkin request

No, it's not.  That's why I didn't add the checkin-needed keyword to the bug...

So yes, the issue is that we have current places that throw DOMExceptions with an NS_ERROR_RANGE_ERR.  There is no corresponding DOMException code, so I think the right thing to do is in fact comment 25: give them sane messages.  Luckily, there are just a few places that do that...

Dhi, are you willing to do that?  I'll post the exact patch I had pushed to try so you can work on top of that.

Alternately, we could file a followup for this part and do comment 24 for now; up to you.
Flags: needinfo?(bzbarsky)
Attached patch What I pushed to try (obsolete) — Splinter Review
Flags: needinfo?(diorahman)
Oh, and looking at the other test failures, the only thing not caused by comment 23 is that the xpcshell test toolkit/components/osfile/tests/xpcshell/test_exception.js expects encoding stuff to throw TypeError.  Needs to be adjusted to expect RangeError.
(In reply to Boris Zbarsky [:bz] from comment #26)
> > is this fixed now in the current checkin request
> 
> No, it's not.  That's why I didn't add the checkin-needed keyword to the
> bug...
> 
> So yes, the issue is that we have current places that throw DOMExceptions
> with an NS_ERROR_RANGE_ERR.  There is no corresponding DOMException code, so
> I think the right thing to do is in fact comment 25: give them sane
> messages.  Luckily, there are just a few places that do that...
> 
> Dhi, are you willing to do that?  I'll post the exact patch I had pushed to
> try so you can work on top of that.
> 
> Alternately, we could file a followup for this part and do comment 24 for
> now; up to you.

Yes, I'll try to work on it.
Flags: needinfo?(diorahman)
(In reply to Boris Zbarsky [:bz] from comment #28)
> Oh, and looking at the other test failures, the only thing not caused by
> comment 23 is that the xpcshell test
> toolkit/components/osfile/tests/xpcshell/test_exception.js expects encoding
> stuff to throw TypeError.  Needs to be adjusted to expect RangeError.

With the patch pushed, for some reason the thrown exception is not EINVAL https://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/tests/xpcshell/test_exception.js#33 I'm not sure but I will check it again.
Flags: needinfo?(bzbarsky)
The EINVAL comes from someone calling OS.File.Error.invalidArgument().  Which looks like it's supposed to happen in toolkit/components/osfile/modules/osfile_shared_front.jsm in AbstractFile.read if constructing the TextDecoder fails, but it's conditioned on the exception being a TypeError, not RangeError.  So need to fix that bit.
Flags: needinfo?(bzbarsky)
It contains what Boris has pushed to try-server, plus implementation of comment 25 and comment 28 (with clue from comment 31)
Attachment #8558251 - Attachment is obsolete: true
Attachment #8558961 - Flags: review?(bzbarsky)
Updated with commit message
Attachment #8558961 - Attachment is obsolete: true
Attachment #8558961 - Flags: review?(bzbarsky)
Attachment #8559085 - Flags: review?(bzbarsky)
Comment on attachment 8559085 [details] [diff] [review]
bug-1125766-1127450-fix.patch updated

>+MSG_DEF(MSG_INVALID_RESPONSE_STATUSCODE_ERROR, 0, JSEXN_RANGEERR, "Invalid response status code.")

It might be good to have a separate string for use in Response::Redirect which says "Invalid redirect status code".

Apart from that this looks fine as far as it goes, but why does toolkit/components/osfile/tests/xpcshell/test_exception.js pass with this patch?  Why is the "encoding: 4" bit still triggering a TypeError?  Is it not getting passed down to the encoding API bits or something?

In any case, r=me with the better message for bogus redirect status codes.
Attachment #8559085 - Flags: review?(bzbarsky) → review+
I hope I could carry over the r+.
Attachment #8559085 - Attachment is obsolete: true
Attachment #8559473 - Flags: review+
(In reply to Boris Zbarsky [:bz] from comment #34)
> Comment on attachment 8559085 [details] [diff] [review]
> bug-1125766-1127450-fix.patch updated
> 
> >+MSG_DEF(MSG_INVALID_RESPONSE_STATUSCODE_ERROR, 0, JSEXN_RANGEERR, "Invalid response status code.")
> 
> It might be good to have a separate string for use in Response::Redirect
> which says "Invalid redirect status code".

I have uploaded the updated patch

> 
> Apart from that this looks fine as far as it goes, but why does
> toolkit/components/osfile/tests/xpcshell/test_exception.js pass with this
> patch?  Why is the "encoding: 4" bit still triggering a TypeError?  Is it
> not getting passed down to the encoding API bits or something?

There is a sanity check on types of options. In case of encoding, the `read` function wants `typeof options.encoding` to be `"string"` (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_native.jsm#43), other than that it returns new TypeError("Invalid type for option encoding");

> 
> In any case, r=me with the better message for bogus redirect status codes.
> I hope I could carry over the r+.

Yep.  Also, I did look at the interdiff and it looks fine.

I did another try push of the next-to-last patch here (back when it was the last one). The results are at https://treeherder.mozilla.org/#/jobs?repo=try&revision=97680662ba41 and there are a bunch of TEST-UNEXPECTED-PASS in part 2 of web-platform-tests.  They were there on the previous try push too, but sorta covered up by all the other noise.  Do you want to put up a new patch removing the relevant failure annotations?  I can do it if you'd prefer...  Just let me know.
Attachment #8558552 - Attachment is obsolete: true
I decided to try it out. Hopefully it will be fine. I'm sorry for not being careful on reading the error logs after the patch is pushed to try-server.

One question: I found that the testing/web-platform/tests/encoding/textdecoder-fatal-streaming.html has a test case when the {fatal: true}, it throws TypeError. is that the expected behavior?
Flags: needinfo?(bzbarsky)
Attachment #8559546 - Flags: review?(bzbarsky)
Attachment #8559473 - Attachment is obsolete: true
Comment on attachment 8559546 [details] [diff] [review]
patch with web-platform-tests tweaks

I think that's the expected behavior, yes.  That's https://encoding.spec.whatwg.org/#dom-textdecoder-decode step 5 substep 3 subsubstep 3, I believe.

r=me; I pushed this to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94c873b8b7f3
Flags: needinfo?(bzbarsky)
Attachment #8559546 - Flags: review?(bzbarsky) → review+
I feel really bad. The last push to try server still failed. Missed to remove the annotation of web-platform-tests of textencoder-fatal.html.
Attachment #8559546 - Attachment is obsolete: true
Attachment #8559597 - Flags: review?(bzbarsky)
Comment on attachment 8559597 [details] [diff] [review]
remove textdecoder-fatal.html.ini

No need to feel bad.  This is what tryserver is for.  ;)

r=me, new try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7929ffdc788
Attachment #8559597 - Flags: review?(bzbarsky) → review+
Dhi, thank you for sticking with this!
https://hg.mozilla.org/mozilla-central/rev/53767c427617
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1130801
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.