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
upstream-sync
Grigory Kruglov 2 years ago committed by mergify[bot]
parent c9a47d08a8
commit 5daa06f7dc

@ -22,6 +22,7 @@ import org.mozilla.fenix.components.metrics.Event
import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.components
import java.lang.ref.WeakReference import java.lang.ref.WeakReference
import mozilla.components.browser.state.selector.findCustomTab import mozilla.components.browser.state.selector.findCustomTab
import mozilla.components.support.base.log.logger.Logger
import org.mozilla.fenix.databinding.BrowserToolbarPopupWindowBinding import org.mozilla.fenix.databinding.BrowserToolbarPopupWindowBinding
object ToolbarPopupWindow { object ToolbarPopupWindow {
@ -34,7 +35,8 @@ object ToolbarPopupWindow {
) { ) {
val context = view.get()?.context ?: return val context = view.get()?.context ?: return
val clipboard = context.components.clipboardHandler val clipboard = context.components.clipboardHandler
if (!copyVisible && !clipboard.containsURL()) return val clipboardUrl = clipboard.getUrl()
if (!copyVisible && clipboardUrl == null) return
val isCustomTabSession = customTabId != null val isCustomTabSession = customTabId != null
@ -54,37 +56,41 @@ object ToolbarPopupWindow {
binding.copy.isVisible = copyVisible binding.copy.isVisible = copyVisible
binding.paste.isVisible = clipboard.containsURL() && !isCustomTabSession val showPaste = clipboardUrl != null && !isCustomTabSession
binding.pasteAndGo.isVisible = binding.paste.isVisible = showPaste
clipboard.containsURL() && !isCustomTabSession binding.pasteAndGo.isVisible = showPaste
binding.copy.setOnClickListener { if (copyVisible) {
popupWindow.dismiss() binding.copy.setOnClickListener { copyView ->
clipboard.text = getUrlForClipboard( popupWindow.dismiss()
it.context.components.core.store, clipboard.text = getUrlForClipboard(
customTabId copyView.context.components.core.store,
) customTabId
view.get()?.let {
FenixSnackbar.make(
view = it,
duration = Snackbar.LENGTH_SHORT,
isDisplayedWithBrowserToolbar = true
) )
.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 { clipboardUrl?.let { url ->
popupWindow.dismiss() binding.paste.setOnClickListener {
handlePaste(clipboard.text!!) popupWindow.dismiss()
} handlePaste(url)
}
binding.pasteAndGo.setOnClickListener { binding.pasteAndGo.setOnClickListener {
popupWindow.dismiss() popupWindow.dismiss()
handlePasteAndGo(clipboard.text!!) handlePasteAndGo(url)
}
} }
view.get()?.let { view.get()?.let {
@ -110,4 +116,12 @@ object ToolbarPopupWindow {
selectedTab?.readerState?.activeUrl ?: selectedTab?.content?.url 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
}
} }

Loading…
Cancel
Save