Add JS_ReportError*UTF8 variants.

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

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

Updated

3 years ago
Depends on: 1289003
Assignee

Updated

3 years ago
Depends on: 1294940
Assignee

Updated

3 years ago
No longer depends on: 1294940
Assignee

Comment 1

3 years ago
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.
Assignee

Comment 2

3 years ago
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
Assignee

Comment 3

3 years ago
Attachment #8788622 - Flags: review?(jwalden+bmo)
Assignee

Comment 4

3 years ago
Attachment #8781393 - Attachment is obsolete: true
Attachment #8781394 - Attachment is obsolete: true
Attachment #8788623 - Flags: review?(jwalden+bmo)
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.
Attachment #8788622 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #8788623 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 7

3 years ago
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.
Attachment #8788622 - Attachment is obsolete: true
Attachment #8789177 - Flags: review?(jwalden+bmo)
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.
Attachment #8789177 - Flags: review?(jwalden+bmo) → review+

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b2d47b72746
https://hg.mozilla.org/mozilla-central/rev/f69bdda3779b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
You need to log in before you can comment on or make changes to this bug.