Skip to content
This repository was archived by the owner on Nov 1, 2022. It is now read-only.

Conversation

@Mugurell
Copy link
Contributor

@Mugurell Mugurell commented Mar 3, 2021

Speculative fix (cannot reproduce the issue) for crashes where based on the
stacktrace the download's mime type was empty.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #9822 (d860b7d) into master (7e3b610) will increase coverage by 0.00%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #9822     +/-   ##
===========================================
  Coverage     74.06%   74.07%             
+ Complexity     5656     5056    -600     
===========================================
  Files           772      702     -70     
  Lines         28763    25601   -3162     
  Branches       4741     4203    -538     
===========================================
- Hits          21303    18963   -2340     
+ Misses         4981     4430    -551     
+ Partials       2479     2208    -271     
Impacted Files Coverage Δ Complexity Δ
...va/mozilla/components/support/ktx/kotlin/String.kt 77.58% <0.00%> (-1.37%) 0.00 <0.00> (ø)
.../feature/downloads/AbstractFetchDownloadService.kt 75.24% <72.72%> (+1.47%) 96.00 <0.00> (ø)
...ozilla/components/feature/downloads/ext/Context.kt 55.00% <0.00%> (-45.00%) 0.00% <0.00%> (ø%)
...mponents/feature/downloads/DownloadNotification.kt 96.57% <0.00%> (-1.37%) 21.00% <0.00%> (-1.00%)
...s/concept/push/exceptions/SubscriptionException.kt
...ava/mozilla/components/concept/push/PushService.kt
...onents/lib/dataprotect/SecureAbove22Preferences.kt
...va/mozilla/components/service/sync/logins/Types.kt
...s/support/android/test/espresso/ViewInteraction.kt
...java/mozilla/components/feature/push/Connection.kt
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e3b610...d860b7d. Read the comment docs.

Comment on lines 1003 to 1006
internal fun getNotEmptyMimeType(mimeType: String?) = if (!mimeType.isNullOrEmpty()) {
mimeType
} else {
"*/*"
Copy link
Contributor

@Amejia481 Amejia481 Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could re-use getSafeContentType that has the same logic plus it tries to extract the MimeTypefrom the physical file, what do you think?

Copy link
Contributor Author

@Mugurell Mugurell Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this new safety net allows for a quick and easy way to ensure we don't get that crash anymore.
Using getSafeContentType would mean taking another stab at inferring the mime type (which I would've preferred to not do if not neded (it is a bit of code executed..) but maybe it would help reducing the number of times */* is returned).
In the current implementation getSafeContentType returns DownloadUtils.sanitizeMimeType(resultContentType) ?: "*/*" and so it still has the possibility of returning an empty String.
Instead of wrapping getSafeContentType and others in getNotEmptyMimeType we could add one last check for empty strings in that method and use that.
Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the patch to use getSafeContentType.
Would appreciate if you could comment on the approach, if it's okay.
I'll work next on updating tests and documentation.

@mergify
Copy link
Contributor

mergify bot commented Mar 4, 2021

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

@Mugurell Mugurell force-pushed the 9821CrashForMimeTypes branch 2 times, most recently from a598e73 to 022af9c Compare March 4, 2021 13:15
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, could we add tests for the new extension function.

@Mugurell Mugurell added the do not land PRs that requires coordination before landing label Mar 5, 2021
…type if otherwise empty

Speculative fix (cannot reproduce the issue) for crashes where based on the
stacktrace the download's mime type was empty.
@Mugurell Mugurell force-pushed the 9821CrashForMimeTypes branch from 022af9c to 67b4c71 Compare March 9, 2021 16:09
@Mugurell Mugurell added 🛬 needs landing (squash) PRs that are ready to land (squashed) and removed do not land PRs that requires coordination before landing labels Mar 10, 2021
@mergify mergify bot merged commit dcc78fd into mozilla-mobile:master Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🛬 needs landing (squash) PRs that are ready to land (squashed)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants