1485066 - Replace JSAutoByteString with UniqueChars

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

We could change JS_EncodeString[ToUTF8] to return UniqueChars and then simply replace all uses of JSAutoByteString with UniqueChars.

Comment 1

[André Bargull [:anba]]

Attachment: bug1485066-part1-remove-jsautobytestring.patch

Changes JS_EncodeString[ToUTF8] to return UniqueChars and replaces all uses of JSAutoByteString with UniqueChars.

Part 2 will perform additional clean-up after this more or less mechanical change.


Notes:
- Rust bindings don't seem to like UniqueChars (or mozilla::UniquePtr in general), so I simply created a wrapper around JS_EncodeStringToUTF8 in js/rust/src/jsglue.cpp. I haven't found out if Rust's |CStr::from_ptr| actually takes care of freeing the memory, so maybe there's a memory leak, but that's a pre-existing issue.
- js/src/vm/SavedStacks.cpp: I've changed |BuildUTF8StackString| to directly return |UniqueChars| instead of |UTF8CharsZ| to avoid converting between |UniqueChars| and |UTF8CharsZ| forth and back. (|BuildUTF8StackString| is only called from <https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/src/jsapi.cpp#5110-5111,5121-5122>, which already works using |UniqueChars|.)

Comment 2

[André Bargull [:anba]]

Attachment: bug1485066-part2-cleanup-out-param.patch

- Replaces UniqueChars out-params with UniqueChars return-values
- Simplify some lines now that UniqueChars are used instead of JSAutoByteString

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 9002833 [details] [diff] [review]
bug1485066-part1-remove-jsautobytestring.patch

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

::: js/public/CharacterEncoding.h
@@ +345,5 @@
> + * contains any nulls.  Avoid using this function if possible, because it will
> + * eventually be removed.
> + */
> +extern JS_PUBLIC_API(JS::UniqueChars)
> +JS_EncodeString(JSContext* cx, JSString* str);

How bad would it be, in a followup, to global-rename to JS_EncodeStringToLatin1?  Make even clearer the awful.

::: js/rust/tests/callback.rs
@@ +59,2 @@
>      let message = CStr::from_ptr(message);
>      println!("{}", str::from_utf8(message.to_bytes()).unwrap());

This ownership model is entirely wrong, CStr::from_ptr just adopts so this leaks as you guessed it might, please file a followup bug to fix this 'cause this is terrible on steroids.  :-(

::: js/src/builtin/TestingFunctions.cpp
@@ +3036,2 @@
>          scope.emplace(JS::StructuredCloneScope::DifferentProcess);
> +    else if (strcmp(scopeStr.get(), "DifferentProcessForIndexedDB") == 0)

Rather than preserve bad API usage, could you ensureLinear and StringEqualsAscii?

@@ +3069,2 @@
>                  // default
> +            } else if (strcmp(poli.get(), "deny") == 0) {

Also linearize and StringEqualsAscii.

@@ +4230,5 @@
>  
>      JSString* str = JS::ToString(cx, v);
>      if (!str)
>          return false;
> +    UniqueChars action = JS_EncodeString(cx, str);

Linearize and StringEqualsAscii below.

@@ +4244,5 @@
>          } else {
>              JSString* str = JS::ToString(cx, v);
>              if (!str)
>                  return false;
> +            UniqueChars phasesStr = JS_EncodeString(cx, str);

Linearize and StringEqualsAscii.

::: js/src/builtin/intl/Collator.cpp
@@ +199,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>      MOZ_ASSERT(args.length() == 1);
>      MOZ_ASSERT(args[0].isString());
>  
> +    UniqueChars locale = JS_EncodeString(cx, args[0].toString());

I think we could use the UTF-8 one here, but it's not really important.  I guess my druthers would do the same std::all_of-style thing as in that other bug and then do the UTF-8 version, but eh.

@@ +276,5 @@
>          return nullptr;
>  
>      if (!GetProperty(cx, internals, internals, cx->names().locale, &value))
>          return nullptr;
> +    UniqueChars locale = JS_EncodeString(cx, value.toString());

Would use same helper her if we had it.

::: js/src/builtin/intl/DateTimeFormat.cpp
@@ +563,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>      MOZ_ASSERT(args.length() == 4);
>      MOZ_ASSERT(args[0].isString());
>  
> +    UniqueChars locale = JS_EncodeString(cx, args[0].toString());

Yeah, there are like a zillion locale-to-chars conversions that need that helper.  Definite need for the followup for that.

::: js/src/builtin/intl/IntlObject.cpp
@@ +49,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>      MOZ_ASSERT(args.length() == 1);
>  
> +    UniqueChars locale = JS_EncodeString(cx, args[0].toString());

This also could use that helper I mentioned somewhere else.  Maybe we really *should* add it as a helper, in the common-functions file, or in the header as an inline so the verification step can be in an #ifdef DEBUG.

@@ +371,5 @@
>  
>      // 1. Assert: locale is a string.
>      RootedString str(cx, args[0].toString());
> +    UniqueChars locale = JS_EncodeStringToUTF8(cx, str);
> +    if (!locale)

Nice, we do UTF-8 assuming here, unlike all the other locale-conversions I've seen so far.  More call for a helper!  I guess now let's upgrade that to a firm patch-to-followup, either this bug or another?

::: js/src/ctypes/CTypes.cpp
@@ +1680,5 @@
>                                 HandleObject funObj, HandleObject typeObj,
>                                 bool isVariadic)
>  {
>    AutoString funSource;
> +  JS::UniqueChars funBytes;

My review comment on the entirety of the changes in this file:

https://www.youtube.com/watch?v=ncGHiVKJh0Y

😂🔫 (to be clear this is a real gun, not a fake gun as the bowdlerizers want it to be these days, and it is pointing at me)

::: js/src/jsapi-tests/tests.h
@@ +109,1 @@
>              if (!!bytes)

if (JS::UniqueChars bytes = JS_EncodeString(cx, str))

@@ +237,1 @@
>                  if (!!bytes)

Ditto.

::: js/src/shell/js.cpp
@@ +1491,5 @@
> +        UniqueChars opt = JS_EncodeStringToUTF8(cx, str);
> +        if (!opt)
> +            return false;
> +
> +        if (strcmp(opt.get(), "strict") == 0) {

ensureLinear and StringEqualsAscii for all this?  (And then EncodeUtf8 in the error case.)

@@ +4484,1 @@
>                  JS_ReportErrorASCII(cx, "Unknown value for option `format`, expected 'multipart' or 'simple', got %s", ValueToPrintableUTF8(cx, optionFormat, &printable));

Yeesh this looks as stupid now as it did yesterday.

@@ +5142,1 @@
>          if (!arg || !argv.append(std::move(arg)))

Yuck this plus the |argv.back()| direct strcmp against a string literal is safe and correct but still horrible-looking.

::: js/src/vm/Debugger.cpp
@@ +4564,5 @@
>      }
>  
>      template <typename T>
>      MOZ_MUST_USE bool commonFilter(T script, const JS::AutoRequireNoGC& nogc) {
> +        if (urlCString.get()) {

Shouldn't need the .get() here.

::: js/xpconnect/src/Sandbox.cpp
@@ +853,5 @@
>              return false;
>          }
>          RootedString nameStr(cx, nameValue.toString());
> +        JS::UniqueChars name = JS_EncodeStringToUTF8(cx, nameStr);
> +        if (!name)

JS_FlattenString and JS_FlatStringEqualsAscii for all this.

@@ +915,4 @@
>              rtcIdentityProvider = true;
>  #endif
>          } else {
> +            JS_ReportErrorUTF8(cx, "Unknown property name: %s", name.get());

And then this you'll have to do JS_EncodeStringToUtf8, I guess.  Better to sequester that to the final error case, tho, IMO.

::: js/xpconnect/src/XPCComponents.cpp
@@ +251,3 @@
>  
>      // we only allow interfaces by name here
> +    if (name && name.get()[0] != '{') {

s/\.get\(\)//

@@ +627,5 @@
> +        return NS_OK;
> +
> +    JS::UniqueChars name = JS_EncodeString(cx, JSID_TO_STRING(id));
> +    if (name &&
> +        name.get()[0] != '{') { // we only allow contractids here

s/\.get\(\)//

@@ +837,3 @@
>      RootedString str(cx, JSID_TO_STRING(id));
> +    JS::UniqueChars name = JS_EncodeString(cx, str);
> +    if (name && name.get()[0] == '{' &&

Ditto.  (Gonna stop mentioning these in this file.)

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +308,5 @@
> +                if (!iface2)
> +                    break;
> +
> +                to = wrapperToReflectInterfaceNames->FindTearOff(iface2, true, &rv);
> +                if (to == nullptr)

While I kinda prefer comparisons against nullptr, normal style is probably !to.

@@ +312,5 @@
> +                if (to == nullptr)
> +                    break;
> +
> +                jso = to->GetJSObject();
> +                if (jso == nullptr)

Ditto.

::: tools/fuzzing/messagemanager/MessageManagerFuzzer.cpp
@@ +200,3 @@
>      MSGMGR_FUZZER_LOG("%*s! Mutated value of type |string|: '%s' to '%s'",
>                        aRecursionCounter * 4, "",
> +                      valueChars, x.get());

That's a macro, it's not obvious exactly what it does, pass valueChars.get() to it.

@@ +264,3 @@
>      MSGMGR_FUZZER_LOG("Mutated '%s' Message: %s",
>                        NS_ConvertUTF16toUTF8(aMessageName).get(),
> +                      strChars.get());

...in the same manner as was done here.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 9002835 [details] [diff] [review]
bug1485066-part2-cleanup-out-param.patch

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

::: js/src/ctypes/CTypes.cpp
@@ +1479,4 @@
>    }
>  
>    JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr,
> +                             CTYPESMSG_EMPTY_FIN, (posStr ? posStr.get() : ""));

Usually we don't parenthesize this stuff, but all this code is kinda trash anyway, sooooooooooo...

::: js/src/shell/js.cpp
@@ +4480,5 @@
>                  useMultipart = false;
>              } else {
> +                UniqueChars printable = ValueToPrintableUTF8(cx, optionFormat);
> +                JS_ReportErrorASCII(cx, "Unknown value for option `format`, expected 'multipart' or 'simple', got %s",
> +                                    printable.get());

Still Dumb.

::: js/src/vm/EnvironmentObject.cpp
@@ +1414,5 @@
>  static void
>  ReportOptimizedOut(JSContext* cx, HandleId id)
>  {
> +    UniqueChars printable = ValueToPrintableLatin1(cx, IdToValue(id));
> +    if (printable) {

Could combine as noted in other file.

::: js/src/vm/Interpreter.cpp
@@ +5289,5 @@
>  {
>      MOZ_ASSERT(errorNumber == JSMSG_UNINITIALIZED_LEXICAL ||
>                 errorNumber == JSMSG_BAD_CONST_ASSIGN);
> +    UniqueChars printable = ValueToPrintableLatin1(cx, IdToValue(id));
> +    if (printable)

Could

  if (UniqueChars printable = ...)

::: js/src/vm/StringType.cpp
@@ +2089,5 @@
>      else
>          str = ToString<CanGC>(cx, v);
>      if (!str)
>          return nullptr;
> +    return JS_EncodeStringToUTF8(cx, RootedString(cx, str));

Pull the RootedString into a local variable.  (I thought we'd written JS::Rooted in such way as to forbid this...)

Comment 5

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] from comment #3)
> ::: js/src/builtin/TestingFunctions.cpp
> @@ +3036,2 @@
> >          scope.emplace(JS::StructuredCloneScope::DifferentProcess);
> > +    else if (strcmp(scopeStr.get(), "DifferentProcessForIndexedDB") == 0)
> 
> Rather than preserve bad API usage, could you ensureLinear and
> StringEqualsAscii?

I had planned to this in a different bug (and already prepared the patch), but since you insist on it, I'll gladly post the various clean-up bugs for review here, too. }:-)


> ::: js/src/builtin/intl/Collator.cpp
> @@ +199,5 @@
> >      CallArgs args = CallArgsFromVp(argc, vp);
> >      MOZ_ASSERT(args.length() == 1);
> >      MOZ_ASSERT(args[0].isString());
> >  
> > +    UniqueChars locale = JS_EncodeString(cx, args[0].toString());
> 
> I think we could use the UTF-8 one here, but it's not really important.  I
> guess my druthers would do the same std::all_of-style thing as in that other
> bug and then do the UTF-8 version, but eh.

I'll start with creating a simple "EncodeLocale" helper in intl/CommonFunctions with std::all_of, but keep calling EncodeLatin1 in the helper, because that's faster for strings guaranteed to contain only ASCII characters (only a mem_cpy) compared to the character-by-character processing for EncodeUTF8. (I was even wondering if it makes sense to add a "Encode-String-which-contains-only-ASCII-and-no-NUL-or-else-assert" helper function. Maybe later when it's more clear where this kind of helper makes sense, apart from the use case for the Intl objects.)


> ::: js/src/ctypes/CTypes.cpp
> @@ +1680,5 @@
> >                                 HandleObject funObj, HandleObject typeObj,
> >                                 bool isVariadic)
> >  {
> >    AutoString funSource;
> > +  JS::UniqueChars funBytes;
> 
> My review comment on the entirety of the changes in this file:
> 
> https://www.youtube.com/watch?v=ncGHiVKJh0Y
> 
> 😂🔫 (to be clear this is a real gun, not a fake gun as the bowdlerizers
> want it to be these days, and it is pointing at me)

Best file ever. Nobody will notice that it's barely maintained. 


> ::: js/xpconnect/src/Sandbox.cpp
> @@ +853,5 @@
> >              return false;
> >          }
> >          RootedString nameStr(cx, nameValue.toString());
> > +        JS::UniqueChars name = JS_EncodeStringToUTF8(cx, nameStr);
> > +        if (!name)
> 
> JS_FlattenString and JS_FlatStringEqualsAscii for all this.

Oh nice. I knew about StringEqualsAscii, but didn't know there's also a public function for the same purpose.

> ::: tools/fuzzing/messagemanager/MessageManagerFuzzer.cpp
> @@ +200,3 @@
> >      MSGMGR_FUZZER_LOG("%*s! Mutated value of type |string|: '%s' to '%s'",
> >                        aRecursionCounter * 4, "",
> > +                      valueChars, x.get());
> 
> That's a macro, it's not obvious exactly what it does, pass valueChars.get()
> to it.

Thanks! I didn't notice that's a macro call.

Comment 6

[André Bargull [:anba]]

Attachment: bug1485066-part1-remove-jsautobytestring.patch

Rebased and applied review comments (except for the ones which will appear in later patches). Carrying r+.

Comment 7

[André Bargull [:anba]]

Attachment: bug1485066-part2-cleanup-out-param.patch

Rebased and applied review comments. Carrying r+.

Comment 8

[André Bargull [:anba]]

Attachment: bug1485066-part3-remove-encodestring-for-comparison.patch

Replace JS_EncodeString with StringEqualsAscii for string comparisons. 


Drive-by fix:
Remove https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/js/src/shell/js.cpp#1492 because it's no longer necessary to root the string in the |args| storage.

Comment 9

[André Bargull [:anba]]

Attachment: bug1485066-part4-remove-AtomToPrintableString-outparam.patch

Same as part 2, but only spotted it after part 2 was already reviewed.

Comment 10

[André Bargull [:anba]]

Attachment: bug1485066-part5-use-non-jsapi-encoding-internally.patch

Switch engine internal callers of JS_EncodeString to EncodeLatin1 resp. JS_EncodeStringToUTF8 to StringToNewUTF8CharsZ (which has a totally confusing name given that it returns UniqueChars and not |UTF8CharsZ| as the name kind of implies).

And implements the helper for the Intl objects as suggested in the review comments for part 1.

Comment 11

[André Bargull [:anba]]

Attachment: bug1485066-part6-quote-string-roundtrips.patch

QuoteString(...) [1] creates internally |char*| and then returns a JSString containing these |char*|. Callers of QuoteString(...) then called JS_EncodeString to transform the JSString back to |char*|. So all in all not really efficient. 

This patch changes QuoteString to directly return |char*| (wrapped in UniqueChars), so the callers can now more efficiently decide how to process the quoted string contents.

Also changed the |quote| parameter from |char16_t| to |char|, so it's more clear that only |char| inputs are valid for the quote character. (And made it an optional parameter, which reads nicer IMO than stray |0|s when calling QuoteString.)

And finally removed [2], because |Sprinter| nowadays always holds a non-null char storage (cf. bug 302531 which added this code).


[1] https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/js/src/vm/Printer.h#218
[2] https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/js/src/vm/Printer.cpp#354-361

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 9003894 [details] [diff] [review]
bug1485066-part3-remove-encodestring-for-comparison.patch

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

::: js/xpconnect/src/Sandbox.cpp
@@ +857,2 @@
>              return false;
> +        if (JS_FlatStringEqualsAscii(nameStr, "Blob")) {

Yuck this is awful.  :-)  I wonder if there's some way we could clean this up sometime with a nicer API that lets you switch on possible strings somehow.

Comment 13

[André Bargull [:anba]]

Attachment: bug1485066-part7-misc-cleanup.patch

Various things to clean-up and to fix I spotted while working on this bug.

- https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/js/src/builtin/TypedObject.cpp#1703-1708 exactly matches ValueToPrintableUTF8, so replace it with a call to ValueToPrintableUTF8.

- https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/js/src/shell/js.cpp#2344 should call JS_NewStringCopyUTF8N, because |ShellContext::readLineBuf| holds UTF-8 encoded characters.

- https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/js/src/shell/js.cpp#2531 change to UTF-8 encoding, so that the |assertEq| shell function can cope with non Latin-1 inputs. (For example |assertEq(0, 1, "\u0141")| now correctly displays "Assertion failed: got 0, expected 1: Ł" instead of "Assertion failed: got 0, expected 1: A".

- Change |js::DecompileArgument| to directly return |JSString*| to avoid lossy Latin-1 conversions. 

- Use UTF-8 encoding in js::ReportInNotObjectError, so that the error message for  |"\u0100" in "\u0101"| is "cannot use 'in' operator to search for 'Ā' in 'ā'" instead of "cannot use 'in' operator to search for '' in ''".

- Add an ASCII assertion to js::Throw(...) to ensure the optional details parameter contains only ASCII characters.

- Change |0| to |'\0'| in vm/Printer.cpp for the NUL character, so there's a clearer distinction when characters and when number values are used.

Comment 14

[André Bargull [:anba]]

Attachment: bug1485066-part8-rename-to-EncodeStringToLatin1.patch

And rename JS_EncodeString to JS_EncodeStringToLatin1.

Comment 15

[André Bargull [:anba]]

Attachment: bug1485066-part9-more-misc-cleanup.patch

More clean-ups, some in preparation for the next patch:

Change parameters to use handle in preparation for the next patch in:
- js/public/Proxy.h
- js/src/proxy/Proxy.cpp
- js/src/proxy/ScriptedProxyHandler.cpp
- js/src/vm/JSObject.h, js::Throw
- js/src/vm/JSObject.cpp, js::Throw

Use existing handles instead of rooting values again in:
- js/src/builtin/Object.cpp
- js/src/vm/Debugger.cpp
- js/src/vm/JSObject.h, js::ReportNotObject, js::NonNullObject
- js/src/vm/JSObject.cpp, js::ReportNotObject

Move code into single callers in:
- js/src/builtin/TypedObject.cpp

Root the option string once and handle OOM in:
- js/src/shell/js.cpp

Pass the jsid instead of a JSString in preparation for the next patch in:
- js/src/vm/NativeObject.cpp

Reduce code duplication by sharing the code paths for Int32 and String values in:
- js/src/vm/SelfHosting.cpp

Comment 16

[André Bargull [:anba]]

Attachment: bug1485066-part10-remove-ValueToPrintable.patch

Remove ValueToPrintableLatin1 and ValueToPrintableUTF8 and add instead IdToPrintableUTF8.

There are a few different patterns in the code base to transform a jsid into a printable UTF-8 (or Latin-1) character sequence, but in the end they all perform exactly the same actions, so it makes sense to create a single helper for this task.

The four patterns were:
1. Invoke IdToValue and ValueToPrintableUTF8.
2. Invoke IdToValue, ValueToSource, and StringToNewUTF8CharsZ.
3. Invoke IdToValue, ValueToSource, create a AutoStableStringChars, and then retrieve the AutoStableStringChars' Two-Byte characters.
4. Invoke IdToValue and ReportValueError (with JSDVG_IGNORE_STACK).

But when looking into them, they are all equivalent, and boil down into pattern 2.

With the new helper IdToPrintableUTF8, there was one remaining caller to ValueToPrintableUTF8 in shell/js.cpp, which can be inlined to directly call JS_EncodeStringToUTF8. And there was also one caller left for ValueToPrintableLatin1 in vm/BytecodeUtil.cpp, which also got inlined. After that ValueToPrintableLatin1 and ValueToPrintableUTF8 were removed.

Comment 17

[André Bargull [:anba]]

Attachment: bug1485066-part11-FormatStackDump-cleanup.patch

Removes the unused |UniqueChars| argument from FormatStackDump and updates FormatStackDump to use |Sprinter| instead of calling JS_sprintf_append.

I don't know the reason behind this check [1] and [2] doesn't provide any additional info. But the check is definitely not sufficient to match the new function syntaxes with generator functions, async functions, methods etc., so I've replaced it with a call to |IsCallable|.

[3] also no longer applies, because we use |Maybe<AutoRealm>| multiple times in tree. (And I also don't actually know why it's necessary to enter the objects realm when only calling ToString...)

[1] https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/js/src/jsfriendapi.cpp#836-838
[2] https://searchfox.org/mozilla-central/diff/cb324c279e1fd70f4d046e191c4583d2bce00263/js/src/xpconnect/src/xpcdebug.cpp#46
[3] https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/js/src/jsfriendapi.cpp#818-822

Comment 18

[André Bargull [:anba]]

Attachment: bug1485066-part12-utf8-for-decompiler.patch

Update the decompiler to output UTF-8.

|\u0141 = 0; [].forEach(\u0141);| now correctly displays:

  typein:1:16 TypeError: Ł is not a function

instead of:

  typein:1:16 TypeError: A is not a function

Comment 19

[André Bargull [:anba]]

Attachment: bug1485066-part13-reporterror-ascii.patch

Change JS_ReportErrorNumberXXX callers to use JS_ReportErrorNumberASCII when the input is definitely ASCII.

Comment 20

[André Bargull [:anba]]

Attachment: bug1485066-part14-utf8-for-FormatStackDump.patch

And update JS::FormatStackDump to output UTF-8.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 9003906 [details] [diff] [review]
bug1485066-part7-misc-cleanup.patch

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

::: js/src/shell/js.cpp
@@ +2360,5 @@
>          return true;
>      }
>  
>      if (args.length() == 1) {
> +        sc->readLineBuf.reset();

Would prefer = nullptr.

::: js/src/vm/Printer.cpp
@@ +351,5 @@
>      return true;
>  }
>  
>  bool
> +QuoteString(Sprinter* sp, JSString* str, char quote/* = '\0'*/)

Space after |quote| is more common and more readable.

@@ +364,5 @@
>             : QuoteString(sp, linear->twoByteRange(nogc), quote);
>  }
>  
>  UniqueChars
> +QuoteString(JSContext* cx, JSString* str, char quote/* = '\0'*/)

Ditto.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 9004251 [details] [diff] [review]
bug1485066-part9-more-misc-cleanup.patch

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

::: js/src/builtin/Object.cpp
@@ +1057,1 @@
>          UniqueChars bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, v, nullptr);

Could just pass args[0] directly.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 9004261 [details] [diff] [review]
bug1485066-part11-FormatStackDump-cleanup.patch

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

::: js/src/jsfriendapi.cpp
@@ +1075,2 @@
>          else
> +            ok = FormatWasmFrame(cx, i, sp, num);

Seems like

  bool ok = i.hasScript()
            ? FormatFrame(...)
            : FormatWasmFrame(...);
  if (!ok)
    ...

is better.

@@ +1079,5 @@
>          num++;
>      }
>  
> +    if (num == 0 && !sp.put("JavaScript stack is empty\n"))
> +        return nullptr;

Modern style is to have fallible goo nested inside non-error-related condition-testing, like

  if (num == 0) {
    if (!sp.put("..."))
      return nullptr;
  }

@@ +1084,2 @@
>  
> +    UniqueChars copy = DuplicateString(sp.string());

Too lazy to check, but does |sp.release()| work here, after prior patching here (or maybe even this patch)?  I think that also eliminates the need for a ROOM goo.

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 9004256 [details] [diff] [review]
bug1485066-part10-remove-ValueToPrintable.patch

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

This is a good simplification.

::: js/src/shell/js.cpp
@@ +4758,5 @@
>                  useMultipart = true;
>              } else if (StringEqualsAscii(linearFormat, "simple")) {
>                  useMultipart = false;
>              } else {
> +                UniqueChars printable = JS_EncodeStringToUTF8(cx, linearFormat);

Need an if-guard around this, since it can fail.

::: js/src/vm/StringType.cpp
@@ +2067,4 @@
>  {
> +    // ToString(<symbol>) throws a TypeError, therefore require that callers
> +    // request source representation when |id| is a symbol value.
> +    MOZ_ASSERT_IF(JSID_IS_SYMBOL(id), asSource);

Mm.  A little fast-and-loose, but okay.

::: js/src/vm/StringType.h
@@ +1682,3 @@
>   */
>  extern UniqueChars
> +IdToPrintableUTF8(JSContext* cx, HandleId id, bool asSource = false);

Could you add/use an enum for this?  Maybe

  enum IdToPrintableBehavior { MightBeSymbol, NeverSymbol };

or something.  Literal true/false are hard for the reader to understand as correct, nor divine any semantics to them.

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 9003907 [details] [diff] [review]
bug1485066-part8-rename-to-EncodeStringToLatin1.patch

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

::: dom/bindings/CallbackInterface.cpp
@@ +22,5 @@
>    }
>    if (!aCallable.isObject() ||
>        !JS::IsCallable(&aCallable.toObject())) {
>      JS::UniqueChars propName =
> +      JS_EncodeStringToLatin1(cx, JS_FORGET_STRING_FLATNESS(JSID_TO_FLAT_STRING(aPropId)));

Faithful translation, but it's wrong as to semantics, looks like.

ThrowErrorMessage per searchfox hits

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/bindings/CallbackInterface.cpp#28
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/bindings/ErrorResult.h#78
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/bindings/BindingUtils.cpp#123

which tails into JS_ReportErrorNumberUTF8VA.  Change this to the UTF-8 version, and ideally get some DOM person to get a necessarily Gecko-specific testcase in place for this.

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +80,5 @@
>      for (unsigned i = 0; i < args.length(); i++) {
>          JSString *str = JS::ToString(cx, args[i]);
>          if (!str)
>              return false;
> +        JS::UniqueChars bytes = JS_EncodeStringToLatin1(cx, str);

Consistent with my previous opinions noted in bug 987069, I would feel pretty good about assuming text sent to stdout being UTF-8.  File a followup to change this to UTF-8?

@@ +116,5 @@
>  
>      JSString *str = JS::ToString(cx, args[0]);
>      if (!str)
>          return false;
> +    JS::UniqueChars bytes = JS_EncodeStringToLatin1(cx, str);

Make the above-requested followup cover both these functions.

::: js/src/ctypes/Library.cpp
@@ +177,2 @@
>        if (pathCharsLatin1)
>          JS_ReportErrorLatin1(cx, "couldn't open library %s: %s", pathCharsLatin1.get(), error);

Does this get fixed in a future patch that I probably have already reviewed because this patch was going to be Slower To Review probably?

::: js/src/jsapi-tests/tests.h
@@ +230,5 @@
>              JS::RootedValue v(cx);
>              JS_GetPendingException(cx, &v);
>              JS_ClearPendingException(cx);
>              JSString* s = JS::ToString(cx, v);
>              if (s) {

Could combine these two into one while you're in the area.

::: js/src/jsnum.cpp
@@ +796,5 @@
>      /*
>       * Create the string, move back to bytes to make string twiddling
>       * a bit easier and so we can insert platform charset seperators.
>       */
> +    UniqueChars numBytes = JS_EncodeStringToLatin1(cx, str);

This could be either UTF-8 or Latin-1 because it's just digits and signs and 'e'/'E' and '.' or 'Infinity' or 'NaN', right?  Given UTF-8 is where we want to go eventually, I'd prefer if you used that.

Or maybe better, add a JS_EncodeAsciiString that delegates to JS_EncodeStringToLatin1 but also asserts the incoming string is pure ASCII.  Fine with me if this is a followup patch.

::: js/xpconnect/src/XPCConvert.cpp
@@ +1367,5 @@
>                  return NS_ERROR_FAILURE;
>  
>              return ConstructException(NS_ERROR_XPC_JS_THREW_JS_OBJECT,
>                                        strBytes.get(), ifaceName, methodName,
>                                        nullptr, exceptn, cx, s.address());

It appears that the message-string passed to ConstructException should be interpreted as UTF-8.

https://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#1195 (if |errorObject|, then the message is overwritten with UTF-8 data)
https://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_mozilla%3A%3Adom%3A%3AException%3E_2&redirect=false (message stored in Exception::mMessage is used at cursory skim in at least three places as UTF-8)

So this looks like it can change to ToUTF8, and below.  Or maybe better, put that additional change in a followup bug some XPConnect suc^H^H^Hreviewer can look at.

::: js/xpconnect/src/XPCThrower.cpp
@@ +166,5 @@
>          jsid id = ccx.GetMember()->GetName();
>          const char* name;
>          JS::UniqueChars bytes;
>          if (!JSID_IS_VOID(id)) {
> +            bytes = JS_EncodeStringToLatin1(ccx, JSID_TO_STRING(id));

This flows into |sz| which can flow into |dom::Throw| which (as I think I mentioned in another comment in this review) funnels into Exception into Exception::mMessage which other code treats as UTF-8, so this should be encoding to UTF-8.

::: tools/fuzzing/messagemanager/MessageManagerFuzzer.cpp
@@ +195,5 @@
>        return false;
>      }
>      JSString* str = JS_NewStringCopyZ(aCx, x.get());
>      aOutMutationValue.set(JS::StringValue(str));
> +    JS::UniqueChars valueChars = JS_EncodeStringToLatin1(aCx, aValue.toString());

Given this goes to stderr, IMO it too could be UTF-8.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 9004262 [details] [diff] [review]
bug1485066-part12-utf8-for-decompiler.patch

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

ȤỐᶭʛ!

::: js/src/builtin/Object.cpp
@@ +1058,5 @@
>          if (!bytes)
>              return false;
>  
> +        JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_UNEXPECTED_TYPE, bytes.get(),
> +                                 "not an object or null");

It deviates from usual style, but I find this stuff's more readable with parameters on a separate line from the message-number.

::: js/src/vm/JSObject.cpp
@@ +228,5 @@
>          UniqueChars bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, v, nullptr);
>          if (!bytes)
>              return false;
> +        JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_UNEXPECTED_TYPE, bytes.get(),
> +                                 "not an object");

Same params-on-separate-line as elsewhere.

::: js/src/vm/SelfHosting.cpp
@@ +305,5 @@
>              return;
>      }
>  
> +    JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, errorNumber, errorArgs[0].get(),
> +                             errorArgs[1].get(), errorArgs[2].get());

Params-separate-line.

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 9004263 [details] [diff] [review]
bug1485066-part13-reporterror-ascii.patch

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

::: js/src/vm/Interpreter.cpp
@@ +1852,5 @@
>          return;
>      }
>  
> +    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_IN_NOT_OBJECT,
> +                              InformalValueTypeName(rref));

I'm not entirely sure that we have a documented requirement that JSClass::name be ASCII and not Latin-1.  I wouldn't make this change, or at least not without making this requirement clearly documented.

Then again, we have other places that make this assumption, e.g. https://searchfox.org/mozilla-central/rev/05d91d3e02a0780f44599371005591d7988e2809/js/src/jsapi.cpp#201 so I guess this would be okay even without any change.

Why does our documentation story suck?  :-(

Comment 28

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] from comment #25)
> ::: dom/bindings/CallbackInterface.cpp
> [...]
> Change this to the UTF-8 version, and ideally get some DOM person
> to get a necessarily Gecko-specific testcase in place for this.

Hmm, CallbackInterface::GetCallableProperty(...) is only called from dom/bindings/Codegen.py generated code, which probably means the |aPropId| parameter can only be non-ASCII if a non-ASCII name is used in the IDL file? I have no idea if this is actually allowed resp. supported by the IDL bindings code. (The in-tree .idl and .webidl files only use non-ASCII characters in comments and strings.)


> ::: js/src/ctypes/Library.cpp
> @@ +177,2 @@
> >        if (pathCharsLatin1)
> >          JS_ReportErrorLatin1(cx, "couldn't open library %s: %s", pathCharsLatin1.get(), error);
> 
> Does this get fixed in a future patch that I probably have already reviewed
> because this patch was going to be Slower To Review probably?
> 

This file isn't changed in later patches. What needs to be fixed here?


> ::: js/src/jsnum.cpp
> @@ +796,5 @@
> >      /*
> >       * Create the string, move back to bytes to make string twiddling
> >       * a bit easier and so we can insert platform charset seperators.
> >       */
> > +    UniqueChars numBytes = JS_EncodeStringToLatin1(cx, str);
> 
> This could be either UTF-8 or Latin-1 because it's just digits and signs and
> 'e'/'E' and '.' or 'Infinity' or 'NaN', right?  Given UTF-8 is where we want
> to go eventually, I'd prefer if you used that.
> 
> Or maybe better, add a JS_EncodeAsciiString that delegates to
> JS_EncodeStringToLatin1 but also asserts the incoming string is pure ASCII. 
> Fine with me if this is a followup patch.
> 

I'll create a follow-up bug to add JS_EncodeAsciiString.

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 9003907 [details] [diff] [review]
bug1485066-part8-rename-to-EncodeStringToLatin1.patch

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

::: dom/bindings/CallbackInterface.cpp
@@ +22,5 @@
>    }
>    if (!aCallable.isObject() ||
>        !JS::IsCallable(&aCallable.toObject())) {
>      JS::UniqueChars propName =
> +      JS_EncodeStringToLatin1(cx, JS_FORGET_STRING_FLATNESS(JSID_TO_FLAT_STRING(aPropId)));

So if

> Hmm, CallbackInterface::GetCallableProperty(...) is only called from
> dom/bindings/Codegen.py generated code, which probably means the |aPropId|
> parameter can only be non-ASCII if a non-ASCII name is used in the IDL
> file? I have no idea if this is actually allowed resp. supported by the
> IDL bindings code. (The in-tree .idl and .webidl files only use non-ASCII
> characters in comments and strings.)

is true about this only being triggerable by .webidl containing non-ASCII property names, maybe we could make Codegen.py assert ASCII and then assert it here too?

::: js/src/ctypes/Library.cpp
@@ +177,2 @@
>        if (pathCharsLatin1)
>          JS_ReportErrorLatin1(cx, "couldn't open library %s: %s", pathCharsLatin1.get(), error);

I'm not sure what I meant here.  Maybe to combine the declaration with the if?

Comment 30

[André Bargull [:anba]]

Attachment: bug1485066-part1-remove-jsautobytestring.patch

Rebased part 1, carrying r+.

Comment 31

[André Bargull [:anba]]

Attachment: bug1485066-part2-cleanup-out-param.patch

Rebased part 2, carrying r+.

Comment 32

[André Bargull [:anba]]

Attachment: bug1485066-part3-remove-encodestring-for-comparison.patch

Rebased part 3, carrying r+.

Comment 33

[André Bargull [:anba]]

Attachment: bug1485066-part4-remove-AtomToPrintableString-outparam.patch

Rebased part 4, carrying r+.

Comment 34

[André Bargull [:anba]]

Attachment: bug1485066-part5-use-non-jsapi-encoding-internally.patch

Rebased part 5, carrying r+.

Comment 35

[André Bargull [:anba]]

Attachment: bug1485066-part6-quote-string-roundtrips.patch

Rebased part 6, carrying r+.

Comment 36

[André Bargull [:anba]]

Attachment: bug1485066-part7-misc-cleanup.patch

Updated part 7 per review comments, carrying r+.

Comment 37

[André Bargull [:anba]]

Attachment: bug1485066-part8-rename-to-EncodeStringToLatin1.patch

Updated part 8 per review comments, carrying r+. The XPConnect and Codegen related review comments will be addressed in separate bugs.

Comment 38

[André Bargull [:anba]]

Attachment: bug1485066-part9-more-misc-cleanup.patch

Updated part 9 per review comments, carrying r+.

Comment 39

[André Bargull [:anba]]

Attachment: bug1485066-part10-remove-ValueToPrintable.patch

Updated part 10 per review comments, carrying r+.

Comment 40

[André Bargull [:anba]]

Attachment: bug1485066-part11-FormatStackDump-cleanup.patch

Updated part 11 per review comments, carrying r+.

Comment 41

[André Bargull [:anba]]

Attachment: bug1485066-part12-utf8-for-decompiler.patch

Updated part 12 per review comments, carrying r+.

Comment 42

[André Bargull [:anba]]

Attachment: bug1485066-part13-reporterror-ascii.patch

Rebase part 13 for good measure, carrying r+.

Comment 43

[André Bargull [:anba]]

Attachment: bug1485066-part14-utf8-for-FormatStackDump.patch

And part 14, too.

Comment 44

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c90fc82e74ccc875f9bb2449534a3cab3a555c01

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d35bb63dbc1d
Part 1: Remove JSAutoByteString. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/c22fc17c9d87
Part 2: Use UniqueChars as return-type in functions previously using JSAutoByteString out-param. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f89260d5e30
Part 3: Replace calls to JS_EncodeString for string comparison with StringEqualsAscii. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf167b0a3af3
Part 4: Replace AtomToPrintableString UniqueChars out-param with UniqueChars return-type. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/8defc9eabfac
Part 5: Use js::EncodeLatin1/StringToNewUTF8CharsZ instead of JS_EncodeString[ToUTF8] for engine internal code. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/b08b0cfc1dbe
Part 6: Reduce char-string-char roundtrips when quoting strings. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/28e7e61c11ec
Part 7: Miscellaneous clean-up for engine internal EncodeString callers. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/6676e8fedcb3
Part 8: Rename JS_EncodeString to JS_EncodeStringToLatin1. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/09b5382e0baf
Part 9: Additional miscellaneous clean-ups around EncodeString callers. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cbc0ae0117a
Part 10: Remove ValueToPrintable{Latin1,UTF8}, add IdToPrintableUTF8. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/d50fcf82556c
Part 11: Remove the unused |inBuf| argument from JS::FormatStackDump and change it to use Sprinter. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/939e27aa2d59
Part 12: Change the decompiler to return UTF-8 encoded strings. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/f09bc4d5fdcc
Part 13: Use JS_ReportErrorNumberASCII when the error arguments are guaranteed to be ASCII characters. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/e40f67f15bf1
Part 14: Change FormatStackDump to return UTF-8 encoded strings. r=Waldo

Comment 46

[Cosmin Sabou [:CosminS]]

Backed out for build bustages on MessageManagerFuzzer.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=busted&fromchange=e40f67f15bf12802b07690301ad9a4eeedcb2b14&tochange=3d23c2f43b8a5ccd1dd21f1240689cea1566deed&selectedJob=197595516

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=197595516&repo=mozilla-inbound&lineNumber=29406

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d23c2f43b8a5ccd1dd21f1240689cea1566deed

[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/tools/fuzzing/messagemanager'
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -target x86_64-apple-darwin11 -B /builds/worker/workspace/build/src/cctools/bin -isysroot /builds/worker/workspace/build/src/MacOSX10.11.sdk -o MessageManagerFuzzer.o -c  -fvisibility=hidden -fvisibility-inlines-hidden -DNDEBUG=1 -DTRIMMED=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/tools/fuzzing/messagemanager -I/builds/worker/workspace/build/src/obj-firefox/tools/fuzzing/messagemanager -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -fno-common -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fsanitize=address -U_FORTIFY_SOURCE -fno-common -fno-exceptions -fno-strict-aliasing -stdlib=libc++ -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O2 -fno-omit-frame-pointer -Werror  -MD -MP -MF .deps/MessageManagerFuzzer.o.pp   /builds/worker/workspace/build/src/tools/fuzzing/messagemanager/MessageManagerFuzzer.cpp
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -  /builds/worker/workspace/build/src/tools/fuzzing/messagemanager/MessageManagerFuzzer.cpp:199:34: error: no matching function for call to 'JS_EncodeStringToUTF8'
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -      JS::UniqueChars valueChars = JS_EncodeStringToUTF8(aCx, aValue.toString());
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -                                   ^~~~~~~~~~~~~~~~~~~~~
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/js/CharacterEncoding.h:369:1: note: candidate function not viable: cannot convert argument of incomplete type 'JSString *' to 'JS::Handle<JSString *>' for 2nd argument
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -  JS_EncodeStringToUTF8(JSContext* cx, JS::Handle<JSString*> str);
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -  ^
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -  1 error generated.
[task 2018-09-05T12:46:06.306Z] 12:46:06     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1110: recipe for target 'MessageManagerFuzzer.o' failed
[task 2018-09-05T12:46:06.307Z] 12:46:06     INFO -  make[4]: *** [MessageManagerFuzzer.o] Error 1
[task 2018-09-05T12:46:06.307Z] 12:46:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/tools/fuzzing/messagemanager'
[task 2018-09-05T12:46:06.307Z] 12:46:06     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'tools/fuzzing/messagemanager/target' failed
[task 2018-09-05T12:46:06.307Z] 12:46:06     INFO -  make[3]: *** [tools/fuzzing/messagemanager/target] Error 2
[task 2018-09-05T12:46:06.307Z] 12:46:06     INFO -  make[3]: *** Waiting for unfinished jobs....

Comment 47

[André Bargull [:anba]]

Attachment: bug1485066-part8-rename-to-EncodeStringToLatin1.patch

JS_EncodeStringToLatin1 accepts |JSString*|, but JS_EncodeStringToUTF8 needs HandleString, therefore we need to root the input when changing the caller from JS_EncodeStringToLatin1 to JS_EncodeStringToUTF8.

Comment 48

[Cosmin Sabou [:CosminS]]

Hi! Andre, now the patches fail to apply with this error:
applying bug1485066-part11-FormatStackDump-cleanup.patch
patching file js/src/jsfriendapi.cpp
Hunk #2 FAILED at 803
Hunk #3 FAILED at 882
Hunk #4 FAILED at 924
3 out of 4 hunks FAILED -- saving rejects to file js/src/jsfriendapi.cpp.rej
The contents of the .rej file you can find it here: https://pastebin.com/td2fcKY2

Comment 49

[André Bargull [:anba]]

(In reply to Cosmin Sabou [:CosminS] from comment #48)
> Hi! Andre, now the patches fail to apply with this error:
> applying bug1485066-part11-FormatStackDump-cleanup.patch
> patching file js/src/jsfriendapi.cpp
> Hunk #2 FAILED at 803
> Hunk #3 FAILED at 882
> Hunk #4 FAILED at 924
> 3 out of 4 hunks FAILED -- saving rejects to file js/src/jsfriendapi.cpp.rej
> The contents of the .rej file you can find it here:
> https://pastebin.com/td2fcKY2

It looks like the parts were not applied in the correct order. Maybe part 8 wasn't applied, because it's now listed at the bottom?

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52ae4d84b11e
Part 1: Remove JSAutoByteString. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/775159907c73
Part 2: Use UniqueChars as return-type in functions previously using JSAutoByteString out-param. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc4218166b8
Part 3: Replace calls to JS_EncodeString for string comparison with StringEqualsAscii. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f546769e86c
Part 4: Replace AtomToPrintableString UniqueChars out-param with UniqueChars return-type. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f57108d94f
Part 5: Use js::EncodeLatin1/StringToNewUTF8CharsZ instead of JS_EncodeString[ToUTF8] for engine internal code. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/21181aea32fa
Part 6: Reduce char-string-char roundtrips when quoting strings. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/b19d4e73e016
Part 7: Miscellaneous clean-up for engine internal EncodeString callers. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5767f1b04c
Part 8: Rename JS_EncodeString to JS_EncodeStringToLatin1. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa11c398f118
Part 9: Additional miscellaneous clean-ups around EncodeString callers. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fb73b65b580
Part 10: Remove ValueToPrintable{Latin1,UTF8}, add IdToPrintableUTF8. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13b67ae8524
Part 11: Remove the unused |inBuf| argument from JS::FormatStackDump and change it to use Sprinter. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb37a9c0cff4
Part 12: Change the decompiler to return UTF-8 encoded strings. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d885143760
Part 13: Use JS_ReportErrorNumberASCII when the error arguments are guaranteed to be ASCII characters. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bafa1db31b1
Part 14: Change FormatStackDump to return UTF-8 encoded strings. r=Waldo

Comment 51

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/52ae4d84b11e
https://hg.mozilla.org/mozilla-central/rev/775159907c73
https://hg.mozilla.org/mozilla-central/rev/8cc4218166b8
https://hg.mozilla.org/mozilla-central/rev/3f546769e86c
https://hg.mozilla.org/mozilla-central/rev/a7f57108d94f
https://hg.mozilla.org/mozilla-central/rev/21181aea32fa
https://hg.mozilla.org/mozilla-central/rev/b19d4e73e016
https://hg.mozilla.org/mozilla-central/rev/9f5767f1b04c
https://hg.mozilla.org/mozilla-central/rev/aa11c398f118
https://hg.mozilla.org/mozilla-central/rev/8fb73b65b580
https://hg.mozilla.org/mozilla-central/rev/e13b67ae8524
https://hg.mozilla.org/mozilla-central/rev/eb37a9c0cff4
https://hg.mozilla.org/mozilla-central/rev/44d885143760
https://hg.mozilla.org/mozilla-central/rev/4bafa1db31b1