DLC: Close stream before validating checksum

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

unspecified
Firefox 56
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [MobileAS])

Attachments

(2 attachments)

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
Whiteboard: [MobileAS]
Pushed by s.kaspari@gmail.com:
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?
https://hg.mozilla.org/mozilla-central/rev/f1ab83f75607
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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+
You need to log in before you can comment on or make changes to this bug.