【Firefox】in演算子に関するエラーメッセージを改善した【SpiderMonkey】

# はじめに

大学の実験でFirefoxのJavaScriptエンジン「SpiderMonkey」にコントリビュートする機会が得られたので、そのときに行った手順を具体的に残したいと思います。
SpiderMonkeyに変更を加えたい人の参考に少しでもなれば幸いです。

# あわせて読みたい

この記事は実験で実際に解決したSpiderMonkeyのIssueのうち「その3」についてです。

- [この実験についての詳細](http://snowman-mh.hatenablog.com/entry/2017/11/14/172328)
- [SpiderMonkeyのコミッターになる方法](https://qiita.com/snowman_mh/items/b2bc19d2af54d79c271a)
- [解決したIssueその1](https://qiita.com/snowman_mh/items/e52202eee378e8481d3b)
- [解決したIssueその2](https://qiita.com/snowman_mh/items/e33f0b57e31f59acdc2d)

# 変更内容

## バグレポート

今回取り組んだIssueはこちら。
https://bugzilla.mozilla.org/show_bug.cgi?id=1352429

つまり、

```js
js> 'hello' in 'hello world';
TypeError: invalid 'in' operand "hello world"
js> true in null;
TypeError: invalid 'in' operand null
```

となっていたものを

```js
js> 'hello' in 'hello world';
TypeError: cannot use 'in' operator to search for 'hello' in 'hello world'
js> true in null;
TypeError: cannot use 'in' operator to search for 'true' in 'null'
```

となるように変更しました。

## パッチ

今回行った変更内容のdiffはこちら。
https://hg.mozilla.org/mozilla-central/rev/389bd78cf79d

## 演算子を解釈してエラーを出力する仕組み

2, 3日ソースコードを眺めただけのやつが言うことなので、間違っているところ、勘違いしているところは多々あるかと思うので、優しく指摘していただけると嬉しいです。

- コンパイラが演算子をパースしてバイトコードに変換したあと、`Interpreter.cpp`内の関数`Interpret(...)`で解釈される
- そして、`in`の右辺がオブジェクトではなかった場合、`ReportなんとかErrorなんとか(...)`みたいな関数を利用してエラーメッセージを出力します
- そのエラー出力の際に、右辺の値が文字列に変換される

## どう変更したか

具体的にどのような変更を行ったのか、先に簡単に紹介します。
今回はエラーメッセージ出力用のヘルパー関数を新たに実装したり、テストを新たに追加したりと比較的大きな変更を加えることができました。

1. `js.msg`内のエラーメッセージが2つの引数を受け取るように変更

    ```diff
- MSG_DEF(JSMSG_IN_NOT_OBJECT, 1, JSEXN_TYPEERR, "invalid 'in' operand {0}")
+ MSG_DEF(JSMSG_IN_NOT_OBJECT, 2, JSEXN_TYPEERR, "cannot use 'in' operator to search for '{0}' in '{1}'")
    ```

1. `in`の左辺・右辺それぞれを文字列に変換してエラーメッセージに渡す以下のヘルパー関数`ReportInNotObjectError(...)`を`Interpreter.cpp`に定義し、`Interpreter.h`で宣言することで外部からも呼べるようにした。また、`in`の両辺に長い文字列が来たときは文字列を短く省略するように実装した。

    ```cpp:Interpreter.cpp
    ...

    #include "vm/StringBuffer.h"


    ...

    void
    js::ReportInNotObjectError(JSContext* cx, HandleValue lref, int lindex,
                               HandleValue rref, int rindex)
    {
        auto uniqueCharsFromString = [](JSContext* cx, HandleValue ref) -> UniqueChars {
            static const size_t MAX_STRING_LENGTH = 16;
            RootedString str(cx, ref.toString());
            if (str->length() > MAX_STRING_LENGTH) {
                StringBuffer buf(cx);
                if (!buf.appendSubstring(str, 0, MAX_STRING_LENGTH))
                    return nullptr;
                if (!buf.append("..."))
                    return nullptr;
                str = buf.finishString();
                if (!str)
                    return nullptr;
            }
            return UniqueChars(JS_EncodeString(cx, str));
        };

        UniqueChars lbytes = lref.isString()
                           ? uniqueCharsFromString(cx, lref)
                           : DecompileValueGenerator(cx, lindex, lref, nullptr);
        if (!lbytes)
            return;
        UniqueChars rbytes = rref.isString()
                           ? uniqueCharsFromString(cx, rref)
                           : DecompileValueGenerator(cx, rindex, rref, nullptr);
        if (!rbytes)
            return;
        JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr, JSMSG_IN_NOT_OBJECT,
                                   lbytes.get(), rbytes.get());
    }


    ...
    ```
1. 該当エラーを呼んでいたところは2箇所あったので、それぞれから上記のヘルパー関数を呼ぶように変更した。`Interpreter.cpp`では、右辺の値を取得してからヘルパー関数に渡している。

    ```diff:Interpreter.cppの2250行目あたり
    ...

    CASE(JSOP_IN)
    {
         HandleValue rref = REGS.stackHandleAt(-1);
         if (!rref.isObject()) {
    -        ReportValueError(cx, JSMSG_IN_NOT_OBJECT, -1, rref, nullptr);
    +        HandleValue lref = REGS.stackHandleAt(-2);
    +        ReportInNotObjectError(cx, lref, -2, rref, -1);
             goto error;
         }
         bool found;

    ...
    ```

    ```diff:BaselineIC.cppの
    ...

        FallbackICSpew(cx, stub, "In");
 
        if (!objValue.isObject()) {
    -       ReportValueError(cx, JSMSG_IN_NOT_OBJECT, -1, objValue, nullptr);
    +       ReportInNotObjectError(cx, key, -2, objValue, -1);
            return false;
        }

    ...
    ```
1. 今回は長い文字列を省略してエラーメッセージに出力するような実装にしたので、適切に文字列が省略されていることをテストする必要があった。以下のテストを追加した。

    ```js:js/src/tests/ecma_6/Expressions/inNotObjectError.js
    var BUGNUMBER = 1352429;
    var summary = 'Error message should provide enough infomation for use of in operator';

    print(BUGNUMBER + ": " + summary);

    function checkErr(substr, str, messageSubstr, messageStr) {
        var caught = false;
        try {
            substr in str;
        } catch (e) {
            caught = true;
            assertEq(e.message.includes(messageSubstr), true);
            assertEq(e.message.includes(messageStr), true);
            assertEq(e.message.length < 100, true);
        }
        assertEq(caught, true);
    }

    // These test cases check if long string is omitted properly.
    checkErr('subString', 'base', 'subString', 'base');
    checkErr('this is subString', 'base', 'this is subStrin...', 'base');
    checkErr('subString', 'this is baseString', 'subString', 'this is baseStri...');
    checkErr('this is subString', 'this is base', 'this is subStrin...', 'this is base');
    checkErr('HEAD' + 'subString'.repeat(30000), 'HEAD' + 'base'.repeat(30000), 'HEADsubStringsub...', 'HEADbasebasebase...');

    // These test cases check if it does not crash and throws appropriate error.
    assertThrowsInstanceOf(() => { 1 in 'hello' }, TypeError);
    assertThrowsInstanceOf(() => { 'hello' in 1 }, TypeError);
    assertThrowsInstanceOf(() => { 'hello' in null }, TypeError);
    assertThrowsInstanceOf(() => { null in 'hello' }, TypeError);
    assertThrowsInstanceOf(() => { null in null }, TypeError);
    assertThrowsInstanceOf(() => { 'hello' in true }, TypeError);
    assertThrowsInstanceOf(() => { false in 1.1 }, TypeError);
    assertThrowsInstanceOf(() => { Symbol.iterator in undefined }, TypeError);
    assertThrowsInstanceOf(() => { [] in undefined }, TypeError);
    assertThrowsInstanceOf(() => { /a/ in 'hello' }, TypeError);
    var str = 'hello';
    assertThrowsInstanceOf(() => { str in 'hello' }, TypeError);
    class A {};
    assertThrowsInstanceOf(() => { new A() in undefined }, TypeError);
    var a = new A();
    a.b = 1.1;
    assertThrowsInstanceOf(() => { a.b in 1.1 }, TypeError);

    if (typeof reportCompare === 'function')
        reportCompare(0, 0);
    ```

# どう手探ったか

それでは上記の変更箇所を具体的にどのように手探っていったかを残していきます。
例によって[Searchfox](https://searchfox.org/mozilla-central/source/)とか[DXR](https://dxr.mozilla.org/mozilla-central/source/)を利用すると快適にコードサーチできます。

1. `"invalid 'in' operand"`で検索すると次のエラーメッセージの定義が見つかったので前述したように変更した

    ```js
MSG_DEF(JSMSG_IN_NOT_OBJECT, 1, JSEXN_TYPEERR, "invalid 'in' operand {0}")
    ```
1. 次は`"JSMSG_IN_NOT_OBJECT"`で検索すると、`Interpreter.cpp`と`BaselineIC.cpp`の2箇所で使用されていた
2. DXRのJump to definition機能を使って、上記2箇所で呼ばれている関数`ReportErrorValue(...)`の定義を見ると、`ReportValueErrorFlags(...)`という関数に`define`されていた
3. そして`ReportValueErrorFlags(...)`の定義を見ると、`jscntxt.cpp`の1030行目あたりに記述されていた

    ```cpp:jscntxt.cppの1030行目あたり
    bool
    js::ReportValueErrorFlags(JSContext* cx, unsigned flags, const unsigned errorNumber,
                                  int spindex, HandleValue v, HandleString fallback,
                          const char* arg1, const char* arg2)
    {
        UniqueChars bytes;
        bool ok;

        MOZ_ASSERT(js_ErrorFormatString[errorNumber].argCount >= 1);
        MOZ_ASSERT(js_ErrorFormatString[errorNumber].argCount <= 3);
        bytes = DecompileValueGenerator(cx, spindex, v, fallback);
        if (!bytes)
            return false;

        ok = JS_ReportErrorFlagsAndNumberLatin1(cx, flags, GetErrorMessage, nullptr, errorNumber,
                                                bytes.get(), arg1, arg2);
        return ok;
}
    ```
1. ここで`jscntxt.cpp`の1030行目にブレークポイントを設定して怪しそうな`bytes.get()`や`arg1`、`arg2`などを見てみることにする

    ```bash
$ lldb ./dist/bin/js
(lldb) b jscntxt.cpp:1030
# ブレークポイントが設定される
(lldb) r temp.js
# ブレークポイントで処理が止まる
(lldb) p bytes.get()
# => 'hello world'となっていた
    ```

    ```js:temp.js
'hello' in 'hello world';
    ```
1. `HandleValue v`を`DecompileValueGenerator(...)`で`UniqueChars bytes`に変換しているっぽいので、`ReportValueErrorFlags(...)`に`HandleValue v`をどのように渡しているかを見てみる
2. 先程発見した、`Interpreter.cpp`で該当のエラーメッセージを呼んでいるところを見てみると、2206行目あたりで`HandleValue rref = REGS.stackHandleAt(-1);`として取得した`rref`を`ReportErrorValue(...)`を介して`ReportValueErrorFlags(...)`に渡していることが分かった。変数の情報などがスタックに溜められていて、そこからその変数などへの参照を取得しているようなことが想像できるコードだった。
3. ここで、`REGS.stackHandleAt(-1);`を`REGS.stackHandleAt(-2);`としてみた状態で`'hello' in 'hello world';`を実行してみると、今まで`'hello world'`と表示されていた部分に`'hello'`と表示された。ということは`-1`と`-2`を使えば両辺の値が取ってこれそうと分かる。
4. その両辺の値それぞれをスタックから取得して渡すとエラーメッセージを出力してくれるようなヘルパー関数を作れば良いことがわかった。そこで、次のようなヘルパー関数を作って呼んでみるとうまく表示されているようだった！

    ```cpp:Interpreter.cppに新しく定義したヘルパー関数
    void
    js::ReportInNotObjectError(JSContext* cx, HandleValue lref, int lindex,
                               HandleValue rref, int rindex)
    {
        UniqueChars lbytes = DecompileValueGenerator(cx, lindex, lref, nullptr);
        if (!lbytes)
            return;
        UniqueChars rbytes = DecompileValueGenerator(cx, rindex, rref, nullptr);
        if (!rbytes)
            return;
        JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr, JSMSG_IN_NOT_OBJECT,
                                   lbytes.get(), rbytes.get());
    }
    ```
1. 長い文字列を省略するという部分を実装する必要があったが、とりあえずここで実験のTAさんに助言を求めると、`HandleValue`で保持している値が文字列かどうかは`isString()`という関数で判断できることと、`isString()`が`true`になる場合は`toString()`を使うと`HandleString`という型に変換できることを教えてもらった。また、`RootedString`という型(`HandleString`から簡単に変換できる)と`StringBuffer`なるものを使うと、文字列を省略したり`"..."`を付けたりするのが簡単にできるから調べてみてとのアドバイスもいただいた。
2. 文字列を省略して(しない場合もあり)`UniqueChars`型に変換して返すユーティリティのようなものを定義して、`isString()`が`true`のときだけそれに通して`lbytes`や`rbytes`に代入し、`isString()`が`false`の場合は従来通り`DecompileValueGenerator(...)`を使って`UniqueChars`型を作れば良いという結論に達し(TAさんにも相談しました)、最終的に次のようなヘルパー関数を定義することになった(完成品のヘルパー関数と同じ)。

    ```cpp:Interpreter.cpp
    ...

    #include "vm/StringBuffer.h"


    ...

    void
    js::ReportInNotObjectError(JSContext* cx, HandleValue lref, int lindex,
                               HandleValue rref, int rindex)
    {
        auto uniqueCharsFromString = [](JSContext* cx, HandleValue ref) -> UniqueChars {
            static const size_t MAX_STRING_LENGTH = 16;
            RootedString str(cx, ref.toString());
            if (str->length() > MAX_STRING_LENGTH) {
                StringBuffer buf(cx);
                if (!buf.appendSubstring(str, 0, MAX_STRING_LENGTH))
                    return nullptr;
                if (!buf.append("..."))
                    return nullptr;
                str = buf.finishString();
                if (!str)
                    return nullptr;
            }
            return UniqueChars(JS_EncodeString(cx, str));
        };

        UniqueChars lbytes = lref.isString()
                           ? uniqueCharsFromString(cx, lref)
                           : DecompileValueGenerator(cx, lindex, lref, nullptr);
        if (!lbytes)
            return;
        UniqueChars rbytes = rref.isString()
                           ? uniqueCharsFromString(cx, rref)
                           : DecompileValueGenerator(cx, rindex, rref, nullptr);
        if (!rbytes)
            return;
        JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr, JSMSG_IN_NOT_OBJECT,
                                   lbytes.get(), rbytes.get());
    }


    ...
    ```
1. これで`Interpreter.cpp`から呼び出していた部分は解決したが、`BaselineIC.cpp`から呼び出されていた部分は解決していない。
2. 改めて`BaselineIC.cpp`を見てみると、`HandleValue objValue`というのが渡っていて、これを`ReportValueError(...)`に渡していたので、`objValue`が`'hello' in 'hello world';`を実行したときの`'hello world'`に相当するということはすぐに分かった。
3. しかし、`'hello'`に相当する`HandleValue`はどうやって取得しようか考えていたところ、同じく`HandleValue`型の`key`という値が渡っていることに気づいた。
4. この`key`が`'hello'`なのではないかと期待を抱き、デバッガを走らせようとしたが、どうやってここに到達させれば良いのかがわからない！(今までどおり`'hello' in 'hello world';`を実行してもここにはこない)
5. TAさんに質問してみたところ、「`Baseline`とは、通常の処理を何回も行って、それでも処理できなかった場合に来る場所」らしいです(よくわからん)。
6. ということで、次のように`--baseline-eager`というオプションをつけると強制的に`Baseline`を使ったコンパイルを行ってくれることを教えてもらい、無事デバッガで`BaselineIC.cpp`で止めることに成功しました。

    ```bash
$ lldb ./dist/bin/js
(lldb) b BaselineIC.cpp:1273
# ブレークポイントが設定される
(lldb) r --baseline-eager temp.js
# ブレークポイントで止まる
(lldb) p *key.toString()
# 'hello'となっている！
    ```
    ```js:temp.js
'hello' in 'hello world';
    ```
1. よって、`ReportValueError(...)`を呼んでいたところを`ReportInNotObjectError(cx, key, -2, objValue, -1);`とすることで、`BaselineIC.cpp`でも所望のエラーメッセージが出力されるようになりました。
2. これで変更は終了し、無事パッチも本体に取り込まれました！