Closed Bug 1383081 Opened 5 years ago Closed 5 years ago
DLC: Close stream before validating checksum
55.21 KB, image/png
59 bytes, text/x-review-board-request
Bug 1261983 introduced a regression: We verify the checksum of downloaded content before the stream is closed now: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b67af192ba8b8d0abbdb0f03bde9809da626d2 This can fail and I saw this fail consistently on my phone after trying to recover from bug 1382596. This should be fixed and uplifted before pushing the new catalog into production.
The good news is that this regression only made it to Beta so far. And looking at historic data in telemetry (See attachment) it luckily didn't affect many clients. There has been a small rise in failures but it's hard to say if this is from this regression. This could heavily depend on Android version and/or device. On the Pixel with Android 8 it's definitely a persistent issue.
Comment on attachment 8889359 [details] Bug 1383081 - (DLC) DownloadAction: Flush stream before verifying checksum. https://reviewboard.mozilla.org/r/159884/#review166000 Makes sense to me. I verified `FilterOutputStream.close` (`BufferedOutputStream extends FilterOutputStream`), which used to be called before the checksum, calls flush: http://androidxref.com/7.1.1_r6/xref/libcore/ojluni/src/main/java/java/io/FilterOutputStream.java#157
Attachment #8889359 - Flags: review?(michael.l.comella) → review+
Iteration: --- → 1.26
Priority: -- → P1
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/f1ab83f75607 (DLC) DownloadAction: Flush stream before verifying checksum. r=mcomella
Comment on attachment 8889359 [details] Bug 1383081 - (DLC) DownloadAction: Flush stream before verifying checksum. Approval Request Comment [Feature/Bug causing the regression]: Regression introduced in bug 1261983 [User impact if declined]: Downloading fonts from our downloadable content server might fail. Some websites will render with the default system fonts instead. Websites might look slightly different. [Is this code covered by automated tests?]: Downloadable content has a good test coverage. [Has the fix been verified in Nightly?]: I debugged and tested this with a local build. This is very hard to verify in Nightly just by using the app. However we have telemetry for downloadable content and should see a drop in checksum errors. [Needs manual test from QE? If yes, steps to reproduce]: - [List of other uplifts needed for the feature/fix]: - [Is the change risky?]: Very low risk. This patch is making sure that we completely write a file to disk before verifying the checksum. [Why is the change risky/not risky?]: Very small change. No change of the code flow itself. [String changes made/needed]: -
Attachment #8889359 - Flags: approval-mozilla-beta?
Comment on attachment 8889359 [details] Bug 1383081 - (DLC) DownloadAction: Flush stream before verifying checksum. fennec regression fix for beta55 should be in 55.0b14 and 55.0rc next week
Attachment #8889359 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.