From 5daa06f7dcd8b98fd98b4a7273f3093692c34209 Mon Sep 17 00:00:00 2001
From: Grigory Kruglov <grigory@kruglov.ca>
Date: Wed, 9 Mar 2022 14:31:24 -0800
Subject: [PATCH] For #23697: Avoid NPEs while pasting URLs from the clipboard

It's possible for clipboard contents to have changed in the time between we've constructed
the onClick listeners and when they're actually invoked. Code before assumed that 'text'
will be not-null, and would crash otherwise. With this change, we're capturing contents of
the clipboard before constructing onClick handlers (while also asserting correct behaviour
of 'ClipboardHandler').

With this change, we'll log an error if we were told that clipboard contains a URL, but immediately
after that check 'text' was actually null. This would indicate one of two things:
- issues within the ClipboardHandler
- changing contents of the ClipboardHandler within a tiny time window between our check and querying
---
 .../mozilla/fenix/utils/ToolbarPopupWindow.kt | 66 +++++++++++--------
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/app/src/main/java/org/mozilla/fenix/utils/ToolbarPopupWindow.kt b/app/src/main/java/org/mozilla/fenix/utils/ToolbarPopupWindow.kt
index 8c91eeb0bb..28b8378aed 100644
--- a/app/src/main/java/org/mozilla/fenix/utils/ToolbarPopupWindow.kt
+++ b/app/src/main/java/org/mozilla/fenix/utils/ToolbarPopupWindow.kt
@@ -22,6 +22,7 @@ import org.mozilla.fenix.components.metrics.Event
 import org.mozilla.fenix.ext.components
 import java.lang.ref.WeakReference
 import mozilla.components.browser.state.selector.findCustomTab
+import mozilla.components.support.base.log.logger.Logger
 import org.mozilla.fenix.databinding.BrowserToolbarPopupWindowBinding
 
 object ToolbarPopupWindow {
@@ -34,7 +35,8 @@ object ToolbarPopupWindow {
     ) {
         val context = view.get()?.context ?: return
         val clipboard = context.components.clipboardHandler
-        if (!copyVisible && !clipboard.containsURL()) return
+        val clipboardUrl = clipboard.getUrl()
+        if (!copyVisible && clipboardUrl == null) return
 
         val isCustomTabSession = customTabId != null
 
@@ -54,37 +56,41 @@ object ToolbarPopupWindow {
 
         binding.copy.isVisible = copyVisible
 
-        binding.paste.isVisible = clipboard.containsURL() && !isCustomTabSession
-        binding.pasteAndGo.isVisible =
-            clipboard.containsURL() && !isCustomTabSession
+        val showPaste = clipboardUrl != null && !isCustomTabSession
+        binding.paste.isVisible = showPaste
+        binding.pasteAndGo.isVisible = showPaste
 
-        binding.copy.setOnClickListener {
-            popupWindow.dismiss()
-            clipboard.text = getUrlForClipboard(
-                it.context.components.core.store,
-                customTabId
-            )
-
-            view.get()?.let {
-                FenixSnackbar.make(
-                    view = it,
-                    duration = Snackbar.LENGTH_SHORT,
-                    isDisplayedWithBrowserToolbar = true
+        if (copyVisible) {
+            binding.copy.setOnClickListener { copyView ->
+                popupWindow.dismiss()
+                clipboard.text = getUrlForClipboard(
+                    copyView.context.components.core.store,
+                    customTabId
                 )
-                    .setText(context.getString(R.string.browser_toolbar_url_copied_to_clipboard_snackbar))
-                    .show()
+
+                view.get()?.let { toolbarView ->
+                    FenixSnackbar.make(
+                        view = toolbarView,
+                        duration = Snackbar.LENGTH_SHORT,
+                        isDisplayedWithBrowserToolbar = true
+                    )
+                        .setText(context.getString(R.string.browser_toolbar_url_copied_to_clipboard_snackbar))
+                        .show()
+                }
+                context.components.analytics.metrics.track(Event.CopyUrlUsed)
             }
-            context.components.analytics.metrics.track(Event.CopyUrlUsed)
         }
 
-        binding.paste.setOnClickListener {
-            popupWindow.dismiss()
-            handlePaste(clipboard.text!!)
-        }
+        clipboardUrl?.let { url ->
+            binding.paste.setOnClickListener {
+                popupWindow.dismiss()
+                handlePaste(url)
+            }
 
-        binding.pasteAndGo.setOnClickListener {
-            popupWindow.dismiss()
-            handlePasteAndGo(clipboard.text!!)
+            binding.pasteAndGo.setOnClickListener {
+                popupWindow.dismiss()
+                handlePasteAndGo(url)
+            }
         }
 
         view.get()?.let {
@@ -110,4 +116,12 @@ object ToolbarPopupWindow {
             selectedTab?.readerState?.activeUrl ?: selectedTab?.content?.url
         }
     }
+
+    private fun ClipboardHandler.getUrl(): String? {
+        if (containsURL()) {
+            text?.let { return it }
+            Logger("ToolbarPopupWindow").error("Clipboard contains URL but unable to read text")
+        }
+        return null
+    }
 }