1492090 - UTF8 incompatibility: JS::CompileOptions::setFileAndLine + JSErrorReport::filename

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[TheQwertiest]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180830143136

Steps to reproduce:

// windows-1251 encoded
const char* filename_cp1251 = "абвгд.js"; // binary: e0 e1 e2 e3 e4 2e 6a 73 00
// utf8 encoded
const char* filename_utf8 = u8"\u0430\u0431\u0432\u0433.js"; // binary: d0 b0 d0 b1 d0 b2 d0 b3 d0 b4 2e 6a 73 00

bool useUtf8;
const char* filename = useUtf8 ? filename_utf8 : filename_cp1251;

JS::CompileOptions opts( cx );
opts.setFileAndLine( filename, 1 );

if ( !JS::Evaluate( cx, opts, scriptCode, scriptCodeLength, &rval) )
{
    ...
    // catch and handle JS exception
    JSErrorReport* report = JS_ErrorFromException( cx, excnObject );
    const char* filename_out = report.filename;
}




Actual results:

const char* filename_out_cp1251 = "\0\0\0\0.js" // binary: 00 00 00 00 00 2e 6a 73 00
const char* filename_out_utf8 = "01234.js" // binary: 30 31 32 33 34 2e 6a 73 00


Expected results:

Filename should've remained the same at least in UTF8 case.

Comment 1

[TheQwertiest]

Typo in filename_utf8, should be:
const char* filename_utf8 = u8"\u0430\u0431\u0432\u0433\u0434.js"

Comment 2

[André Bargull [:anba]]

I have a local patch to change the compile functions to use UTF-8 encoded file names instead of using the system encoding, but it still needs some testing: https://hg.mozilla.org/try/rev/a9250ad22f049d81ee25a625af1348cc52d7c7c2

Comment 3

[André Bargull [:anba]]

Attachment: bug1492090-part1-comments-macros-and-scopexit.patch

Clean-up patch 1 of 3:

Update a few comments, replace #define with constexpr, and use ScopeExit for more robust file handler clean-up.

I've only left a TODO note for ScriptSource::introductionType_, because I didn't want to spend time investigating the dozen undocumented introduction type constants...

Comment 4

[André Bargull [:anba]]

Attachment: bug1492090-part2-utf8-output-and-CompileUtf8Path.patch

Clean-up patch 2 of 3:

- Use CompileUtf8Path instead of CompileUtf8File for the xpc-shells, so the next patches don't need to handle UTF-8 to narrow/wide string conversions here. (This change modifies an error message, so I had to update one xpcshell test.)
- Change a few additional callers to use UTF-8 for printing.
- Change SetARMHwCapFlags to use JS_EncodeStringToUTF8, because SetARMHwCapFlags only performs ASCII string comparisons.

Comment 5

[André Bargull [:anba]]

Attachment: bug1492090-part3-shell-os-functions.patch

Clean-up patch 3 of 3:

- Reduce code duplication for file operations by adding new helpers functions to OSObject.{h,cpp}.
- This change results in more string copying in some functions, which makes it look inefficient, but we'll need these additional string copies for part 5.
- I didn't bother to copy the "Use Latin1 variant here [...]" comments when calling JS_ReportErrorLatin1, because part 5 will change it to JS_ReportErrorUTF8 anyway.
- Removed the misleading comment about EINVAL for _splitpath. (EINVAL is only reported when _splitpath_s is used, so the 'secure' _splitpath version.)
- And moved some declarations into if-statements for more concise code.

Comment 6

[André Bargull [:anba]]

Attachment: bug1492090-part4-narrow-wide-utf8-encoding.patch

So, now for the more interesting patches.

It looks like the C++ standard functions to convert from narrow multi-byte strings to UTF-8 strings, is to use |mbsrtowcs| first, which gives us a wide string, and then use |std::codecvt_utf8<wchar_t>| to convert the wide string into a UTF-8 string. (I didn't see a direct narrow multi-byte string to UTF-8 string conversion utility in the C++ std functions.)

Technically speaking codecvt_utf8 was deprecated in C++17, so it doesn't seem preferable to use it here. But given that we don't yet use C++17 (with C++17 we might be able to use <filesystem>) and the C++ standard committee didn't propose any alternatives, I'd say it's okay-ish to use codecvt_utf8 for now. Except we still seem to support building Firefox on Linux with old libstd++ versions which don't provide <codecvt>. So when we're on Linux, instead of using codecvt_utf8, we simply assume wchar_t is UTF-32/UCS-4 and then manually convert the wchar_t string to UTF-8. *shrug*

The encode narrow/wide to/from UTF-8 functions are marked as JS_FRIEND_API, because we also need them for the xpc-shell. :-/


All this assumes the narrow multi-byte string encoding / execution character set / ANSI code page matches the file system encoding, which may not necessarily be the case, but I think this approach is still better than the status-quo, which simply converts everything to Latin-1.

Comment 7

[André Bargull [:anba]]

Attachment: bug1492090-part5-utf8-script-file-name.patch

The actual patch to change the internal encoding of script file names to UTF-8.

I hope I've spotted every place where script file names are converted to JSStrings, but thankfully Gecko seems to use URI/URL encoded file names exclusively, so there wasn't much to change outside of SpiderMonkey.

- A backward compatibility layer to accept non-UTF-8 file names wasn't added, because it didn't seem worth the additional complexity. 
- Windows is special and needs the wide string functions for proper access to files whose name isn't part of the current ANSI code page, no big surprise here.
- I'm not 100% sure about the change in SavedStacks::getLocation():
  - AtomizeUTF8Chars can trigger GC, so having the PCKey on the stack leads to a hazard failure.
  - To work around the hazard failure, I've simply changed the PCKey for the |lookupForAdd| to use a temporary object.
  - But is this change valid or are there some HashMap::AddPtr invariants which require that the key is alive as long as the AddPtr is used?

Also:
- The nullptr special case in JS_NewStringCopyZ was annoying when changing callers to use JS_NewStringCopyUTF8N.
- JS_NewStringCopyUTF8N/JS_NewStringCopyUTF8Z are not really ergonomic when compared to JS_NewStringCopyZ.
- *grumbles*

Comment 8

[André Bargull [:anba]]

Attachment: bug1492090-part6-more-utf8-followups.patch

Use the narrow/wide conversion functions in a few more places to replace Latin-1 with UTF-8 strings.

Comment 9

[Jeff Walden [:Waldo]]

Bug 987069 had several rounds of iteration on making file names UTF-8 back in the day, that may be informative/helpful here.  Or maybe not.  I dumped multiple weeks into reviewing those and following call chains, but we never landed any of it.  :-(

Comment 10

[André Bargull [:anba]]

Yes, I saw a couple of TODO notes mentioning bug 987069 in the code I've modified. Bug 987069 additionally changed the type of JSScript::filename from |const char*| to |JS::ConstUTF8CharsZ| to be more explicit about the encoding, but with today's ConstUTF8CharsZ we can't do this easily, because ConstUTF8CharsZ now requires a |strlen| call when constructing it, which is kind of annoying. OTOH using ConstUTF8CharsZ obviously makes it easier to spot which code needs to be updated, because err... everything needs to be updated after the type change.

Comment 11

[André Bargull [:anba]]

Oh, I just realized I can write tests for the patch using the |evaluate| function. Part 7 to follow tomorrow(?). :-)

(Normal files with Unicode names aren't accepted by the try-server: "UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 37: ordinal not in range(128)" -> <https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=199701887&revision=e7e00cc0e16980a175f4d67629c3eb38795e4b53>)

Comment 12

[TheQwertiest]

> and then use |std::codecvt_utf8<wchar_t>| to convert the wide string 

AFAIK std::codecvt_* is very very (very (really very)) slow and should not be used in any intensive manner. 
It can be slower by an order of magnitude than native WinAPI MultiByteToWideChar/WideCharToMultiByte equivalents, which is caused by it's C++ specification, and was one of the reasons why it was deprecated.

Just a heads up =)

Comment 13

[André Bargull [:anba]]

I guess it's acceptable to use codecvt here, because the only non-shell testing function which end up calling codecvt are JS::CompileUtf8Path and JS::EvaluateUtf8Path, and these two shouldn't be performance-sensitive.

Comment 14

[André Bargull [:anba]]

Attachment: bug1492090-part5-utf8-script-file-name.patch

When writing the tests, I've found some more places which need to be updated to handle UTF-8 strings. Changes:
- GetLcovInfo() in builtin/TestingFunctions.cpp now needs to use JS_NewStringCopyUTF8N.
- DebuggerScript_getUrlImpl in vm/Debugger.cpp also needs to use the NewStringCopyUTF8 functions.
- wasm::ScriptedCaller::filename was already treated as a UTF-8/ASCII string from Gecko, but the shell function ConsumeBufferSource() allowed to insert Latin-1 strings. Updated both to clarify UTF-8 strings are now used.
- And left a TODO note in vm/Probes.cpp for DTrace.

Comment 15

[André Bargull [:anba]]

Attachment: bug1492090-part7-tests.patch

And some tests.

Comment 16

[André Bargull [:anba]]

In part 5 I was a bit too aggressive when removing the Windows-only code in XPCShellImpl.cpp, these lines need to stay: https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/js/xpconnect/src/XPCShellImpl.cpp#165-175

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 9010701 [details] [diff] [review]
bug1492090-part1-comments-macros-and-scopexit.patch

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

Stealing.

::: js/src/builtin/TestingFunctions.cpp
@@ +2378,5 @@
> +    auto closeFile = mozilla::MakeScopeExit([&dumpFile] {
> +        if (dumpFile) {
> +            fclose(dumpFile);
> +        }
> +    });

*applause*

::: js/src/vm/JSAtom.cpp
@@ +918,5 @@
>  
>  JSAtom*
>  js::AtomizeUTF8Chars(JSContext* cx, const char* utf8Chars, size_t utf8ByteLength)
>  {
>      // This could be optimized to hand the char16_t's directly to the JSAtom

Pre-existing, but they're not char16_t's.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 9010701 [details] [diff] [review]
bug1492090-part1-comments-macros-and-scopexit.patch

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

(only noticed the steal after I was done with this, but hey, comments...)

::: ipc/testshell/XPCShellEnvironment.cpp
@@ -289,5 @@
>           *
>           * Support the UNIX #! shell hack; gobble the first line if it starts
> -         * with '#'.  TODO - this isn't quite compatible with sharp variables,
> -         * as a legal js program (using sharp variables) might start with '#'.
> -         * But that would require multi-character lookahead.

lol stab

::: js/public/CompilationAndEvaluation.h
@@ -122,5 @@
>   * Evaluate the provided UTF-8 data in the scope of the current global of |cx|,
>   * and return the completion value in |rval|.  If the data contains invalid
>   * UTF-8, an error is reported.
> - *
> - * The |options.utf8| flag is asserted to be true.  At a future time this flag

Oh, whoops, forgot to remove these.  😳

::: js/public/CompileOptions.h
@@ +120,5 @@
>      bool hideScriptFromDebugger = false;
>  
>      /**
> +     * |introductionType| is a statically allocated C string. See JSScript.h
> +     * for more information.

I mildly hate to do this, but this is public API documentation, directing the reader to internal API of...different documentation status.  No bueno.  Document introduction types here, and make JSScript.h point here.

I'm fine flipping the XXX over to here, as long as you're cleaning this up later in these patches.  But you better or else I've got an r- coming for you somewhere here.  :-)

::: js/src/shell/js.cpp
@@ +2365,5 @@
>  ReadLine(JSContext* cx, unsigned argc, Value* vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    constexpr size_t InitialBufferSize = 256;

Maybe a blank line after this, to separate preliminaries from actual algorithm?

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 9010707 [details] [diff] [review]
bug1492090-part3-shell-os-functions.patch

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

::: js/src/shell/OSObject.cpp
@@ +90,5 @@
>      return false;
>  }
>  
> +static UniqueChars
> +DirectoryName(JSContext* cx, const char* path)

All these various helpers are really nice.  Too bad they can't be file-statics.

@@ +96,5 @@
> +#ifdef XP_WIN
> +    char dirName[PATH_MAX + 1];
> +    char* drive = nullptr;
> +    char* fileName = nullptr;
> +    char* fileExt = nullptr;

Could constexpr these.

@@ +102,5 @@
> +    _splitpath(path, drive, dirName, fileName, fileExt);
> +
> +    return DuplicateString(cx, dirName);
> +#else
> +    char dirName[PATH_MAX + 1];

Move this decl outside the #ifdef, because both arms share it.  Also the final |return DuplicateString(...)|.

@@ +104,5 @@
> +    return DuplicateString(cx, dirName);
> +#else
> +    char dirName[PATH_MAX + 1];
> +
> +    strncpy(dirName, path, PATH_MAX + 1);

Use ArrayLength(dirName) instead?

@@ +166,2 @@
>      if (resolveMode == ScriptRelative) {
> +        path = DirectoryName(cx, scriptFilename.get());

UniqueChars path = resolveMode == ScriptRelative
                   ? DirectoryName(cx, scriptFilename.get())
                   : GetCWD(cx);
if (!path) ...

feels nicer than a bunch of line breaks and syntax noise.

@@ +183,5 @@
> +    memcpy(result.get(), path.get(), pathLen);
> +    result[pathLen] = '/';
> +    memcpy(result.get() + pathLen + 1, filename.get(), filenameLen);
> +
> +    return JS_NewStringCopyZ(cx, result.get());

Wouldn't this be cleaner as

  StringBuffer buf(cx);
  if (!buf.append(path.get(), strlen(path.get())))
      return false;
  if (!buf.append('/'))
      return false;
  if (!buf.append(filename.get(), strlen(filename.get())))
      return false;
  return buf.finishString();

?

@@ +186,5 @@
> +
> +    return JS_NewStringCopyZ(cx, result.get());
> +}
> +
> +FILE*

static?

@@ +190,5 @@
> +FILE*
> +OpenFile(JSContext* cx, const char* filename, const char* mode)
> +{
> +    FILE* file = fopen(filename, mode);
> +    if (!file) {

More readable as

  if (FILE* file = ...)
    return file;

  if (UniqueChars error = ...) {
    ...
  }

  return nullptr;

@@ +200,5 @@
> +    }
> +    return file;
> +}
> +
> +bool

static?

@@ +204,5 @@
> +bool
> +ReadFile(JSContext* cx, const char* filename, FILE* file, char* buffer, size_t length)
> +{
> +    size_t cc = fread(buffer, sizeof(char), length, file);
> +    if (cc != length) {

As in the other, invert to make the success case return fast, de-indent the rest of the body.

@@ +218,5 @@
> +    }
> +    return true;
> +}
> +
> +bool

static?

Comment 20

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] from comment #18)
> ::: js/public/CompileOptions.h
> @@ +120,5 @@
> >      bool hideScriptFromDebugger = false;
> >  
> >      /**
> > +     * |introductionType| is a statically allocated C string. See JSScript.h
> > +     * for more information.
> 
> I mildly hate to do this, but this is public API documentation, directing
> the reader to internal API of...different documentation status.  No bueno. 
> Document introduction types here, and make JSScript.h point here.
> 
> I'm fine flipping the XXX over to here, as long as you're cleaning this up
> later in these patches.  But you better or else I've got an r- coming for
> you somewhere here.  :-)
> 

Hmm, but I also don't want to document the ad-hoc introduction types defined from various shell helpers [1] in a public API.

[1] https://searchfox.org/mozilla-central/search?q=setIntroductionType&case=true&regexp=false&path=js%2Fsrc%2Fshell%2Fjs.cpp

Instead I could just change the comment from:
---
|introductionType| is a statically allocated C string: one of "eval", "Function", or "GeneratorFunction".
---

to just:
---
|introductionType| is a statically allocated C string. For example "eval", "Function", or "GeneratorFunction".
---

That way it doesn't sound like *only* "eval", "Function", or "GeneratorFunction" are allowed introduction types.

Comment 21

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] from comment #19)
> ::: js/src/shell/OSObject.cpp
> @@ +102,5 @@
> > +    _splitpath(path, drive, dirName, fileName, fileExt);
> > +
> > +    return DuplicateString(cx, dirName);
> > +#else
> > +    char dirName[PATH_MAX + 1];
> 
> Move this decl outside the #ifdef, because both arms share it.  Also the
> final |return DuplicateString(...)|.
> 

The declarations and calls will diverge in part 5.

> @@ +183,5 @@
> > +    memcpy(result.get(), path.get(), pathLen);
> > +    result[pathLen] = '/';
> > +    memcpy(result.get() + pathLen + 1, filename.get(), filenameLen);
> > +
> > +    return JS_NewStringCopyZ(cx, result.get());
> 
> Wouldn't this be cleaner as
> 
>   StringBuffer buf(cx);
>   if (!buf.append(path.get(), strlen(path.get())))
>       return false;
>   if (!buf.append('/'))
>       return false;
>   if (!buf.append(filename.get(), strlen(filename.get())))
>       return false;
>   return buf.finishString();
> 
> ?

Part 5 changes the JS_NewStringCopyZ call to JS_NewStringCopyUTF8N and because StringBuffer doesn't support UTF-8, I'll leave this as is.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 9010711 [details] [diff] [review]
bug1492090-part4-narrow-wide-utf8-encoding.patch

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

If this is really what the standard library thinks is a good clear way to do decoding and stuff, no wonder they deprecated/removed this API.  Yikes is this stuff terrible to understand.

::: js/public/CharacterEncoding.h
@@ +337,5 @@
> +
> +/**
> + * Encode a wide string to a UTF-8 string.
> + *
> + * NOTE: Should only be used when interacting with Windows API functions.

Is this really right?  Seems like it ought be usable with any wchar_t data really.  The true distinction would seem to be that it must not be used for normal data *that was created UTF-16*, and ordinarily if you have wchar_t data you can't actually know that -- you'd have had char16_t instead for it, right?

So I guess maybe I'd say "NOTE: Wide and wchar_t are not identical to char16_t.  If you have char16_t data that contains UTF-16, use a TwoByteChars API to encode it."  Maybe?  Let's figure out something.

::: js/src/js.msg
@@ +365,5 @@
> +// System encoding errors
> +MSG_DEF(JSMSG_CANT_CONVERT_TO_NARROW,  0, JSEXN_NOTE, "can't convert to narrow string")
> +MSG_DEF(JSMSG_CANT_CONVERT_TO_WIDE,    0, JSEXN_NOTE, "can't convert to wide string")
> +MSG_DEF(JSMSG_CANT_CONVERT_WIDE_TO_UTF8, 0, JSEXN_NOTE, "can't convert wide string to UTF-8")
> +MSG_DEF(JSMSG_CANT_CONVERT_UTF8_TO_WIDE, 0, JSEXN_NOTE, "can't convert UTF-8 to wide string")

JSEXN_RANGEERR for all of these, really.  Or maybe TypeError, but RangeError feels slightly better.  I don't even know what happens if you report errors with JSEXN_NOTE -- that's just the type for notes tacked onto other errors, and I don't think it gets thrown directly.

::: js/src/vm/CharacterEncoding.cpp
@@ +544,5 @@
> +        JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_CANT_CONVERT_TO_WIDE);
> +        return nullptr;
> +    }
> +    MOZ_ASSERT(std::mbsinit(&mb),
> +               "multi-byte state is in its initial state when no conversion error occured");

occurred

@@ +582,5 @@
> +    }
> +
> +    // STL returns |codecvt_base::partial| for empty strings.
> +    if (len == 0) {
> +        return utf8;

You didn't fill in the null-terminator here.  Can we get a test somehow that would have caught this?  jsapi-test is probably simplest.

@@ +621,5 @@
> +    }
> +    auto utf8 = cx->make_pod_array<char>(utf8BufLen.value());
> +    if (!utf8) {
> +        return nullptr;
> +    }

Everything from the |len| computation down to here is identical between these two halves.  No bueno.  Define all this in a lambda that takes in the max-character-length as argument so it isn't wholly duplicated between the two halves.

@@ +633,5 @@
> +        }
> +    }
> +    *dst = '\0';
> +
> +    return utf8;

Move the shared return outside the #ifdef.

@@ +657,5 @@
> +        JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_CANT_CONVERT_TO_NARROW);
> +        return nullptr;
> +    }
> +    MOZ_ASSERT(std::mbsinit(&mb),
> +               "multi-byte state is in its initial state when no conversion error occured");

occurred

@@ +690,5 @@
> +    }
> +
> +    // STL returns |codecvt_base::partial| for empty strings.
> +    if (len == 0) {
> +        return wideChars;

Missing null termination again.

@@ +713,5 @@
> +    size_t len = strlen(chars);
> +    auto wideChars = cx->make_pod_array<wchar_t>(len + 1);
> +    if (!wideChars) {
> +        return nullptr;
> +    }

Move the len/wideChars computation outside the #ifndefs, since it's shared between the two.

Comment 23

[Jeff Walden [:Waldo]]

(In reply to André Bargull [:anba] from comment #21)

Wouldn't this be cleaner as

StringBuffer buf(cx);
if (!buf.append(path.get(), strlen(path.get())))
return false;
if (!buf.append('/'))
return false;
if (!buf.append(filename.get(), strlen(filename.get())))
return false;
return buf.finishString();

?

Part 5 changes the JS_NewStringCopyZ call to JS_NewStringCopyUTF8N and
because StringBuffer doesn't support UTF-8, I'll leave this as is.

I have no recollection at all of what any of this was/is at this point -- but in going through old bugmail for this, I see that maybe StringBuffer::append(const mozilla::Utf8Unit* units, size_t len) might be useful here. Figure out whether it makes any sense to use that?

Comment 24

[Jason Orendorff [:jorendorff]]

Review ping.

Comment 25

[TheQwertiest]

Note: this bug is present in ESR68 as well.

Comment 26

[Philip Chimento [:ptomato]]

I'd like to work on bug 1505129 and ultimately bug 1506323, so I'd appreciate these patches being landed since this is a blocker for it.

Comment 27

[TheQwertiest]

Note: still present in ESR91.10

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Waldo isn't working for Mozilla anymore. Steven, any chance you can help find someone to review these? Though they'll probably need rebasing at this point too :-(

Comment 29

[Tooru Fujisawa [:arai]]

Comment on attachment 9011013 [details] [diff] [review]
bug1492090-part5-utf8-script-file-name.patch

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

Sorry that this got slipped through.
Although some part needs rebase, the patch itself looks good.
Shall I rebase, or do you want to rebase?

Comment 30

[Tooru Fujisawa [:arai]]

I'll rebase this onto m-c tip.

Comment 31

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 1: Fix some comments, remove a #define, and make one fclose call more robust. r=tcampbell!

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 2: Use UTF-8 encoding when printing to stdout and use CompileUtf8Path for xpc-shells. r=nbp!

Depends on D151445

Comment 33

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 3: Make shell file system functions more reusable. r=nbp!

Depends on D151446

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 4: Add encoding functions to/from UTF-8 to system narrow/wide encoding. r=nbp!

Depends on D151447

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 5: Use UTF-8 for script file names. r=nbp!

Depends on D151448

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 6: Use UTF-8 encoding for additional shell and testing functions. r=nbp!

Depends on D151449

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 7: Add tests for UTF-8 encoding in script file names. r=nbp!

Depends on D151450

Comment 38

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 8: Use UTF-8 in ReadGeckoInterpProfilingStack. r=nbp!

Depends on D151451

Comment 39

[Tooru Fujisawa [:arai]]

Attachment: Bug 1492090 - Part 9: Use UTF-8 in module ThrowResolutionError. r=nbp!

Depends on D151452

Comment 40

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #29)

Shall I rebase, or do you want to rebase?

Thanks for taking care of the rebase!

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddb2eb1a1c62
Part 1: Fix some comments, remove a #define, and make one fclose call more robust. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/8e3b95812c39
Part 2: Use UTF-8 encoding when printing to stdout and use CompileUtf8Path for xpc-shells. r=nbp
https://hg.mozilla.org/integration/autoland/rev/bc39eac4ecca
Part 3: Make shell file system functions more reusable. r=nbp
https://hg.mozilla.org/integration/autoland/rev/4e7c4724e994
Part 4: Add encoding functions to/from UTF-8 to system narrow/wide encoding. r=nbp
https://hg.mozilla.org/integration/autoland/rev/e4aad0fb85b9
Part 5: Use UTF-8 for script file names. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d2604a65c523
Part 6: Use UTF-8 encoding for additional shell and testing functions. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ed83091dfdb5
Part 7: Add tests for UTF-8 encoding in script file names. r=nbp
https://hg.mozilla.org/integration/autoland/rev/aeee4b4981b4
Part 8: Use UTF-8 in ReadGeckoInterpProfilingStack. r=nbp
https://hg.mozilla.org/integration/autoland/rev/1e0705e94f79
Part 9: Use UTF-8 in module ThrowResolutionError. r=nbp

Comment 42

[Serban Stanca [:SerbanS]]

Backed out for causing multiple xpcshell failures.

• Backout link

---

• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-PASS | netwerk/test/unit/test_socks.js | xpcshell return code: 0

---

• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-PASS | testing/xpcshell/example/unit/test_check_nsIException_failing.js | xpcshell return code: 0

---

• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-PASS | xpcom/tests/unit/test_bug476919.js | xpcshell return code: 0

Comment 43

[Tooru Fujisawa [:arai]]

the failure on windows is fixed.

the failure on android seems to be caused by std::mbsrtowcs call.
calling std::mbsrtowcs inside JS::EncodeNarrowToUtf8 caused the test to unconditionally pass (the test is expected to fail). even if the result is unused.
https://treeherder.mozilla.org/jobs?repo=try&revision=b58e3a8061c53c97f7bd8eb8fd338782965ad098&selectedTaskRun=IJ4Wuxe-SnOGBZ41hiW2-w.0

commenting out the call makes the test green:
https://treeherder.mozilla.org/jobs?repo=try&revision=401b906975067a8959176427e93d50d4af0bdb96&selectedTaskRun=UAvQnvTCQL2s0WSLKZ7GtA.0

some kind of memory corruption or something would be happening there

Comment 44

[Tooru Fujisawa [:arai]]

looks like there's also an issue in the test harness.
I put MOZ_RELEASE_ASSERT(false); and all test passes.
so, if the shell crashes, it's treated as test pass?

Comment 45

[Tooru Fujisawa [:arai]]

The crash is most likely caused by an unexpected behavior in std::mbsrtowcs implementation on Android.

According to the document, calling std::mbsrtowcs(dst, &src, len, ps) with dst==nullptr doesn't move src:

https://en.cppreference.com/w/cpp/string/multibyte/mbsrtowcs

The conversion stops if:

• The multibyte null character was converted and stored.
  src is set to a null pointer and *ps represents the initial shift state.
• An invalid multibyte character (according to the current C locale) was encountered.
  src is set to point at the beginning of the first unconverted multibyte character.
• the next wide character to be stored would exceed len.
  src is set to point at the beginning of the first unconverted multibyte character. This condition is not checked if dst is a null pointer.
  ...
  Notes
  This function moves the src pointer to the end of the converted multibyte string.
  This doesn't happen if dst is a null pointer.

but on Android, calling std::mbsrtowcs with dst==nullptr moves src to point nullptr.

std::mbsrtowcs is supposed to be called twice, and we do so in JS::EncodeNarrowToUtf8, but chars pointer is overwritten in the first call and the 2nd call hits null-pointer-deref.

  size_t wideLen = std::mbsrtowcs(nullptr, &chars, 0, &mb);
...
  mozilla::DebugOnly<size_t> actualLen =
      std::mbsrtowcs(wideChars.get(), &chars, bufLen, &mb);

I've looked into the Android source, but I don't find any issue in the source there...

anyway, using temporary variable solves the issue.

Comment 46

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #45)

I've looked into the Android source, but I don't find any issue in the source there...

It seems this was fixed in https://android.googlesource.com/platform/bionic/+/89e29ee485253ad39b8bfd514f1c3b5c3e52f98d%5E%21/, which also points to this bug report. So I guess the test environment just uses an old Android version.

Comment 47

[Tooru Fujisawa [:arai]]

(In reply to André Bargull [:anba] from comment #46)

(In reply to Tooru Fujisawa [:arai] from comment #45)

I've looked into the Android source, but I don't find any issue in the source there...

It seems this was fixed in https://android.googlesource.com/platform/bionic/+/89e29ee485253ad39b8bfd514f1c3b5c3e52f98d%5E%21/, which also points to this bug report. So I guess the test environment just uses an old Android version.

Thanks!
I've added a workaround to JS::EncodeNarrowToUtf8 and also added comment.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87a02a089678
Part 1: Fix some comments, remove a #define, and make one fclose call more robust. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/a42a6210de08
Part 2: Use UTF-8 encoding when printing to stdout and use CompileUtf8Path for xpc-shells. r=nbp
https://hg.mozilla.org/integration/autoland/rev/65e26a85981e
Part 3: Make shell file system functions more reusable. r=nbp
https://hg.mozilla.org/integration/autoland/rev/768f35c0db6b
Part 4: Add encoding functions to/from UTF-8 to system narrow/wide encoding. r=nbp
https://hg.mozilla.org/integration/autoland/rev/a872a1fd5147
Part 5: Use UTF-8 for script file names. r=nbp
https://hg.mozilla.org/integration/autoland/rev/1377e06cff9f
Part 6: Use UTF-8 encoding for additional shell and testing functions. r=nbp
https://hg.mozilla.org/integration/autoland/rev/c4547e2db877
Part 7: Add tests for UTF-8 encoding in script file names. r=nbp
https://hg.mozilla.org/integration/autoland/rev/a488d7627491
Part 8: Use UTF-8 in ReadGeckoInterpProfilingStack. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7201691d9e37
Part 9: Use UTF-8 in module ThrowResolutionError. r=nbp

Comment 49

[Norisz Fay [:noriszfay]]

Backed out for causing xpcshell failures at test_async_response_sending.js.

Backout link

Push with failures

Failure log

There's also this xpcshell failure too.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/062102d66345
Part 1: Fix some comments, remove a #define, and make one fclose call more robust. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/70831a15254d
Part 2: Use UTF-8 encoding when printing to stdout and use CompileUtf8Path for xpc-shells. r=nbp
https://hg.mozilla.org/integration/autoland/rev/519aee0147a0
Part 3: Make shell file system functions more reusable. r=nbp
https://hg.mozilla.org/integration/autoland/rev/68328f314a79
Part 4: Add encoding functions to/from UTF-8 to system narrow/wide encoding. r=nbp
https://hg.mozilla.org/integration/autoland/rev/416af93c3205
Part 5: Use UTF-8 for script file names. r=nbp
https://hg.mozilla.org/integration/autoland/rev/0a5fa01b9714
Part 6: Use UTF-8 encoding for additional shell and testing functions. r=nbp
https://hg.mozilla.org/integration/autoland/rev/0b7b5cf30557
Part 7: Add tests for UTF-8 encoding in script file names. r=nbp
https://hg.mozilla.org/integration/autoland/rev/51d9f8a4331e
Part 8: Use UTF-8 in ReadGeckoInterpProfilingStack. r=nbp
https://hg.mozilla.org/integration/autoland/rev/feb9591113b7
Part 9: Use UTF-8 in module ThrowResolutionError. r=nbp

Comment 51

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/062102d66345
https://hg.mozilla.org/mozilla-central/rev/70831a15254d
https://hg.mozilla.org/mozilla-central/rev/519aee0147a0
https://hg.mozilla.org/mozilla-central/rev/68328f314a79
https://hg.mozilla.org/mozilla-central/rev/416af93c3205
https://hg.mozilla.org/mozilla-central/rev/0a5fa01b9714
https://hg.mozilla.org/mozilla-central/rev/0b7b5cf30557
https://hg.mozilla.org/mozilla-central/rev/51d9f8a4331e
https://hg.mozilla.org/mozilla-central/rev/feb9591113b7