1594905 - GeckoInputStream read() method not throwing IOException

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Chris Peterson [:cpeterson]]

This bug was originally reported in A-C's GitHub issue tracker:
https://github.com/mozilla-mobile/android-components/issues/4999

And was found while investigating a Fenix download issue:
https://github.com/mozilla-mobile/fenix/issues/6434

The A-C reporter says:

I narrowed the problem down to the copyInChunks() method from AbstractFetchDownloadService. Here, I tested with Fenix, Reference Browser and samples-browser from a-c. From my tests I observed that samples-browser, which uses the okio source as an inputStream, returns the appropriate IOException when WiFi connection is interrupted mid-download, while Fenix and Reference Browser, which both use the GeckoInputStream instead, return -1 (which is considered as EOF, so a success response).

As per java documentation, an input stream implementation should throw an IOException "if the first byte cannot be read for any reason other than the end of the file", so this behaviour should be fixed in GeckoInputStream.

Comment 1

[Chris Peterson [:cpeterson]]

Sawyer says fixing this reproducible crash (related to bug 1594734) is a high priority, so I'm tentatively assigning this bug to GV's November sprint.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

EOF does not mean success, it just means there's no more data. For an HTTP[S] response, you have to do more work to determine if the download is complete by checking against the Content-length header or sometimes verifying checksums. You have to do that work whether GeckoInputStream throws IOException or not.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

I've reconsidered after talking it over with Sawyer. There is some value in doing what we can here, and plenty of evidence in Gecko that using the OnStopRequest() status is helpful.

Comment 4

[Sawyer Blatz [:sblatz]]

Let's make sure this fix gets into GeckoBeta so we can ship it in our next Fenix release (don't want to wait for 72).

Comment 5

[Sawyer Blatz [:sblatz]]

Snorp, are you able to help me understand what this OnStopRequest() would do? Would this be a callback on the session? Would it only be called in failure cases?

I'm worried this solution would not help us inside of AbstractFetchDownloadService (https://github.com/mozilla-mobile/android-components/blob/master/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt#L291-L304)

Is it possible for you to throw an IOException similar to okio? This is what I would expect from a file stream that suddenly closes.

Comment 6

[Chris Peterson [:cpeterson]]

James says he's working on this in November, but Fenix is not blocked by this bug.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1594905 - Propagate Gecko channel errors to GeckoInputStream

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4258604bf44
Propagate Gecko channel errors to GeckoInputStream r=geckoview-reviewers,esawin

Comment 9

[Andrei Ciure[:aciure]]

Backed out changeset c4258604bf44 (bug 1594905) for causing linting failure in GeckoWebExecutor.java

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=278463610&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=c4258604bf44144e9d37a6856bba75e4970cbe01

backout: https://hg.mozilla.org/integration/autoland/rev/122c24709eacd47d926ab83919fa56576f60d6ca

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69e4788ac659
Propagate Gecko channel errors to GeckoInputStream r=geckoview-reviewers,esawin

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/69e4788ac659

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Whoops, need to clear the NI here -- I would expect us to throw in similar situations to okio now.