Closed Bug 1594905 Opened Last month Closed 10 days ago

GeckoInputStream read() method not throwing IOException

Categories

(GeckoView :: General, defect, P1)

All
Android
defect

Tracking

(firefox71 affected, firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox71 --- affected
firefox72 --- fixed

People

(Reporter: cpeterson, Assigned: snorp, NeedInfo)

References

Details

(Whiteboard: [geckoview:m1911])

Attachments

(1 file)

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.

Blocks: 1594734

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.

Priority: -- → P1
Whiteboard: [geckoview:m1911]

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.

Status: NEW → RESOLVED
Closed: 29 days ago
Resolution: --- → WONTFIX

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.

Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

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).

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.

Flags: needinfo?(snorp)

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

Assignee: nobody → snorp
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4258604bf44
Propagate Gecko channel errors to GeckoInputStream r=geckoview-reviewers,esawin
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69e4788ac659
Propagate Gecko channel errors to GeckoInputStream r=geckoview-reviewers,esawin
Status: REOPENED → RESOLVED
Closed: 29 days ago10 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.