Open Bug 1492090 Opened 4 years ago Updated 23 days ago

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

Categories

(Core :: JavaScript Engine, defect, P3)

68 Branch
defect

Tracking

()

People

(Reporter: qwertiest, Assigned: anba)

References

Details

Attachments

(16 files, 1 obsolete file)

14.00 KB, patch
jorendorff
: review+
Waldo
: review+
Details | Diff | Splinter Review
8.92 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
27.08 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
12.95 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
8.39 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
74.73 KB, patch
arai
: review+
Details | Diff | Splinter Review
10.71 KB, patch
arai
: review+
Details | Diff | Splinter Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
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.
Typo in filename_utf8, should be:
const char* filename_utf8 = u8"\u0430\u0431\u0432\u0433\u0434.js"
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
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...
Assignee: nobody → andrebargull
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #9010701 - Flags: review?(jwalden+bmo)
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.
Attachment #9010704 - Flags: review?(jwalden+bmo)
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.
Attachment #9010707 - Flags: review?(jwalden+bmo)
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.
Attachment #9010711 - Flags: review?(jwalden+bmo)
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*
Attachment #9010721 - Flags: review?(jwalden+bmo)
Use the narrow/wide conversion functions in a few more places to replace Latin-1 with UTF-8 strings.
Attachment #9010722 - Flags: review?(jwalden+bmo)
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.  :-(
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.
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>)
> 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 =)
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.
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.
Attachment #9010721 - Attachment is obsolete: true
Attachment #9010721 - Flags: review?(jwalden+bmo)
Attachment #9011013 - Flags: review?(jwalden+bmo)
And some tests.
Attachment #9011014 - Flags: review?(jwalden+bmo)
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 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.
Attachment #9010701 - Flags: review?(jwalden+bmo) → review+
Attachment #9010704 - Flags: review?(jwalden+bmo) → review+
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?
Attachment #9010701 - Flags: review+
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?
Attachment #9010707 - Flags: review?(jwalden+bmo) → review+
Priority: -- → P3
(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.
(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.
Blocks: 1505129
Flags: needinfo?(jorendorff)
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.
Attachment #9010711 - Flags: review?(jwalden) → review+

(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?

Attachment #9010722 - Flags: review?(jwalden) → review+

Review ping.

Flags: needinfo?(jorendorff) → needinfo?(jwalden)

Note: this bug is present in ESR68 as well.

Version: 60 Branch → 68 Branch

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.

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 :-(

Flags: needinfo?(jwalden) → needinfo?(sdetar)
Attachment #9011013 - Flags: review?(jwalden) → review?(arai.unmht)
Attachment #9011014 - Flags: review?(jwalden) → review?(arai.unmht)
Flags: needinfo?(sdetar)
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?
Attachment #9011013 - Flags: review?(arai.unmht) → review+
Attachment #9011014 - Flags: review?(arai.unmht) → review+
Flags: needinfo?(arai.unmht)

I'll rebase this onto m-c tip.

Flags: needinfo?(arai.unmht)

(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!

You need to log in before you can comment on or make changes to this bug.