Most of JS_ReportError consumer can be changed to UTF8 variant, so we need to add it before replacing consumers

Lossy variant of conversion functions don't need JSContext, but js::ExclusiveContext only, as it doesn't report error. This is needed to use it in AutoMessageArgs::init.

Currently, error reporting API uses UTF-16 as internal representation, that will be changed in bug 1283710 tho, until then, UTF-8 vairnat needs conversion from UTF-8 to UTF-16. So I'm about to land whole patches at once, to reduce the unnecessary conversion. (in current plan, most error reporting call will use UTF8 variant) Changes: * added UTF8 variants * added ErrorArgumentsType parameter to some functions, that will be finally passed to AutoMessageArgs * renamed populateUncaughtExceptionReport{,VA} to populateUncaughtExceptionReportUTF8{,VA} for clarification * when ArgumentsAreUTF8 is passed, it converts passed string to UTF-161

Comment on attachment 8788622 [details] [diff] [review] Part 1: Make lossy conversion available off main thread. Review of attachment 8788622 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/CharacterEncoding.h @@ +264,3 @@ > > extern TwoByteCharsZ > +LossyUTF8CharsToNewTwoByteCharsZ(js::ExclusiveContext* cx, const ConstUTF8CharsZ& utf8, size_t* outlen); Converting these to take a base class is fine if every user can see the base-class-ness. But ExclusiveContext isn't part of public API, and not everyone can see the inheritance relationship. We can't even forward-declare an inheritance relationship. So making this change will break all public API users of this function -- which may not exist now, but we want to exist in the long run. So this won't work. As for what we *should* do? Uh... We could have the JSContext* API here, and we could have an ExclusiveContext* API in jscntxt.h, in namespace js instead of namespace JS. That would also force most users to qualify their calls to say which they wanted. I guess that's not the worst thing in the world? ::: js/src/vm/CharacterEncoding.cpp @@ +230,2 @@ > { > + JSContext* cx = maybeCx->asJSContext(); These casts seem extremely unideal. Whatever we end up doing for this, I don't think I can stamp anything with this sort of cast in it. Maybe that requires pushing the handling in cases that report, upward slightly into the InflateUTF8StringToBuffer template parameters or so.

Comment on attachment 8788623 [details] [diff] [review] Part 2: Add JS_ReportError*UTF8 variants. Review of attachment 8788623 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +5560,5 @@ > va_list ap; > > AssertHeapIsIdle(cx); > va_start(ap, format); > + ReportErrorVA(cx, JSREPORT_ERROR, format, ArgumentsAreASCII, ap); I think this API needs to be considered to be Latin-1, not ASCII. It might be that most callers pass ASCII in, but I don't think we can assume it like this. Gotta consider each one individually and switch it over to a properly-encoded thing, then get rid of this. @@ +5677,5 @@ > bool ok; > > AssertHeapIsIdle(cx); > va_start(ap, format); > + ok = ReportErrorVA(cx, JSREPORT_WARNING, format, ArgumentsAreASCII, ap); Again, I think this needs to be Latin-1 until we examine every individual caller and convert them to a deliberately-typed thing. ::: js/src/jscntxt.cpp @@ +558,5 @@ > allocatedElements_ = true; > MOZ_ASSERT(charArgLength == js_strlen(args_[i])); > lengths_[i] = charArgLength; > + } else if (typeArg == ArgumentsAreUTF8) { > + char* charArg = va_arg(ap, char*); const char*? @@ +560,5 @@ > lengths_[i] = charArgLength; > + } else if (typeArg == ArgumentsAreUTF8) { > + char* charArg = va_arg(ap, char*); > + size_t charArgLength = strlen(charArg); > + JS::UTF8Chars utf8(charArg, charArgLength); I'd just put the strlen() into the declaration here.

Added ExclusiveContext variant for Lossy functions, in js namespace in jscntxt.h. now helper functions are template of JSContext*/ExclusiveContext*. Added error reporting functions for ExclusiveContext, that does nothing. Also added helper function to call pod_malloc on JSContext*/ExclusiveContext* template parameter. Lossy functions in JS namespace calls functions in js namespace.

Comment on attachment 8789177 [details] [diff] [review] Part 1: Make lossy conversion available off main thread. Review of attachment 8789177 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jscntxt.h @@ +11,5 @@ > > #include "mozilla/MemoryReporting.h" > > #include "js/GCVector.h" > +#include "js/CharacterEncoding.h" Mis-alphabetized, make check-style or whatever will complain. @@ +815,5 @@ > }; > > +/* > + * ExclusiveContext variant of encoding functions, for off main thread use. > + * Please refer CharacterEncoding.h for the details. "variants" and "off-main-thread" and "Refer to CharacterEncoding.h for details." ::: js/src/vm/CharacterEncoding.cpp @@ +280,2 @@ > static bool > +InflateUTF8StringToBuffer(ContextT cx, const UTF8Chars src, CharT* dst, size_t* dstlenp, class ContextT and |ContextT* cx| seems better here. @@ +420,2 @@ > static CharsT > +InflateUTF8StringHelper(ContextT cx, const UTF8Chars src, size_t* outlen) ContextT* cx here as well. @@ +424,5 @@ > *outlen = 0; > > JS::SmallestEncoding encoding; > + if (!InflateUTF8StringToBuffer<Action, CharT, ContextT>( > + cx, src, /* dst = */ nullptr, outlen, &encoding)) ContextT can be inferred from the type of the provided argument, so just do if (!InflateUTF8StringToBuffer<Action, CharT>(cx, src, ...)) @@ +432,2 @@ > > + CharT* dst = AllocateChar<CharT>(cx, *outlen + 1); // +1 for NUL Can't you just do cx->template pod_malloc<CharT>(*outlen + 1)? I *think* that's what the extra template function's trying to get around, but I'm not sure. @@ +441,5 @@ > MOZ_ASSERT(*outlen == srclen); > for (uint32_t i = 0; i < srclen; i++) > dst[i] = CharT(src[i]); > } else { > + MOZ_ALWAYS_TRUE((InflateUTF8StringToBuffer<Copy, CharT, ContextT>( No ContextT here, either.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2d47b727467980a41f31428bde7308b689446f Bug 1295017 - Part 1: Make lossy conversion available off main thread. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/f69bdda3779b11fba984cc21296004bc89edeb49 Bug 1295017 - Part 2: Add JS_ReportError*UTF8 variants. r=jwalden

https://hg.mozilla.org/mozilla-central/rev/4b2d47b72746 https://hg.mozilla.org/mozilla-central/rev/f69bdda3779b

Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.