1905875 - Android Components: HttpIconLoader partially loads Icons if content length is not specified

Status: RESOLVED FIXED

(Fenix :: General, defect)

Description

[Artem Chepurnyi]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:127.0) Gecko/20100101 Firefox/127.0

Steps to reproduce:

This bug is related to org.mozilla.components:browser-icons:127.0.2.

Extend mozilla.components.browser.icons.BrowserIcons with a custom preparer that points to icons that do not have the Content-Length header in the response. This makes mozilla.components.browser.icons.loader.HttpIconLoader#toIconLoaderResult(...) go the alternative route that sometimes loads icons only partially.

Actual results:

while (inputStream.read(buffer).also { bytesInChunk = it } != -1) {
  outStream.write(buffer, 0, bytesInChunk)
  bytesRead += bytesInChunk  
  
  if (bytesRead > MAX_DOWNLOAD_BYTES || bytesRead > memoryInfoProvider.getAvailMem()) {
    return@useStream IconLoader.Result.NoResult
  }

  if (bytesInChunk < DEFAULT_BUFFER_SIZE) {
    break
  }
}

This snippet assumes that InputStream#read(...) always reads the size of the buffer bytes, unless it's the end of a content. Unfortunately that is not correct, InputStream#read(...) can return any value in range of -1..buffer.size, where only -1 means the end of a content.

This results in an icon sometimes partially loading if the server returns less bytes then requested in a single read.

Expected results:

while (inputStream.read(buffer).also { bytesInChunk = it } != -1) {
  outStream.write(buffer, 0, bytesInChunk)
  bytesRead += bytesInChunk  
  
  if (bytesRead > MAX_DOWNLOAD_BYTES || bytesRead > memoryInfoProvider.getAvailMem()) {
    return@useStream IconLoader.Result.NoResult
  }
}

Additional condition check should be removed.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:royang, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Roger Yang [:royang]]

Hi Cathy, can you please take a look? The reporter thinks that we don't need the check:

  if (bytesInChunk < DEFAULT_BUFFER_SIZE) {
    break
  }

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Comment 4

[Cathy Lu [:calu]]

Hey Donal, we use HttpIconLoader for all sorts of images from websites.

I tried just removing the check but the tests fail, I'll try and take a look at the tests soon.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1847372

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Hi calu, just resurfacing this based on comment 4 to make sure this didn't fall off your radar.

Comment 7

[Cathy Lu [:calu]]

Attachment: Bug 1905875 - Fix partially loaded icons when content length is missing

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b741ea7b4c
Fix partially loaded icons when content length is missing r=android-reviewers,owlish

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f1b741ea7b4c

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:calu, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox132 to wontfix.

For more information, please visit BugBot documentation.