DroidKaigi 2024 Code Bug Fix Challenge 2問目

こんにちは。AndroidアプリエンジニアのChigitaとIkedaです。

先日開催されたDroidKaigi 2024のLINEヤフー企業ブースでは、「Code Bug Fix Challenge」を実施しました。

「Code Bug Fix Challenge」とは、問題コードに含まれるバグを見つけ、そのバグを修正してもらったり、将来のバグを防ぐためにはどんなテストコードを書けばいいのかを、来場者に考えてもらう企画です。

ブースに来ていただいた方には意見を書いた付箋を貼ってもらい、その意見をもとにLINEヤフーの開発者と交流しました。

本記事では、今回出題した「Code Bug Fix Challenge」の2問目の解説をします。

Out of sight, out of mind.

出題コード

Fragment内でのWebViewの実装です。
WebViewの振る舞いはWebViewClientで実装しています。
WebView内のリンクをクリックしたときには、URLパラメータを削除したURLを端末のデフォルトブラウザで開く実装になっています。
また、購読しているViewModel内のuiStateが更新されるたびに、WebViewでURLを読み込むようになっています。

class MainFragment : Fragment() {
    private lateinit var viewModel: MainViewModel
 
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        val webView = view.findViewById<WebView>(R.id.webView)
        webView.webViewClient = MyWebViewClient()
        lifecycleScope.launch {
            repeatOnLifecycle(Lifecycle.State.STARTED) {
                viewModel.uiState.collect { uiState ->
                    webView.loadUrl(uiState.url)
                }
            }
        }
    }
 
    inner class MyWebViewClient : WebViewClient() {
        var url: String = ""
 
        override fun shouldOverrideUrlLoading(view: WebView, url: String?): Boolean {
            // If the property value is not an empty or blank string, load it instead of the given URL.
            if (url != "") {
                loadUrlInDefaultBrowser(view, removeQueryParameters(url!!))
            } else {
                loadUrlInDefaultBrowser(view, removeQueryParameters(url))
            }
            return false
        }
 
        override fun onPageFinished(view: WebView?, url: String?) = onLoadCompleted()
 
        override fun onReceivedError(
            view: WebView?, request: WebResourceRequest?, error: WebResourceError?
        ) = onLoadFailed()
 
        override fun onReceivedHttpError(
            view: WebView?, request: WebResourceRequest?, error: WebResourceError?
        ) = onLoadFailed()
 
        // Returns URL that query parameters are removed.
        // For example, if the given URL is "https://example.com?param1=value1&m2=value2",
        // this function returns "https://example.com".
        private fun removeQueryParameters(originalUrlString: String): String {
            var shouldRemove = false
            var urlWithoutParam = ""
            originalUrlString.forEach { char ->
                if (shouldRemove) {
                    urlWithoutParam += ""
                }
                if (char == '?') {
                    shouldRemove = true
                }
                urlWithoutParam += char
            }
            return urlWithoutParam
        }
 
        fun updateUrl(url: String) {
            this.url = url
        }
    }
 
    private fun loadUrlInDefaultBrowser(view: WebView, url: String) {
        // Open given url with default browser.
        val intent = Intent(Intent.ACTION_VIEW, Uri.parse(url))
        startActivity(intent)
    }
 
    private fun onLoadCompleted() {
        // Set webView load success flag to `true`.
        viewModel.updateWebViewLoadSuccessFlag(true)
    }
 
    private fun onLoadFailed() {
        // Set webView load success flag to `false`.
        viewModel.updateWebViewLoadSuccessFlag(false)
    }
}

出題意図

独自のWebViewClientが関連するようなコードを書く際に、どのように、Fragment、WebViewClient そして ViewModel の責務を分割するかについてを議題の中心において出題しました。

ブースでいただいたコメント

多くのコメントをいただきました。いくつかピックアップします。

強制アンラップを行っているため、状態に応じてクラッシュする危険性がある。
null 安全にするべき / 早期リターンするべき
removeQueryParametersメソッドのロジックが間違っているため、パースされる値が意図しないものになっている。
shouldOverrideUrlLoadingの戻り値について、falseを返すとwebviewの中でURLを読み込んでしまう。
inner class である MyWebViewClient が Fragment の参照を持っているため、onDestroyView で webViewClient への参照を切らなければメモリリークが発生する

議論した内容

いただいたコメントをもとに、当日その場で議論したことをいくつかピックアップします。

webviewClient の中にロジックが一部含まれており、クラスの責務が不明瞭になっている。クラスの責務の不明瞭さが、テストの書きづらさにつながることがある。
UnitTest を書くことで、ロジックに関するバグは発見できる。
URL パラメータを除去するような実装は独自で再実装するのではなく、標準ライブラリを用いて実装する方が潜在的な問題を減らすことができる。
WebViewを扱うのはやや難しいが、それでも Espresso-Web などを用いることで、UI Test の形でもテストを記述できる。多くの部分の実装を UnitTest で保証しつつ、UI Test の導入も考えた方がよい。

ボードの写真

解説

出題したコード上のバグを説明します。

override fun shouldOverrideUrlLoading(view: WebView, url: String?): Boolean {
    // If the property value is not an empty or blank string, load it instead of the given URL.
    if (url != "") {
        loadUrlInDefaultBrowser(view, removeQueryParameters(url!!))
    } else {
        loadUrlInDefaultBrowser(view, removeQueryParameters(url))
    }
    return false
}

override fun onPageFinished(view: WebView?, url: String?) = onLoadCompleted()

shouldOverrideUrlLoading()の引数であるurlはnullableだが、loadUrlInDefaultBrowser()に渡すときに強制アンラップしている

このままでもコンパイルエラーとはなりませんが、nullの値が渡されたときにNullPointerExceptionとなってしまいます。
nullableな値を扱うときはnullチェックを入れましょう。

if(url != "")でプロパティのurlが空文字かどうかの判定をしたいが、返り値のurlの方を参照してしまっている

コメントにIf the property value is not an empty or blank string, load it instead of the given URL.とあるように、ここではプロパティのurlが空文字かどうかの判定をしたいです。
しかしこのifの中で確認しているurlはshouldOverrideUrlLoadingの引数を参照してしまっているためプロパティの値を参照できていません。

WebView内で遷移先URLを開いていないので、返り値はtrueを指定するべきである

shouldOverrideUrlLoadingでは、同じWebViewで読み込みたいときはfalse、読み込みたくないとき（外部のブラウザで開くなど）はtrueを返す必要があります。
ここで呼び出しているloadUrlInDefaultBrowser()の関数名や実装内容を見ると、端末のデフォルトブラウザで開いていることがわかります。
よってここでは返り値にtrueを指定するべきです。

onPageFinished() ＝読み込み成功時の処理を書く場所　ではない

WebViewClientのonPageFinished()は、WebViewがコンテンツの読み込みに完了した場合に呼ばれます。
これは読み込み成功時に呼ばれるのはもちろんのこと、読み込み失敗時にも呼ばれることに注意しなくてはいけません。
例えば404になるページを読み込んだ場合、onReceivedError() → onPageFinished()の順番で呼ばれます。
ですのでこのコードでは、WebViewは読み込みに失敗しているのに、アプリ内の読み込み成功フラグはtrueになってしまう不整合が起きます。
WebViewの読み込み成功/失敗の状態を管理したいときには、成功フラグとは別に失敗フラグを持つことを考えましょう。

lifecycleScope.launch {
    repeatOnLifecycle(Lifecycle.State.STARTED) {
        viewModel.uiState.collect { uiState ->
            webView.loadUrl(uiState.url)
        }
    }
}

this.lifecycleScopeを使用しているが、onDestroy()のタイミングが違うため、viewの方のlifecycleScopeを使用するべき

現在の構造だと view が破棄された後にもビューに関連する操作が可能なため、メモリリークやクラッシュの可能性があります。
解決策としてthis.lifecycleScopeの代わりに、 viewLifecycleOwnerを使用しましょう。

mutableStateFlowのcollectとWebViewのurlロードについて

repeatOnLifecycle内でmutableStateFlowであるuiStateをcollectし、uiState内の値が変わるたびにurlをWebViewが読み込んでいます。
urlの値が変わったタイミングでWebViewに読み込ませる、というのが理想的でありますが、ここではuiState内のurl以外の値が変わってもWebViewが再読み込みされてしまいます。
その場合、次のような懸念があります。
例えば読み込むWebページ側で閲覧数のログを計測している場合、計測値が不正確になってしまいます。
また、ユーザーが閲覧途中でuiStateの値が変わった場合、再読み込みによって一番上までスクロールされてしまうかもしれません。
collectしている値をWebViewで読み込みさせたいときには、WebViewで読み込み中のURLと読み込みたいURLが異なる場合に読み込みさせるようにする必要があります。

private fun removeQueryParameters(originalUrlString: String): String {
    var shouldRemove = false
    var urlWithoutParam = ""
    originalUrlString.forEach { char ->
        if (shouldRemove) {
            urlWithoutParam += ""
        }
        if (char == '?') {
            shouldRemove = true
        }
        urlWithoutParam += char
    }
    return urlWithoutParam
}

処理の順番が正しくない

ここではコメントに書いてあるとおり、引数のURLに含まれるURLパラメータを削除したものを返す関数となっています。
一見、一文字ずつURLを変数に追加していき、"?"が来たらフラグを立ててそれ以降の文字を追加しない処理になっており正常に動作するように見えます。
しかし、if (shouldRemove)の処理の後にif (char == '?')の判定があるため、変換処理後のURLに"?"が残ってしまいます。
また、if (shouldRemove)の中でurlWithoutParam += ""の後にreturnしてあげないと、結局最後のurlWithoutParam += charで文字が追加されてしまいます。
よって、if (char == '?')の判定を最初にして、if (shouldRemove)の判定内にreturn@forEachを追加することで正常に動作します。
一方で、URLのハンドリングは独自で実装するよりもUri型に変換して操作した方が簡単かつ安全でしょう。

想定解答

修正コード

混入させた不具合を修正したコードです。

class MainFragment : Fragment() {
    private lateinit var viewModel: MainViewModel
 
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        val webView = view.findViewById<WebView>(R.id.webView)
        webView.webViewClient = MyWebViewClient()
        viewLifecycleOwner.lifecycleScope.launch {
            repeatOnLifecycle(Lifecycle.State.STARTED) {
                viewModel.uiState.collect { uiState ->
                    if(webView.url != uiState.url){
                        webView.loadUrl(uiState.url)
                    }
                }
            }
        }
    }
}
 
class MyWebViewClient(val viewModel: MainViewModel) : WebViewClient() {
    var url: String = ""

    override fun shouldOverrideUrlLoading(view: WebView, url: String?): Boolean {
        // If the property value is not an empty or blank string, load it instead of the given URL.
        if (this.url != "") {
            loadUrlInDefaultBrowser(view, removeQueryParameters(this.url))
        } else {
            loadUrlInDefaultBrowser(view, removeQueryParameters(url))
        }
        return true
    }

    override fun onPageStarted(view: WebView?, url: String?, favicon: Bitmap?) {
        viewModel.updateWebViewLoadCompletedFlag(false)
        viewModel.updateWebViewLoadFailedFlag(false)
    }

    override fun onPageFinished(view: WebView?, url: String?) = onLoadCompleted()

    override fun onReceivedError(
        view: WebView?, request: WebResourceRequest?, error: WebResourceError?
    ) = onLoadFailed()

    override fun onReceivedHttpError(
        view: WebView?, request: WebResourceRequest?, error: WebResourceError?
    ) = onLoadFailed()

    override fun onReceiverdSslError(
        view: WebView?, request: WebResourceRequest?, errorResponse: WebResourceResponse?
    ) = onLoadFailed()


    fun updateUrl(url: String) {
        this.url = url
    }

    // Returns URL that query parameters are removed.
    // For example, if the given URL is "https://example.com?param1=value1&m2=value2",
    // this function returns "https://example.com".
    private fun removeQueryParameters(originalUrlString: String): String {
        var shouldRemove = false
        var urlWithoutParam = ""
        originalUrlString.forEach { char ->
            if (char == '?') {
                shouldRemove = true
            }
            if (shouldRemove) {
                urlWithoutParam += ""
                return@forEach
            }
            urlWithoutParam += char
        }
        return urlWithoutParam
        
        // Uriを使った方が簡単かつ安全です
        // val uri = Uri.parse(originalUrlString)
        // val builder = uri.buildUpon().clearQuery()
        // return builder.build().toString()
    }

    private fun loadUrlInDefaultBrowser(url: String) {
        // Open given url with default browser.
        val intent = Intent(Intent.ACTION_VIEW, Uri.parse(url))
        startActivity(intent)
    }

    private fun onLoadCompleted() {
        // Set webView load success flag to `true`.
        viewModel.updateWebViewLoadCompleteFlag(true)
    }

    private fun onLoadFailed() {
        // Set webView load success flag to `false`.
        viewModel.updateWebViewLoadFailedFlag(true)
    }
}

テストについて

不明確なクラスの責任によってテストが書きにくい構造になっている

この問題には、大きくFragment、ViewModel そして、WebViewClient の3つの登場人物が現れます。これらのクラスの責任が明確でなく、それぞれが不適切な形でロジックを持っているのが多くのバグの原因だと言えます。
このように責任が明確でないクラス構造は、テストを書きにくくします。

例えば、MyWebViewClient を inner class ではなく別ファイルで定義するクラスとしてみましょう。
onLoadCompleted や onLoadFailedは外から注入する形で依存関係を切り出すことができるため、MainFragment や MainViewModel との隠された依存関係を切り出しつつ、テストを書きやすい形になります。
そうすると、今回の問題に多く埋め込まれていた MyWebViewCilent のバグは事前にユニットテストで検知できるでしょう。

おわりに

「Code Bug Fix Challenge」の2問目について解説しました。

この記事では、WebViewを含む処理で陥りやすい問題点その解決策を紹介しました。クラスの責務を明確にし適切な粒度で切り出すことでテストの記述を容易にし、問題を事前に検知できることを示しました。また、WebViewClient にはさまざまな設定項目があるためこれらについても正しい知識が必要であることも紹介しました。

また後日、3問目以降の出題についても解説記事を投稿予定です。お楽しみに！