More than 5 years have passed since last update.

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

Last updated at 2017-11-14Posted at 2017-11-13

はじめに

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

あわせて読みたい

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

変更内容

バグレポート

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

つまり、

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

となっていたものを

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なんとか(...)みたいな関数を利用してエラーメッセージを出力します
そのエラー出力の際に、右辺の値が文字列に変換される

どう変更したか

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

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

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}'")

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

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());
}


...

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

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;

...

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;
    }

...

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

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とかDXRを利用すると快適にコードサーチできます。

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

MSG_DEF(JSMSG_IN_NOT_OBJECT, 1, JSEXN_TYPEERR, "invalid 'in' operand {0}")
```

次は"JSMSG_IN_NOT_OBJECT"で検索すると、Interpreter.cppとBaselineIC.cppの2箇所で使用されていた
DXRのJump to definition機能を使って、上記2箇所で呼ばれている関数ReportErrorValue(...)の定義を見ると、ReportValueErrorFlags(...)という関数にdefineされていた

そしてReportValueErrorFlags(...)の定義を見ると、jscntxt.cppの1030行目あたりに記述されていた

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;

}
```

ここでjscntxt.cppの1030行目にブレークポイントを設定して怪しそうなbytes.get()やarg1、arg2などを見てみることにする

$ 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';
```

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

その両辺の値それぞれをスタックから取得して渡すとエラーメッセージを出力してくれるようなヘルパー関数を作れば良いことがわかった。そこで、次のようなヘルパー関数を作って呼んでみるとうまく表示されているようだった！

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());
}

長い文字列を省略するという部分を実装する必要があったが、とりあえずここで実験のTAさんに助言を求めると、HandleValueで保持している値が文字列かどうかはisString()という関数で判断できることと、isString()がtrueになる場合はtoString()を使うとHandleStringという型に変換できることを教えてもらった。また、RootedStringという型(HandleStringから簡単に変換できる)とStringBufferなるものを使うと、文字列を省略したり"..."を付けたりするのが簡単にできるから調べてみてとのアドバイスもいただいた。

文字列を省略して(しない場合もあり)UniqueChars型に変換して返すユーティリティのようなものを定義して、isString()がtrueのときだけそれに通してlbytesやrbytesに代入し、isString()がfalseの場合は従来通りDecompileValueGenerator(...)を使ってUniqueChars型を作れば良いという結論に達し(TAさんにも相談しました)、最終的に次のようなヘルパー関数を定義することになった(完成品のヘルパー関数と同じ)。

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());
}


...

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

$ 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';
```

よって、ReportValueError(...)を呼んでいたところをReportInNotObjectError(cx, key, -2, objValue, -1);とすることで、BaselineIC.cppでも所望のエラーメッセージが出力されるようになりました。
これで変更は終了し、無事パッチも本体に取り込まれました！

You get articles that match your needs
You can efficiently read back useful information
You can use dark theme

What you can do with signing up