Skip to content

Commit

Permalink
Issue mozilla-mobile#7888: Remove workaround for URI not supported ex…
Browse files Browse the repository at this point in the history
…ception
  • Loading branch information
csadilek committed Feb 2, 2021
1 parent 37d1148 commit 6ee8de5
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 19 deletions.
Expand Up @@ -17,8 +17,8 @@ import mozilla.components.concept.fetch.Response
import mozilla.components.concept.fetch.isSuccess
import mozilla.components.support.base.log.logger.Logger
import mozilla.components.support.ktx.android.net.isHttpOrHttps
import mozilla.components.support.ktx.kotlin.sanitizeURL
import java.io.IOException
import java.lang.IllegalArgumentException
import java.util.concurrent.TimeUnit

private const val CONNECT_TIMEOUT = 2L // Seconds
Expand All @@ -42,7 +42,7 @@ class HttpIconLoader(
// recently: https://github.com/mozilla-mobile/android-components/issues/2591

val downloadRequest = Request(
url = resource.url,
url = resource.url.sanitizeURL(),
method = Request.Method.GET,
cookiePolicy = if (request.isPrivate) {
Request.CookiePolicy.OMIT
Expand All @@ -65,20 +65,11 @@ class HttpIconLoader(
} catch (e: IOException) {
logger.debug("IOException while trying to download icon resource", e)
IconLoader.Result.NoResult
} catch (e: IllegalArgumentException) {
// Despite checking that the icon URL scheme is http or https we're
// still seeing GeckoView rejecting requests due to unsupported URI
// schemes. We believe this could be caused by GeckoView converting
// the String toLowerCase, but can't find a reproducible case.
// Instead of crashing let's log the details so we can get to bottom
// of it once we can reproduce.
logger.debug("Invalid request to fetch icon: $downloadRequest", e)
IconLoader.Result.NoResult
}
}

private fun shouldDownload(resource: IconRequest.Resource): Boolean {
return resource.url.toUri().isHttpOrHttps && !failureCache.hasFailedRecently(resource.url)
return resource.url.sanitizeURL().toUri().isHttpOrHttps && !failureCache.hasFailedRecently(resource.url)
}
}

Expand Down
Expand Up @@ -31,7 +31,6 @@ import org.mockito.Mockito.verify
import org.mockito.Mockito.verifyNoMoreInteractions
import java.io.InputStream
import java.io.IOException
import java.lang.IllegalArgumentException

@RunWith(AndroidJUnit4::class)
class HttpIconLoaderTest {
Expand Down Expand Up @@ -227,17 +226,29 @@ class HttpIconLoaderTest {
}

@Test
fun `Loader will return NoResult for IllegalArgumentExceptions`() {
fun `Loader will sanitize URL`() {
val client: Client = mock()

val loader = HttpIconLoader(client)
doThrow(IllegalArgumentException("test")).`when`(client).fetch(any())
doReturn(
Response(
url = "https://www.example.org",
headers = MutableHeaders(),
status = 404,
body = Response.Body.empty())
).`when`(client).fetch(any())

val resource = IconRequest.Resource(
url = "http://www.example.org",
type = IconRequest.Resource.Type.APPLE_TOUCH_ICON
loader.load(
mock(), mock(), IconRequest.Resource(
url = " \n\n https://www.example.org \n\n ",
type = IconRequest.Resource.Type.APPLE_TOUCH_ICON
)
)

assertEquals(IconLoader.Result.NoResult, loader.load(mock(), mock(), resource))
val captor = argumentCaptor<Request>()
verify(client).fetch(captor.capture())

val request = captor.value
assertEquals("https://www.example.org", request.url)
}
}

0 comments on commit 6ee8de5

Please sign in to comment.