• 報告の流れ（おさらい）
• 不具合の原因箇所を特定する
• 不具合の修正方針を考え、実際に修正してみる
• 修正方針が正しいかどうか確かめる
• 修正を提案可能な状態に仕上げる
• 回帰テストを追加する
• まとめ

結城です。

このブログでは過去何度か、FirefoxやThunderbirdの不具合の原因調査や修正の事例をご紹介してきました。 ですが、原因の見つけ方や修正方針の検討の詳細な所はあまり言語化してきていなかったように思われます。

この記事では、Firefoxへの不具合報告の仕方の解説の続編として、Firefoxを使用していて遭遇した不具合の原因を特定し、経緯や事情を調査して適切な修正内容をまとめる所までの詳細な流れをご紹介します。

報告の流れ（おさらい）

前回記事の冒頭において、オープンソース開発プロジェクトへの外部からの関わり方として以下の3段階がある旨を述べました。

1. 不具合を報告する。
2. 原因を調査して明らかにする。
3. 不具合を修正するパッチを提供する。

本記事では、このうち2の部分について、2022年に筆者が実際に報告し2025年末に解決となったBug 1762249の事例を参考にして説明します。 「どのような不具合だったか」については前回記事で詳しく説明しているので、この記事から読もうとしている方は、まず前回記事からご覧下さい。

不具合の原因箇所を特定する

前回記事で詳述しましたが、この事例では、問題の発生時にFirefoxがJavaScriptの例外を投げています。 端的には、この例外の出所がまさに「修正が必要な箇所」と言えます。

ただ、今回発生している例外はアドオンからは「Error: An unexpected error occurred（エラー：予期しないエラーが発生しました）」という抽象的なメッセージを伴った物としてしか捕捉できていません。 about:debugging から開けるアドオン用のデバッグツールのコンソールで▸をクリックしてログを展開しても、例外がセキュリティ境界をまたいで通知されている¹せいで、スタックトレースは得られていません。

このような場合、Firefox本体のエラーコンソール（Ctrl-Shift-J）でヒントを得られることがあります。 FirefoxのWebExtensions APIはほぼすべてJavaScriptで実装されており、Firefox自体のデバッグツールであれば、Firefox内で生じた例外の詳細な情報を閲覧できます。 今回の事例でも、Firefoxのエラーコンソールにはスタックトレースを伴った例外の情報が出力されていました²。

Failed to create tab tabbrowser.js:2845
    addTab chrome://browser/content/tabbrowser.js:2845
    create chrome://browser/content/parent/ext-tabs.js:818

CanonicalBrowsingContext.setCrossGroupOpener: Argument 1 is not an object. tabbrowser.js:2839
    addTab chrome://browser/content/tabbrowser.js:2839
    create chrome://browser/content/parent/ext-tabs.js:818

このスタックトレースからは、以下の情報が読み取れます。

• browser.tabs.create() の実装が含まれている ext-tabs.js の818行目で呼び出した addTab() で例外が発生していて、tabbrowser.js の2845行目で報告された。
• その元々の例外は、CanonicalBrowsingContext インターフェースの setCrossGroupOpener メソッドを tabbrowser.js の2839行目で呼び出した際に、第1引数がオブジェクトになっていないことが原因で発生した物である。

Firefoxのオンラインコード検索サービスであるSearchfoxでファイル名やメソッド名を手がかりに検索して、（当時の）実装の当該箇所を見てみたところ、setCrossGroupOpener() メソッドの引数には openerBrowser.browsingContext が渡されていました。

2719 try {
...（中略）...
2836     if (openerBrowser && !openWindowInfo) {
2837       b.browsingContext.setCrossGroupOpener( // ここで例外が発生（根本的な原因）
2838         openerBrowser.browsingContext
2839       );
2840     }
2841   }
2842 } catch (e) {
2843   Cu.reportError("Failed to create tab"); // 1つ目の例外はここで記録されている
2844   Cu.reportError(e); // 2つ目の例外（根本的な原因）はここで記録されている
...（中略）...
2852 }

変数名から見て、openerBrowser は tabs.create() のパラメーターの openerTabId で指定されたタブに対応するオブジェクトと考えられます。 アンロードされたタブの場合はこのオブジェクトのbrowsingContextプロパティが null や undefined などの無効な値になっているらしく、setCrossGroupOpener() メソッドはそのような引数を想定していないために例外が発生している、という状況のようです。

なお、Searchfoxでの検索結果の行番号と、実際のスタックトレースに現れている行番号には若干のズレがありますが、これは、Searchfoxのインデックス作成時に使われたソースコードのリポジトリーのリビジョンと、Firefoxのバイナリーのビルド元となったリビジョンとが異なっているせいです。 リビジョンが大きく離れていると、Searchfoxでの検索結果と実際の実行時のスタックトレースとの乖離が大きくなってしまい、調査が難航しがちです。 実際の動作での検証・調査は、可能な限り新しめのビルドのFirefoxで行うようにしましょう。

不具合の修正方針を考え、実際に修正してみる

以上の調査結果から単純に考えると、openerBrowser.browsingContext が null や undefined のときは setCrossGroupOpener() メソッドを呼ばないようにすれば問題は解決しそうです³。

考えた修正方針が本当に有効かどうかは、実際にFirefoxに変更を反映して試してみないと分かりません。 そこで、WindowsでのFirefoxのビルド手順に従ってビルド環境を整えた上で、cloneしたリポジトリーの browser/base/content/tabbrowser.js （当時）の当該箇所を以下のように変更してみました。

if (
  openerBrowser &&
  !openWindowInfo &&
  openerBrowser.browsingContext // この条件を追加した
) {
  b.browsingContext.setCrossGroupOpener(
    openerBrowser.browsingContext
  );
}

Firefoxのビルドは基盤層を除いてビルドする「Artifact Modeビルド」と基盤層まで含めてビルドする「フルビルド」の2通りがありますが、今回のようにJavaScript実装の部分だけに変更が留まる場合は、動作の確認はArtifact Modeビルドで充分に行えます。 ということでビルドして報告時のテストケースで動作を確かめてみたところ、果たして、アンロードされたタブを openerTabId で指定した場合でも tabs.create() でタブを無事に開けるようになっていました。

修正方針が正しいかどうか確かめる

ただ、これで実際に動作はするようになりましたが、本当に「適切な修正方法」なのかどうかには不安が残ります。 「既存のタブからタブを開く」ときに setCrossGroupOpener() を呼ばないと他の部分で不具合が起こるようなら、この修正方法は適切でないことになります。

ということで、setCrossGroupOpener() を呼ばないことがどんな影響をもたらすのかを調べてみます。 Searchfoxでソースコードの当該箇所を表示しているときにメソッド名をクリックすると、そのメソッド名をSearchfoxで検索することができますので、この方法でメソッドの定義を見てみることにします。

検索結果を眺めると、C++の実装の中に setCrossGroupOpener() メソッドの実装箇所らしき結果⁴があります。 リンク先に遷移して実装を見てみると以下のようになっていて、端的に言うと、引数で渡されたオブジェクトの識別子を SetCrossGroupOpenerId() メソッドに渡すだけの物でした。

void CanonicalBrowsingContext::SetCrossGroupOpener(
    CanonicalBrowsingContext& aCrossGroupOpener, ErrorResult& aRv) {
  if (!IsTopContent()) {
    aRv.ThrowNotAllowedError(
        "Can only set crossGroupOpener on toplevel content");
    return;
  }
  if (mCrossGroupOpenerId != 0) {
    aRv.ThrowNotAllowedError("Can only set crossGroupOpener once");
    return;
  }

  SetCrossGroupOpenerId(aCrossGroupOpener.Id());
}

同じようにして SetCrossGroupOpenerId() メソッドの実装を見てみると、以下のように、メンバー変数 mCrossGroupOpenerId に値を代入するだけのいわゆるセッターであると分かりました。

void CanonicalBrowsingContext::SetCrossGroupOpenerId(uint64_t aOpenerId) {
  MOZ_DIAGNOSTIC_ASSERT(IsTopContent());
  MOZ_DIAGNOSTIC_ASSERT(mCrossGroupOpenerId == 0,
                        "Can only set CrossGroupOpenerId once");
  mCrossGroupOpenerId = aOpenerId;
}

ここから、メンバー変数 mCrossGroupOpenerId の参照箇所や、このメンバー変数のゲッターである SetCrossGroupOpenerId() メソッドの呼び出し箇所を辿ってみました。 すると、メンバー変数の参照箇所はセッターとゲッターだけで、ゲッターの呼び出し箇所は docshell/base/nsDSURIContentListener.cpp 内の以下の1箇所のみであると分かりました。

already_AddRefed<BrowsingContext>
MaybeCloseWindowHelper::ChooseNewBrowsingContext(BrowsingContext* aBC) {
  RefPtr<BrowsingContext> opener = aBC->GetOpener();
  if (opener && !opener->IsDiscarded()) {
    return opener.forget();
  }

  if (!XRE_IsParentProcess()) {
    return nullptr;
  }

  opener = BrowsingContext::Get(aBC->Canonical()->GetCrossGroupOpenerId());
  if (!opener || opener->IsDiscarded()) {
    return nullptr;
  }
  return opener.forget();
}

このメソッドの詳細はよく分かっていませんが、コードを見ると、親にあたるページがアンロード済みのときは特に何もせず終了するように書かれています。 つまり、「あるタブから子タブが開かれた後で、親タブの側がアンロードされていた場合には、この部分では何もしないようになっている」と言うことができ、「アンロード済みのタブに対して子タブが開かれた場合でも、あるタブから子タブが開かれた後で親タブの側がアンロードされたのと同等に扱われるようだ」「今回の修正の結果タブを開く処理の中で setCrossGroupOpener() を呼ばなくなったとしても問題は無さそうだ」と推測することができます。

とはいえ、それでもまだ推測だけで、確信は持てません。 このような場合、筆者は「その箇所を元々実装した人」に質問してみるようにしています。 この段階ではまだパッチの作成やレビュー依頼をしていないので、今回はBMO（bugzilla.mozilla.org）でのコメントでメンションしてみるのがよさそうです。

その箇所を誰が実装したかは、とりあえずblameの情報を参照してみることにします。 Searchfoxでは、検索結果としてファイルの内容を表示しているときに、行番号の左のグレーの領域にポインターを載せると、その行の由来となったコミットの詳細が表示されます。 これを見ると、このメソッドの作者はNika Layzellさんという方だと分かりました。

Nikaさんが今回の問題の詳細を把握しているとは限らないため、背景事情の簡単な説明も含める形で質問文を以下のように考えて、「Request information from」欄にNikaさんのメールアドレスを入力する形でBMO上でコメントしました。

Just skipping CanonicalBrowsingContext.setCrossGroupOpener() for null openerBrowser.browsingContext looks reasonable for me, because the only one code using the returned value of GetCrossGroupOpenerId() looks to be here and the function is designed to do nothing when the opener tab is already discarded. Is it a correct approach?

（空の openerBrowser.browsingContext に対して CanonicalBrowsingContext.setCrossGroupOpener() の呼び出しを省略することは妥当に思われます。なぜなら、GetCrossGroupOpenerId() の戻り値を使用している箇所は唯一ここだけで、この関数は親のタブがアンロード済みの時は何もしないように設計されているからです。この修正方針は妥当でしょうか？）

すると、数時間後に以下のような返信を頂くことができました。

Skipping that call if the opener browser is not available seems very reasonable, yes.

（はい、親タブが利用できないときにこのメソッドを呼ぶ処理をスキップするのは妥当に思われます。）

これで、自身を持ってこの修正を提案することができます。

修正を提案可能な状態に仕上げる

修正がうまくいき、方針としても問題無いことを確認できました。 次は、この変更をFirefoxの修正として提案することを見据えて、変更内容をブラッシュアップしてみます。

変更の結果が「手元で動作を確認できている」だけでは、提案として不充分です。 オープンソースの開発プロジェクト全般で言えることですが、変更を提案する際は以下の条件を満たす必要があります。

1. 既存の実装の方針に沿った設計になっている。
2. 既存の実装とコーディングスタイルが一致している。
3. 提案する変更はライセンス面で問題がない。
4. 修正が機能していることを検証する自動テストを用意できている。

1を満たすには、周辺の実装をよく見て設計を理解する必要があります。 今回の変更のように条件文を1つ足す程度でも、「その場所でその判定を行うのは妥当か？」といった点をよく考えないといけません。

2は、Firefoxの場合はコードフォーマッターが提供されているので、それを使うようにします。 リポジトリーの最上位ディレクトリーで ./mach lint --fix ファイルやディレクトリーへのパス （今回なら ./mach lint --fix browser/base/content/tabbrowser.js）というコマンド列を実行すると、文法エラーのような静的に確認可能なミスが無いかが確認され、Mozillaのコーディングスタイルに沿うようにコードが自動的に更新されます。 VSCodeでのファイルの保存時に自動的にフォーマッターを実行する設定も用意されているので、VSCodeを使っている人は是非設定しておきましょう。

3は、今回のように自分で考えて行った1行だけの変更のようなケースではあまり関係ないのですが、既存の他のプロジェクトから関数やモジュールの単位で実装を拝借したときに気をつけなくてはなりません。 Firefoxのソースコード全体はMozilla Public License 2.0（MPL2）でライセンスされており、MITライセンスやBSDライセンスのようにMPL2と矛盾しないライセンスのプロジェクトのコードであれば問題無く持ち込めます⁵が、矛盾する条項を持つライセンスのコードを持ち込むことはできません。

4は、いわゆる回帰テストの追加です。 Firefoxの開発には非常に多くの人が関わっており、自分がせっかく直した部分も、他の人による変更でまた動作しなくなる恐れがあります。 そのような後退バグの発生を予防する⁶ため、変更の提案には「行った変更の結果期待される動作」を検証する自動テストも含めなくてはなりません。 これは次節で詳述します。

回帰テストを追加する

Firefoxの回帰テストは大抵、テスト対象の実装があるディレクトリーの配下の test ディレクトリー内に置かれています。 今回変更した browser/base/content/tabbrowser.js は、browser/base/content/test/ にカテゴリーごとに分けて大量のテストが置かれていました。 この中から、行った変更に関係していそうなディレクトリーにある似た趣旨のテストを探し、それを参考にテストを追加します。

既に同趣旨のテストがあるなら、その中に条件文や検証項目を足すだけでもよいですが、今回は general 配下に browser_bug1762249.js という名前⁷で以下のテスト⁸を追加してみました。

add_task(async function () {
  // 他のテストでも使われていたユーティリティ関数を使って、
  // テスト用にブラウザーのウィンドウを新たに開く
  const win = await BrowserTestUtils.openNewBrowserWindow();

  // 内部APIを使って、アンロードされた状態のタブを開く
  const discardedOwnerTab = win.gBrowser.addTab("", {
    createLazyBrowser: true,
    triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
    allowInheritPrincipal: true,
  });
  // そのタブを親のタブとして、もう1つタブを開く
  const childTab = win.gBrowser.addTab("about:blank", {
    ownerTab: discardedOwnerTab,
    openerBrowser: discardedOwnerTab.linkedBrowser,
    triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
    allowInheritPrincipal: true,
  });

  // タブが開かれたことを確認する
  ok(
    childTab,
    "child tab is successfully opened as a child of a discarded owner"
  );

  // 他のテストでも使われていたユーティリティ関数を使って、
  // テスト用に開いたブラウザーのウィンドウを閉じる
  await promiseWindowClosed(win);
});

修正の発端になったのはWebExtensions API経由での操作でしたが、修正の対象は「WebExtensions APIが呼ばれたときにFirefoxの内部的なAPIを使ってタブを開く処理」ではなく、そこで呼び出されている「タブを開くためのFirefoxの内部的なAPI」の部分だけだったため、今回はこのように内部的なAPIのレベルでのテストとしてみました。

新たなテストのファイルを追加した場合、そのテストは自動的には実行されません。 CIで実行してもらえるように、テストは同じディレクトリー内のINIファイルに明示的に列挙する必要があります。 今回であれば、テストが browser/base/content/test/general/browser_bug1762249.js にあるため、登録のために編集するINIファイルは、同じ階層の browser/base/content/test/general/browser.ini となります。

このファイルをエディターで開くと、末尾の方に以下のように書かれていました。

[browser_newwindow_focus.js]
https_first_disabled = true
skip-if = os == "linux" && !e10s # Bug 1263254 - Perma fails on Linux without e10s for some reason.
# DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
[browser_bug1299667.js]
https_first_disabled = true
# DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.

「ここにこれ以上テストを追加するな、個別のディレクトリーを使用せよ」というコメントが気になりますが、他にも同じ記述が多数あり、どう従えばよいか判断が付かなかったため、とりあえず他のテストを真似して、末尾に以下の行を加えてみました。

[browser_bug1762249.js]
# DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.

（なお、次の記事で書きますが、案の定これはパッチ提出後のコードレビューで「ここに追加しないでって書いてあるのになんで無視してるの？ tabs ディレクトリーの中に、Bugの番号ではなくテスト対象の挙動を説明するファイル名を付けてテストを置いて」と指摘を受けてしまいました。既存のコードの中には「歴史的経緯でこうなっているが、次に新しくやるときは、このやり方を踏襲せず正しいやり方に改めないといけない」という部分があり、今回はそういうケースでした。）

テストを追加したら、実際にそのテストを動かしてみます。 リポジトリーの最上位ディレクトリーで ./mach test テストファイルまたはディレクトリーのパス を実行すると、指定したテストをファイルやディレクトリー単位で実行することができます。 今回の事例であれば ./mach test browser/base/content/test/general/browser_bug1762249.js です。 テストの実行にはFirefoxのビルド済みバイナリーが必要なので、まだしていないのであれば、先にArtifact Modeでビルドを行っておきましょう。

まとめ

以上、Firefoxの不具合の原因を特定して自分の手元で修正してみた事例について、実際に行った調査の進め方や修正方針の確認の仕方、提案を見据えての修正内容の仕上げ方をご紹介しました。

ソフトウェア開発者であっても、有名なオープンソース製品に触れるときは、ともすれば「お客さんとして使うだけ」になってしまいがちでしょう。 しかし、FirefoxやVSCodeのようにJavaScriptベースで開発されていて開発ツールも内蔵しているアプリケーションでは、この事例のように、アプリケーションや拡張機能の内部で発生したエラーの詳細をその場で辿って調べることができます。 また、サーバー上で動作するWebサービス型のアプリケーションであれば、ログに表れたスタックトレースを手がかりにして、npmやpipやgem等で導入された依存ライブラリー内で発生したエラーの原因を辿ることもできます。 オープンソースのソフトウェアの不具合修正に関わってみる最初の題材として、皆さんも是非、自分が遭遇した不具合の原因調査に取り組んでみて下さい。

次回記事では、シリーズ完結編として、ここで仕上げた変更内容をパッチとして提出してレビューを受ける手順をご紹介します。 乞う御期待！

株式会社クリアコードは、FirefoxやThunderbirdの法人運用において生じた様々な問題について、ソースコードレベルの調査も含めたサポートを行うサービスを有償でご提供しています。 また、FirefoxやThunderbird自体の不具合が原因だった場合、開発元に報告を行い、可能な場合は修正の提案も行っています。 FirefoxやThunderbirdの運用で何かお困りの企業の運用担当者さまは、お問い合わせフォームよりお問い合わせ下さい。

ツイートする

フォローする