I should have noticed that sooner :(

I found some other bugs and spec updates. Morphing the bug.

- Cahnged ToLowerCase to ASCIIToLower. - Added 2 encodings ("iso-8859-8-i", "x-user-defined") to catch up the spec. - Fixed "shift_jis" encoding name. - Removed "x-windows-949" hack because EUC-KR decoder was just an alias of x-windows-949 decoder and TextEncoder does not support non-UTF encodings. - Added testcases verifying above changes.

Improved the test code a bit.

The first argument of encode/decode has a default value until https://www.w3.org/Bugs/Public/show_bug.cgi?id=19646 is fixed.

This patch also fix a bug of js_ErrorToException. js_ErrorToException couldn't create an Exception with a non-ASCII message.

According to the latest spec.

Please flag any other patches that you need reviewed.

Comment on attachment 674832 [details] [diff] [review] Change some exceptions from EncodingError to TypeError Review of attachment 674832 [details] [diff] [review]: ----------------------------------------------------------------- I think bz needs to review this. I can do the TextEncoder/Decoder parts once he has ok'ed the other changes. It seems very unfortunate to me to require a context for throwing type errors. Can't the generated bindings code do that?

TypeError is spec'ed by the Encoding Standard itself, not the WebIDL spec. So I don't think it belongs to the bindings code. Anne, why did you change the error to TypeError?

I think the right way to do this is to have ThrowTypeError on ErrorResult, as this patch does, but have that just set state on the object. Then the generated code should do the actual exception-throwing. That's sort of been the exceptions plan all along.

And in particular, what the codegen does right now looks like this: if (rv.Failed()) { return ThrowMethodFailedWithDetails<true>(cx, rv, "XMLHttpRequest", "send"); } and ThrowMethodFailedWithDetails could do special things in case rv actually had a TypeError thrown on it.

How can I pass around varargs across the function?

Hmm. You want varargs because you want to be able to report the label that was passed in? In this case, I would say just throw NOT_UTF for either failure. But in general we'd need to figure out some way of doing that, yes... One option, I guess, might be for the ErrorResult to have a JSContext*. But I'd really like to keep it as cheap as possible in the non-exceptional cases. :(

Comment on attachment 671648 [details] [diff] [review] patch v2 https://hg.mozilla.org/integration/mozilla-inbound/rev/b448aae5a5d4

Comment on attachment 674828 [details] [diff] [review] Update WebIDL definition https://hg.mozilla.org/integration/mozilla-inbound/rev/05636b372638

To answer comment 15, the idea is to emulate the semantics of a string enum, without writing out the actual enum in Web IDL as it would be very verbose (at least for TextDecoder). If I add a comment to that effect to the spec, would that be sufficient?

https://hg.mozilla.org/mozilla-central/rev/b448aae5a5d4 https://hg.mozilla.org/mozilla-central/rev/05636b372638

> without writing out the actual enum in Web IDL as it would be very verbose Why is that a problem, exactly? But note that in this case the callee only accepts the UTF-8/16 encodings anyway, not all of them....

TextDecoder supports non-UTF encodings while TextEncoder doesn't.

In which case it might actually make sense to have them take different types in the IDL to make this clear, fwiw.

Other problem is, Web IDL enum doesn't support case-insensitive matching and trimming space characters.

Ah. Yes, ok. That last makes it infeasible to use here.

bz, shall I name the argument utfLabel for TextEncoder to indicate it is somewhat different? FWIW, I'm happy to use a different exception here, maybe NotSupportedError? EncodingError just did not seem right as it's not related to any encoding or decoding error, it's just an incorrect argument.

> utfLabel for TextEncoder to indicate it is somewhat different? Might not be a bad idea. I don't think there's a problem with having a TypeError here per se, for what it's worth. But then again, I also think that it doesn't matter that much what sorts of exceptions are thrown. ;)

I changed the function not to depend on varargs.

The only overhead is one pointer-size storage. It will not be even touched unless TypeError is thrown. It should be negligible because ErrorResult is noncopyable.

Comment on attachment 675735 [details] [diff] [review] Implement ErrorResult::ThrowTypeError Why not make mErrorNumber be a dom::ErrNum? >+ MOZ_ASSERT(argCount <= 10); How about doing that in ThrowTypeError? So if argCount there is too large, simply delete message, set mMessage to null, and move on (after asserting). That will make the failure mode "wrong exception thrown" not "out of bounds stack write. Alternately, you could just set argCount = NS_MIN(argCount, 10) or something and ignore extra args (after the assert you have). But also, you could just do: const jschar* args[argCount+1]; and not worry about all this, no? >+#ifdef DEBUG >+ ~ErrorResult() { >+ MOZ_ASSERT_IF(IsTypeError(), !mMessage); >+ } >+#endif There are ErrorResult consumers who don't ever use ThrowMethodFailedWithDetails. In fact, they just swallow the error... It might be safer to just make mMessage an nsAutoPtr.... r=me with that, I guess.

(In reply to Boris Zbarsky (:bz) from comment #34) > Why not make mErrorNumber be a dom::ErrNum? Will do. > >+ MOZ_ASSERT(argCount <= 10); > > How about doing that in ThrowTypeError? So if argCount there is too large, > simply delete message, set mMessage to null, and move on (after asserting). > > That will make the failure mode "wrong exception thrown" not "out of bounds > stack write. It will crash with buffer overrun in js anyway. https://mxr.mozilla.org/mozilla-central/source/js/src/jscntxt.cpp?rev=d1984b76a0b2#886 > Alternately, you could just set argCount = NS_MIN(argCount, 10) or something > and ignore extra args (after the assert you have). We can't change the argCount here (see the js code above). I don't think assertion can actually fail for a reason other than silly typo because argCount is statically embedded in Errors.msg. > But also, you could just do: > > const jschar* args[argCount+1]; > > and not worry about all this, no? MSVC doesn't support C99 variable length arrays :( > >+#ifdef DEBUG > >+ ~ErrorResult() { > >+ MOZ_ASSERT_IF(IsTypeError(), !mMessage); > >+ } > >+#endif > > There are ErrorResult consumers who don't ever use > ThrowMethodFailedWithDetails. In fact, they just swallow the error... Even worse, it will cause memory leak. But no such consumers use NS_ERROR_TYPE_ERR because ThrowTypeError is a newly added method. Also I added assertions to catch such a usage. > It might be safer to just make mMessage an nsAutoPtr.... nsAutoPtr will always initialize mMessage even if no errors are thrown. Is it OK?

> nsAutoPtr will always initialize mMessage even if no errors are thrown. Is it OK? And it will always check mMessage on destruction. If it doesn't matter, I'll gladly replace it with nsAutoPtr.

> We can't change the argCount here (see the js code above). My point is that you can just ignore args after the 10th one. Is that not the case? > MSVC doesn't support C99 variable length arrays :( Even in C++? I was pretty sure we use them elsewhere in the tree.... > Even worse, it will cause memory leak. Well, right. That's the problem. Swallowing the error is intended behavior for those consumers, but leaking is not. > Also I added assertions to catch such a usage. Hmm. OK, fair. Given that, I guess let's go without nsAutoPtr for now. I agree that having the extra init/check is undesirable.

(In reply to Boris Zbarsky (:bz) from comment #37) > > We can't change the argCount here (see the js code above). > > My point is that you can just ignore args after the 10th one. Is that not > the case? OK, I've moved assertion into ThrowTypeError and added |argCount = std::min<uint16_t>(argCount, 10);|. (NS_MIN is going to be obsoleted per bug 786533.) > > MSVC doesn't support C99 variable length arrays :( > > Even in C++? I was pretty sure we use them elsewhere in the tree.... When I changed the code to |const jschar* args[argCount + 1];|, compile failed with the following error: h:/m/mozilla-central/dom/bindings/BindingUtils.cpp(82) : error C2057: expected constant expression h:/m/mozilla-central/dom/bindings/BindingUtils.cpp(82) : error C2466: cannot allocate an array of constant size 0 h:/m/mozilla-central/dom/bindings/BindingUtils.cpp(82) : error C2133: 'args' : unknown size Supporting variable length arrays in C++ is a gcc extension. Even C++11 didn't spec that. I'd be surprised if it is actually used in our XP code. > > Also I added assertions to catch such a usage. > > Hmm. OK, fair. Given that, I guess let's go without nsAutoPtr for now. I > agree that having the extra init/check is undesirable. I've added a comment explaining why a raw pointer is used.

Rebased to tip

Comment on attachment 675729 [details] [diff] [review] Add a JSAPI to make a report with unicode arguments Review of attachment 675729 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +6732,5 @@ > + va_list ap) > +{ > + AssertHeapIsIdle(cx); > + js_ReportErrorNumberVA(cx, JSREPORT_ERROR, errorCallback, userRef, > + errorNumber, JS_FALSE, ap); The JS_FALSE here (which I would ordinarily say should be false, just so you're aware for other patches) is totally unintelligible when read here. :-) These days we would make this an enum ArgumentType { AsciiString, UnicodeString } so that places like this could pass something readable, and more obviously correct or not-correct. Could you file a followup bug to make this change, please? Could be good first bug material, if I don't get irritated enough by this to just go do it myself. :-) ::: js/src/jscntxt.cpp @@ -914,5 @@ > argLengths[i] = js_strlen(reportp->messageArgs[i]); > totalArgsLength += argLengths[i]; > } > - /* NULL-terminate for easy copying. */ > - reportp->messageArgs[i] = NULL; SpiderMonkey's error reporting API, mechanism, and code is a disaster, as I'm sure you've discovered. :-) Which makes me think that here, if you're not going to null-terminate in the case when messageArgs were passed in from the start, this entry is NULL. You should assert this fact as well, so if any user screws it up somehow at some point, we'll quickly discover it. @@ +1072,5 @@ > + void *userRef, const unsigned errorNumber, > + const jschar **args) > +{ > + if (checkReportFlags(cx, &flags)) > + return JS_TRUE; SpiderMonkey doesn't use JS_TRUE or JS_FALSE in new code -- we use true and false directly. We also use bool instead of JSBool, everywhere except in APIs (at least for the moment; SpiderMonkey's transition to being C++ means we might remove JSBool entirely sometime). (There might be one or two cases where we make exceptions, that don't immediately spring to mind; nothing in this function is such a case.) @@ +1073,5 @@ > + const jschar **args) > +{ > + if (checkReportFlags(cx, &flags)) > + return JS_TRUE; > + JSBool warning = JSREPORT_IS_WARNING(flags); bool

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #40) > Comment on attachment 675729 [details] [diff] [review] > Add a JSAPI to make a report with unicode arguments > > Review of attachment 675729 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jsapi.cpp > @@ +6732,5 @@ > > + va_list ap) > > +{ > > + AssertHeapIsIdle(cx); > > + js_ReportErrorNumberVA(cx, JSREPORT_ERROR, errorCallback, userRef, > > + errorNumber, JS_FALSE, ap); > > The JS_FALSE here (which I would ordinarily say should be false, just so > you're aware for other patches) is totally unintelligible when read here. > :-) > > These days we would make this an enum ArgumentType { AsciiString, > UnicodeString } so that places like this could pass something readable, and > more obviously correct or not-correct. Could you file a followup bug to > make this change, please? Could be good first bug material, if I don't get > irritated enough by this to just go do it myself. :-) Filed bug 808286. > ::: js/src/jscntxt.cpp > @@ -914,5 @@ > > argLengths[i] = js_strlen(reportp->messageArgs[i]); > > totalArgsLength += argLengths[i]; > > } > > - /* NULL-terminate for easy copying. */ > > - reportp->messageArgs[i] = NULL; > > SpiderMonkey's error reporting API, mechanism, and code is a disaster, as > I'm sure you've discovered. :-) Which makes me think that here, if you're > not going to null-terminate in the case when messageArgs were passed in from > the start, this entry is NULL. You should assert this fact as well, so if > any user screws it up somehow at some point, we'll quickly discover it. Added an assertion. > @@ +1072,5 @@ > > + void *userRef, const unsigned errorNumber, > > + const jschar **args) > > +{ > > + if (checkReportFlags(cx, &flags)) > > + return JS_TRUE; > > SpiderMonkey doesn't use JS_TRUE or JS_FALSE in new code -- we use true and > false directly. We also use bool instead of JSBool, everywhere except in > APIs (at least for the moment; SpiderMonkey's transition to being C++ means > we might remove JSBool entirely sometime). (There might be one or two cases > where we make exceptions, that don't immediately spring to mind; nothing in > this function is such a case.) Done. > @@ +1073,5 @@ > > + const jschar **args) > > +{ > > + if (checkReportFlags(cx, &flags)) > > + return JS_TRUE; > > + JSBool warning = JSREPORT_IS_WARNING(flags); > > bool Done.

The latest spec doesn't mention useBOM internal flag anymore.

Using -u0 to reduce the patch size.

- Disabled tests for unpaired surrogates because Gecko doesn't comply the spec yet. - Removed "ibm864" from and added "iso-8859-8-i" and "x-user-defined" to the encoding list. The test doesn't catch up the latest spec yet.

The new imported test caused an assertion failure here. https://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.cpp?rev=265427a0694d#150

Interdiff for the reference. https://bugzilla.mozilla.org/attachment.cgi?oldid=676692&action=interdiff&newid=678164&headers=1

Added an assertion to catch the error earlier.

Interdiff https://bugzilla.mozilla.org/attachment.cgi?oldid=677999&action=interdiff&newid=678165&headers=1

Comment on attachment 678164 [details] [diff] [review] Part 4: Implement ErrorResult::ThrowTypeError r=me

Comment on attachment 678112 [details] [diff] [review] Part 6: Remove encoding detection using BOM Review of attachment 678112 [details] [diff] [review]: ----------------------------------------------------------------- r=me assuming that this is what the spec says. That said, I'm a bit surprised that the API doesn't support any type of BOM detection. But I guess this is fine to do for now and if people want BOM detection they can request it added to the spec.

Unbitrotted

https://hg.mozilla.org/integration/mozilla-inbound/rev/c083bb05e3fd https://hg.mozilla.org/integration/mozilla-inbound/rev/71c0fa0964c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/4aeebb3cacfa https://hg.mozilla.org/integration/mozilla-inbound/rev/6c91d0bc259e https://hg.mozilla.org/integration/mozilla-inbound/rev/3788795c4a18 https://hg.mozilla.org/integration/mozilla-inbound/rev/39bf83f930d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/870d9bf8dc88

https://hg.mozilla.org/mozilla-central/rev/c083bb05e3fd https://hg.mozilla.org/mozilla-central/rev/71c0fa0964c3 https://hg.mozilla.org/mozilla-central/rev/4aeebb3cacfa https://hg.mozilla.org/mozilla-central/rev/6c91d0bc259e https://hg.mozilla.org/mozilla-central/rev/3788795c4a18 https://hg.mozilla.org/mozilla-central/rev/39bf83f930d3 https://hg.mozilla.org/mozilla-central/rev/870d9bf8dc88

I've created: https://developer.mozilla.org/en-US/docs/Web/API/TextDecoder https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder with the spec used here. A note in https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/19 has been added too.