はじめまして。プロダクト開発チームの小野寺 (ryoqun)です。
Google Chrome (以下、Chrome)にて、HTMLのレンダリングの回帰バグが紛れ込み、その影響でSPEEDAの一部分のレイアウトが崩れてしまう問題が発生しました。そこで、「Chrome hacking」と称し、数名の希望者を社内で募り、みんなでこのバグを調査、あわよくば解決しChromeのコミットログに@uzabase.comのドメインを刻もうと奮い立ちました。
しかし結論として、別の案件が入り、作業を中断している間に先を越され、名を刻むことはできませんでした。つまりは現在このバグは別の開発者によって修正が完了しています。しかし、結果的にはOSSのソースコードレベルでの調査の実例としては非常に好例となりました。
その活動記録として、SPEEDA上での問題の発覚からChromeのバグであるという原因の特定や調査から収束に至るまでの一連の出来事を共有したいと思います。
- 前提として、本当にChromeのレイアウトバグでした
- バグの発覚と内容
- とりあえず応急処置
- ミニマルテストケースの作成
- Chromiumのビルド
- 実際にHackして怪しい箇所を見つける
- コミットの特定
- いざ修正！(は叶わず…)
- コードリーディング
- 感想
- まとめ
前提として、本当にChromeのレイアウトバグでした
最初はChromeのバグだと断言できませんでした。
そのため、本当にChromeがCSS 2.1のレイアウト回帰バグを混入させてしまったことが原因だと分かった時は驚きでした。
当然として、SPEEDAでのレイアウトバグの発覚直後は、SPEEDAのCSSの問題だと考えていました。 というのもCSSは呆れるくらいに枯れたバグの入る余地のないWebの基礎技術だからです。
CSS 2.1として2011年6月にW3C Recommendationとなり、それから5年以上が経過しています。それまでの歩みは決して容易いものではなかったため、CSSは鉄壁の仕様となっています(1990年代の血みどろのブラウザ競争の中で産み落とされたCSS 1.0がのたうちまわり、当時のWebエンジニアたちをInteroperatabilityの名の下苦悩させ、戦禍の反省とでもいうかのごとく「複数レンダリングエンジン上で実装済み」という大義名分の元、W3CによってひねりだされたCSS 2.0が2000年代を通し、これでもかというくらいに精緻に策定され、晴れてCSS 2.1は生み出されました)。
培われた仕様の厳密性、テストケースの網羅性はもはや芸術レベルで、1つの仕様に対しての手厚さとしては数有る仕様の中でもトップクラスにCSSは位置すると思います(特に個人的には9 Visual formatting modelや10 Visual formatting model detailsあたりは傑作だと思います)。
ということで、2017年の今日において、枯れたCSSに対し、IE 6やガラケーと戯れて涙を飲みながらレイアウトバグの回避を模索していた苦悩の2000年代を彷彿とさせる事象に再び直面し、非常に印象的でした。 どんなに枯れていようが常にソフトウェアにはバグがつきものであり、バグに直面した時、時には自分たちのコードだけでなくミドルウェアも疑う必要性を痛感しました。 また、CSSを正しく実装することがいかに難しいことであるかの証左なのかもしれません。
バグの発覚と内容
今回のバグは、SPEEDAの本番環境にて、デスクトップ向けのChromeのStableチャンネルに59が出始めてからようやく気づきました。
バグの内容は、サイト検索フォームの下に表示されるサジェスト候補の一覧が異様に高くなってしまうというものでした。SPEEDAは一般公開されているサービスではないので見せられるスクリーンショットがかなり限定的でわかりにくいのですが、正しいレンダリング時の画像はこのようになります：
逆に、正しくないレンダリング時の画像は、このようにかなり縦長な感じになってしまいます：
これだけだとイメージがつきにくいのですが、サジェスト候補が画面表示領域に対してかなりの占有率になってしまい、ユーザーにも違和感を与えるレベルになってしまいました。
とりあえず応急処置
他のブラウザや以前のバージョンのChromeでは問題が起きなかったことから、どう考えてもChromeのバグらしいというのが判明してきました。そうなってくるとChromeはすぐには修正されないのでまずは応急処置です。 レンダリングエンジンがどう動いているかを想像しつつ、クロスブラウザで無害で等価なCSSを色々と試行錯誤した結果、結局は以下の変更だけで直ってしまいました。
.g-search-suggest li .suggestItem {
- display: block;
+ display: inline-block;
padding-left: 90px;
color: #555;
CSS的にはほとんど等価なはずなので、やはりどう考えてもChromeのバグのようでした(ちなみに、こういうレイアウトバグの回避策なんてものは、すっかり失われし技術となってしまいました)。
この応急処置をSPEEDAに反映し、次にChromeを直そうということになりました。
ミニマルテストケースの作成
ともかくも最初はミニマルテストケースを作りました。そうすることによって社内に公開しても大丈夫でGoogleにもバグレポートを送れるようになります。 作ったミニマルテストケースは↓の通りです。
<!DOCTYPE html> <html> <head> <style> .suggestItemOk1 { display: inline; } .suggestItemOk2 { display: inline-block; } .suggestItemNg { display: block; } .item { overflow: hidden; display: inline-block; } </style> </head> <body> <ul> <li><span class="suggestItemOk1"><span class="item">AAA</span></span></li> <li><span class="suggestItemOk1"><span class="item">BBB</span></span></li> <li><span class="suggestItemOk1"><span class="item">CCC</span></span></li> <li><span class="suggestItemOk2"><span class="item">AAA</span></span></li> <li><span class="suggestItemOk2"><span class="item">BBB</span></span></li> <li><span class="suggestItemOk2"><span class="item">CCC</span></span></li> <li><span class="suggestItemNg"><span class="item">AAA</span></span></li> <li><span class="suggestItemNg"><span class="item">BBB</span></span></li> <li><span class="suggestItemNg"><span class="item">CCC</span></span></li> </ul> </body> </html>
ミニマルテストケースの正しいレンダリング時の画像はこうなります:
逆に、正しくないレンダリング時の画像はこうなります：
SPEEDA上でのレイアウトバグとなんとなく似ているのは想像できるかと思います。
このミニマルテストケースから分かることは、display: list-itemとoverflow: hidden,display: blockが組み合わさるとどうやらまずいということです。その情報を元にChromiumのバグを検索してみましたが、同様のバグが見当たらなかったため、自分たちで直してみようということになりました(ちなみに、今現在はこのキーワードで検索すると、今回の回帰バグのレポートを見つけることができます)。
Chromiumのビルド
Chromeは、オープンソースであるChromiumから作られています。そこでオープンソースの真価を発揮ということで、手元のマシンでビルドしてみました。Chromiumは相当な数のサードパーティーライブラリに依存していますが、独自ツール(gyp)を使って比較的簡単にビルド環境を構築できます。ただストレージ容量は結構必要で、例えば私の場合は50GBは必要でした。
また、今回は回帰バグなのでChromium 58とChromium 59のどちらも並行させてビルドし、比較調査しやすいようにしました。
実際にHackして怪しい箇所を見つける
ChromeのDeveloper Toolsから得られる情報だけではレイアウトバグの状況が分からなかったので、ブラウザの真骨頂であるレンダリングの中のレイアウト(Reflow)コードを読む必要があります。大抵の大規模ソフトウェアは開発目的で色々な内部状態をダンプする機能があり、Chromiumも例外ではありません。ですが、今回の参加メンバーはChromeにはそれほど詳しくないため、当初はそのやり方が分からず、ソースコードとWebと変更履歴をつっつきまわり、最終的にはデバッグ情報を出力させることができました。
具体的には下のように、デバッグ関数をよく通るであろうコードパスから呼び出してみました。
diff --git a/third_party/WebKit/Source/core/layout/LayoutListItem.cpp b/third_party/WebKit/Source/core/layout/LayoutListItem.cpp index 92af305..946e7c9 100644 --- a/third_party/WebKit/Source/core/layout/LayoutListItem.cpp +++ b/third_party/WebKit/Source/core/layout/LayoutListItem.cpp @@ -466,6 +466,10 @@ void LayoutListItem::PositionListMarker() { void LayoutListItem::Paint(const PaintInfo& paint_info, const LayoutPoint& paint_offset) const { + this->ShowTreeForThis(); + this->ShowLayoutTreeForThis(); + this->ShowLineTreeForThis(); + //this->ShowDebugData(); ListItemPainter(*this).Paint(paint_info, paint_offset); }
そして、レイアウトバグの有無によって、↓のような違う2つのレイアウトツリーが生成されていることが分かりました。
正しいレイアウトツリー:
LayoutListItem 0x19a65d2243d0 LI
LayoutListMarker (anonymous) 0x19a65d218df0
LayoutBlockFlow 0x19a65d218f18 SPAN class="suggestItemOk2"
LayoutBlockFlow 0x19a65d219040 SPAN class="item"
LayoutText 0x19a65d2415f0 #text "CCC"
LayoutListItem 0x19a65d224290 LI
LayoutBlockFlow 0x19a65d218828 SPAN class="suggestItemNg"
LayoutListMarker (anonymous) 0x19a65d2184b0
LayoutBlockFlow 0x19a65d218cc8 SPAN class="item"
LayoutText 0x19a65d241528 #text "AAA"
正しくないレイアウトツリー(suggestItemNgのLayoutListMarkerが外出しされてしまっている):
LayoutListItem 0x19a65d224650 LI
LayoutListMarker (anonymous) 0x19a65d219168
LayoutBlockFlow 0x19a65d219290 SPAN class="suggestItemOk2"
LayoutBlockFlow 0x19a65d2193b8 SPAN class="item"
LayoutText 0x19a65d240ee8 #text "CCC"
LayoutListItem 0x19a65d224790 LI
LayoutBlockFlow (anonymous) 0x19a65d218a78
LayoutListMarker (anonymous) 0x19a65d219608
LayoutBlockFlow 0x19a65d219e20 SPAN class="suggestItemNg"
LayoutBlockFlow 0x19a65d219cf8 SPAN class="item"
LayoutText 0x19a65d241910 #text "AAA"
正しくないレイアウトツリー中で、アドレスが0x19a65d218a78のLayoutBlockFlowが余計に生成されています。これによって余計な論理的な行が追加され、意図せず高さがおかしくなってしまうというからくりのようでした。
ここまでくればもう峠を越していて、あとはこの差異をとことん調べ込んでいけばよくなります。
今まではまったくの五里霧中で、どこにバグがあるのか分からず怪しそうなところをとにかく広く浅く探す必要がありました。胸をなでおろせた瞬間でした。
コミットの特定
<li>関連の実装が肝になっているようだったので、LayoutListItemのソースコードを入念に見ました。調べた結果、回帰バグを混入させたコミットを特定することができ、それをgit revertしてビルドし直したらバグが発生しなくなりました！
いろいろな切り口で調べたのですが、結果的にはgit annotateが決め手でした。比較的浅い回帰バグにはgit annotateは有効です。
変更内容としては非常に小さいです。ちなみにこの変更を見てみると、もともとはまた別のレイアウトのバグを直そうとしていたようです。
diff --git a/third_party/WebKit/Source/core/layout/LayoutListItem.cpp b/third_party/WebKit/Source/core/layout/LayoutListItem.cpp index 6c98974..4dbf2a7 100644 --- a/third_party/WebKit/Source/core/layout/LayoutListItem.cpp +++ b/third_party/WebKit/Source/core/layout/LayoutListItem.cpp @@ -257,6 +257,11 @@ static LayoutObject* getParentOfFirstLineBox(LayoutBlockFlow* curr, if (currChild == marker) continue; + // Shouldn't add marker into Overflow box, instead, add marker + // into listitem + if (currChild->hasOverflowClip()) + break; + if (currChild->isInline() && (!currChild->isLayoutInline() || curr->generatesLineBoxesForInlineChild(currChild)))
いざ修正！(は叶わず…)
直そう！と思って一旦保留していたら、先を越され、その間にupstreamで修正されてしまいました。非常に残念です。
修正に必要なコードはたったの一行でした。もともとが2行を追加しただけで回帰バグが発生したのですから、その2行のどちらかを直せば回帰バグは直るというわけです。
diff --git a/third_party/WebKit/Source/core/layout/LayoutListItem.cpp b/third_party/WebKit/Source/core/layout/LayoutListItem.cpp index 18e98a78..893ee6e 100644 --- a/third_party/WebKit/Source/core/layout/LayoutListItem.cpp +++ b/third_party/WebKit/Source/core/layout/LayoutListItem.cpp @@ -259,7 +259,7 @@ static LayoutObject* GetParentOfFirstLineBox(LayoutBlockFlow* curr, // Shouldn't add marker into Overflow box, instead, add marker // into listitem - if (curr_child->HasOverflowClip()) + if (curr->HasOverflowClip()) break; if (curr_child->IsInline() &&
コードリーディング
今回の一連の修正で問題なのは、ListMarkerをLayout Treeに配置する場所です。
回帰バグの発生前後に関わらず、大前提としてLayoutBlockFlow(class=suggeestItemNg)の子としてListMarkerを追加する必要があります。しかし、その前提が1つ目の修正で崩れてしまいました。回帰バグによりLayoutListItem(<li>)の子として追加するように意図せず実装が変わってしまいました。
その原因を少し説明します。
まず、1つ目の修正で本当に直したかったことは、<li>にoverflow: hiddenな子要素があるとき、その子としてListMarkerを追加するとclipされ、表示されないという問題でした。
その場合はListItemの子としてListMarkerを入れる必要があります。なので1つ目の修正ではそういうロジックをGetParentOfFirstLineBox()に新規に追加しました。
具体的には、特定条件時にGetParentOfFirstLineBox()からはbreak経由でnullptrを返し、呼び元であるUpdateMarkerLocation()がListItemの子としてListMarkerを追加するというものです。しかし、その判定条件が正しくなく回帰バグが発生してしまいました。
ちなみに、この処理の副作用として改行が必然的に発生します(これがCSS的に正しいかは微妙です)。これは1つ目の修正としては許容するようですが、我々のミニマルテストケースでは許容されません。ミニマルテストケースの正しいレイアウトの挙動はLayoutBlockFlow(class=suggeestItemNg)の子としてListMarkerを追加することです。
追加した判定条件中でHasOverflowClip()が判定すべき対象はcurr(つまりはLayoutBlockFlow(class=suggeestItemNg))でありcurr_childではありません。まさに2つ目の修正ではそうなっています。
というのも、curr_childをどうこうというよりもまずはcurrがHasOverflowClip()でないならば、currはListMarkerの親として適切なので、currにListMarkerを追加すべきだからです。
上のミニマルテストケースは<span class="suggeestItemNg">の子として<span class="item">がいます。正しくない条件では、overflow: hiddenな<span class="item">が<span class="suggeestItemNg">の子要素となっているために、判定結果が誤って真になり、ListMarkerがListItemの子として追加されてしまいました。繰り返しますが、本来はoverflow: hiddenでないLayoutBlockFlow(class=suggeestItemNg)の子として追加すべきです。
結果、不要なLayoutBlockFlowができたことで論理改行が発生し、最終的には高さが意図せず高くなってしまうというレイアウト崩れが発生しました。
感想
参加したメンバーの感想です。
小野寺: 複数人でレイアウトロジックの動きを追うのは難しかったです。当社のSPEEDA開発グループではペアプロを積極採用しているのでペアプロの応用実践として何かいい解決案を考えてみたいと思いました。
北内: レンダリングエンジンのソースコードを追うのは骨の折れる作業でしたが、複数人で協力しながら作業したおかげで根気よく進めることができました。また、Appleと共同で開発していたWebKitからフォークしてBlinkに移行したことにともない、メンバ関数の名前をlower-camelcaseからupper-camelcaseに変更するといった変更履歴を見ることができたのも興味深かったです。
鈴木: Chromeがマルチプロセスで動いているからかデバッガでうまくプロセスにattachできなかったため、git grepとデバッグプリントを利用した最終的かつ原始的な手法でバグを調査しましたが、結果的に、これは思いの外有効な策となりました。また、複数人でバグ調査を行う場合、様々な視点・観点を得られ、またメンタル的にもメリットがあるので、機会があればおすすめしたいです。
久保: SREチームでインフラエンジニアとして普段業務をしているため、Chromeのバグ改修は自分には非常にハードルが高く、先輩方についていくだけで必死でしたが、Chromeのような超大なソースのバグの原因を特定する際に、どのようにあたりをつけていくのかについて少し掴めたように思います。今後のSREチームとしての業務に活かせると思いました（Uzabaseのinfraチームは今年の7月よりinfraチームからSREチームに変わり、4Q(10月)以降本格的にSREチームとしてサービス改善にコミットし、バグの改修やレスポンス改善などこれまでのインフラレイヤーにとどまらない業務範囲になります）。
まとめ
今回はソースコードレベルまでの調査を業務で行いました。当社では今後もOSSにも積極的に取り組んでいきたいと思います。Chrome内のソースコードが原因の修正までは特定できたのはよかったのですが、別件の案件が入り、Chrome hackingを一旦保留にしていたら、upstreamでその間に修正されてしまい惜しかったです(本来は自分たちでバグレポートを立てて、テストケース込みでパッチを提出しようとしていたのですが……)。
長くなりましたが、最後にまとめでこの記事を終わりたいと思います。
- 天下のGoogleのしかもChromeでさえも回帰バグが紛れ込んでしまうことがある。
- オープンソースだと簡単なバグは自分たちで調査＆修正はやろうと思えばできて、みんなでOSSに貢献できる。
- 弊社では、時にはミドルウェアへのソースコード調査＆解決も厭わない情熱あふれる問題解決大好きエンジニアを募集しています。
