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

Closes #5284: Adds progress bar to download notification #5285

Merged
merged 1 commit into from
Dec 27, 2019

Conversation

sblatz
Copy link
Contributor

@sblatz sblatz commented Dec 10, 2019


DEMO:
progress bar 2019-12-19 14_20_54

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.

@sblatz sblatz requested a review from pocmo December 10, 2019 22:08
@sblatz sblatz force-pushed the progress-bar-notif branch from 2071ba5 to f5782c5 Compare December 10, 2019 22:10
@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #5285 into master will decrease coverage by 0.34%.
The diff coverage is 42.22%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5285      +/-   ##
============================================
- Coverage      80.3%   79.95%   -0.35%     
+ Complexity     4219     4205      -14     
============================================
  Files           545      543       -2     
  Lines         19174    19193      +19     
  Branches       2772     2780       +8     
============================================
- Hits          15397    15346      -51     
- Misses         2618     2689      +71     
+ Partials       1159     1158       -1
Impacted Files Coverage Δ Complexity Δ
...mponents/feature/downloads/DownloadNotification.kt 0% <0%> (-70.33%) 0 <0> (-9)
.../feature/downloads/AbstractFetchDownloadService.kt 44.73% <48.71%> (-2.46%) 22 <11> (ø)
...onents/support/sync/telemetry/BaseGleanSyncPing.kt 100% <0%> (ø) 11% <0%> (ø) ⬇️
...mozilla/components/support/locale/LocaleManager.kt
...va/mozilla/components/support/locale/Extensions.kt
...nts/support/locale/LocaleAwareAppCompatActivity.kt
...omponents/support/locale/LocaleAwareApplication.kt
...ponents/service/location/MozillaLocationService.kt 98.03% <0%> (ø) 3% <0%> (?)
...ocation/search/RegionSearchLocalizationProvider.kt 85.71% <0%> (ø) 3% <0%> (?)
... and 2 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 d10c360...fb3847f. Read the comment docs.

@sblatz sblatz force-pushed the progress-bar-notif branch 2 times, most recently from ed159d7 to 25aad45 Compare December 11, 2019 00:55
@sblatz sblatz changed the title Closes #5284: Adds progress bar to download notification Closes #5284: Adds progress bar to download notification RUN AGAIN Dec 11, 2019
@sblatz sblatz changed the title Closes #5284: Adds progress bar to download notification RUN AGAIN Closes #5284: Adds progress bar to download notification Dec 11, 2019
Copy link
Contributor

@pocmo pocmo left a comment

Choose a reason for hiding this comment

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

Oh, thank god. I recently downloaded some large files with Fenix and was completely clueless about what's going on without the progress. :)

@pocmo pocmo added the 🕵️‍♀️ needs review PRs that need to be reviewed label Dec 11, 2019
}
}

notificationTimer.scheduleAtFixedRate(timerTask, 0, PROGRESS_UPDATE_INTERVAL)
Copy link
Contributor

@pocmo pocmo Dec 11, 2019

Choose a reason for hiding this comment

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

I am a bit confused about the lifetime of the tasks. We schedule one task for every download here. But in startDownoadJob() we cancel all tasks on the timer. So I am wondering:

  • Doesn't the cancel() call in startDownoadJob() cause all other notifications to get stale except the new one?
  • We never cancel a specific task after the download is done right? Only if the service stops and the Timer object gets garbage collected then they get away (but the docs are not very specific on when that happens).

I wonder if we could use a single Kotlin Coroutine to update all notifications, something like:

val notificationUpdateScope = MainScope() // Or a different scope if that the notification update does not need to happen on the main thread.

override fun onCreate() {
  // [..]
  notificationUpdateScope.launch {
     delay(...)
     updateProgress()
  }
}

override fun onDestroy() {
   // [..]
   notificationUpdateScope.cancel()
}

fun updateProgress() {
   // Update progress of all notifications.
}

Does this make sense? I think with a single coroutine it is much easier to understand its lifetime and avoiding multiple zombie threads being left behind. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All great points. I have updated this PR with the ideas you suggested.

@sblatz sblatz force-pushed the progress-bar-notif branch 2 times, most recently from 53babdd to 75ae0e4 Compare December 11, 2019 22:38
if (download.status != DownloadJobStatus.ACTIVE) { continue }
// We must be synchronized here to avoid the status getting set to PAUSED on this line
// and then overwriting that with an ongoing notification anyway
displayOngoingDownloadNotification(download.state, download.currentBytesCopied)
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'm having trouble with this race condition. Do you have any suggestions about how to solve it? I thought putting a synchronized on this and all other state setting would be sufficient but it doesn't seem so 😕

@pocmo

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, this has fallen through the cracks. I can try to help with that this week. What is happening in that race condition? Notifications do not update or with wrong state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What’s happening is a user presses PAUSE, the notification then gets a timed update to display the current progress (ACTIVE state), so the “pause” action gets eaten but the download is still paused and stuck in a state where the user sees it as “frozen”

@sblatz sblatz force-pushed the progress-bar-notif branch from 75ae0e4 to 3ef13f5 Compare December 11, 2019 22:40
@sblatz sblatz force-pushed the progress-bar-notif branch 2 times, most recently from 53263d4 to 15c7a71 Compare December 19, 2019 18:02
@sblatz sblatz closed this Dec 19, 2019
@sblatz sblatz force-pushed the progress-bar-notif branch from 15c7a71 to e39b6af Compare December 19, 2019 18:03
@sblatz sblatz reopened this Dec 19, 2019
@sblatz sblatz force-pushed the progress-bar-notif branch 4 times, most recently from 2c7384c to 4b5f9fb Compare December 19, 2019 19:00
*/
private fun sendDownloadCompleteBroadcast(downloadID: Long, status: DownloadJobStatus) {
private fun sendDownloadNotInProgress(downloadID: Long, status: DownloadJobStatus) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps sendDownloadFinished is better? I know this name is ugly but I want to be clear about what's going on here.

currentDownloadJobState.status = DownloadJobStatus.PAUSED
synchronized(context) {
currentDownloadJobState.status = DownloadJobStatus.PAUSED
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we synchronize you can remove the @Volatile from DownloadJobState. Instead you could try adding @GuardedBy("context") to the status property to denote that this needs to be sychronized on context when accessed/modified. That itself doesn't do anything but hopefully the IDE (or lint) will show a warning if you don't do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooo didn't know about that keyword. Thanks!

@sblatz sblatz force-pushed the progress-bar-notif branch from 4d01145 to 3988196 Compare December 19, 2019 19:17
Copy link
Contributor

@pocmo pocmo left a comment

Choose a reason for hiding this comment

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

Great! 👍

bors r+

@sblatz sblatz force-pushed the progress-bar-notif branch from 254c1d7 to 2f9515a Compare December 19, 2019 19:55
@sblatz sblatz changed the title Closes #5284: Adds progress bar to download notification Closes #5284: Adds progress bar to download notification RE RUN Dec 26, 2019
@sblatz sblatz changed the title Closes #5284: Adds progress bar to download notification RE RUN Closes #5284: Adds progress bar to download notification Dec 26, 2019
@sblatz
Copy link
Contributor Author

sblatz commented Dec 26, 2019

bors retry

bors bot pushed a commit that referenced this pull request Dec 26, 2019
5285: Closes #5284: Adds progress bar to download notification r=pocmo a=sblatz



Co-authored-by: Sawyer Blatz <[email protected]>
@sblatz
Copy link
Contributor Author

sblatz commented Dec 26, 2019

bors r-

@bors
Copy link

bors bot commented Dec 26, 2019

Canceled

@sblatz
Copy link
Contributor Author

sblatz commented Dec 26, 2019

bors r=@pocmo

bors bot pushed a commit that referenced this pull request Dec 26, 2019
5285: Closes #5284: Adds progress bar to download notification r=pocmo a=sblatz



Co-authored-by: Sawyer Blatz <[email protected]>
@bors
Copy link

bors bot commented Dec 26, 2019

Timed out

@sblatz
Copy link
Contributor Author

sblatz commented Dec 26, 2019

bors retry

bors bot pushed a commit that referenced this pull request Dec 26, 2019
5285: Closes #5284: Adds progress bar to download notification r=pocmo a=sblatz



Co-authored-by: Sawyer Blatz <[email protected]>
@sblatz
Copy link
Contributor Author

sblatz commented Dec 26, 2019

I'm really at a loss here for what the out of memory could be caused by. It looks like it fails on every run of when the cancel button is clicked onCancelDownload must be called but only on CI. The whole build passes for me locally (and also passed on Github...?). When I debug sample browser I don't see a spike in memory consumption when following the steps of that test :\

Any ideas @pocmo / @jonalmeida

@bors
Copy link

bors bot commented Dec 26, 2019

Timed out

Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

I left some of my thought processes down below.


We can start by looking at the trace from the OOM in the CI logs:

Taskcluster CI logs.
TEST: when the cancel button is clicked onCancelDownload must be called
  [Robolectric] mozilla.components.feature.downloads.SimpleDownloadDialogFragmentTest.when the cancel button is clicked onCancelDownload must be called: sdk=28; resources=binary
  I/MonitoringInstr: Instrumentation started!
  I/MonitoringInstr: Setting context classloader to 'org.robolectric.internal.bytecode.SandboxClassLoader@1005e1f9', Original: 'org.robolectric.internal.bytecode.SandboxClassLoader@1005e1f9'
  Exception in thread "DefaultDispatcher-worker-2 @coroutine#69" java.lang.OutOfMemoryError: GC overhead limit exceeded
  at net.bytebuddy.description.type.TypeDescription$ForLoadedType.of(TypeDescription.java:8235)
  at net.bytebuddy.description.type.TypeDescription$Generic$LazyProjection$ForLoadedReturnType.asErasure(TypeDescription.java:6519)
  at net.bytebuddy.description.method.MethodDescription$AbstractBase.asTypeToken(MethodDescription.java:837)
  at net.bytebuddy.dynamic.scaffold.MethodGraph$Compiler$Default$Key$Harmonized.extend(MethodGraph.java:973)
  at net.bytebuddy.dynamic.scaffold.MethodGraph$Compiler$Default$Key$Store$Entry$Resolved.extendBy(MethodGraph.java:1408)
  at net.bytebuddy.dynamic.scaffold.MethodGraph$Compiler$Default$Key$Store.registerTopLevel(MethodGraph.java:1110)
  at net.bytebuddy.dynamic.scaffold.MethodGraph$Compiler$Default.doAnalyze(MethodGraph.java:634)
  at net.bytebuddy.dynamic.scaffold.MethodGraph$Compiler$Default.compile(MethodGraph.java:567)
  at net.bytebuddy.dynamic.scaffold.MethodGraph$Compiler$AbstractBase.compile(MethodGraph.java:465)
  at org.mockito.internal.creation.bytebuddy.MockMethodAdvice.isOverridden(MockMethodAdvice.java:134)
  at java.io.InputStream.read(InputStream.java:101)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.copyInChunks(AbstractFetchDownloadService.kt:357)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.access$copyInChunks(AbstractFetchDownloadService.kt:68)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$performDownload$1$1.invoke(AbstractFetchDownloadService.kt:332)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$performDownload$1$1.invoke(AbstractFetchDownloadService.kt:68)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.useFileStreamLegacy(AbstractFetchDownloadService.kt:449)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.useFileStream$feature_downloads_debug(AbstractFetchDownloadService.kt:400)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$performDownload$1.invoke(AbstractFetchDownloadService.kt:331)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$performDownload$1.invoke(AbstractFetchDownloadService.kt:68)
  at mozilla.components.concept.fetch.Response$Body.useStream(Response.kt:79)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.performDownload$feature_downloads_debug(AbstractFetchDownloadService.kt:327)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.startDownloadJob$feature_downloads_debug(AbstractFetchDownloadService.kt:282)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$broadcastReceiver$2$1$onReceive$3.invokeSuspend(AbstractFetchDownloadService.kt:123)
  at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
  at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
  at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:561)
  at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:727)
  at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:667)
  at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:655)
  Exception in thread "DefaultDispatcher-worker-3 @coroutine#81" java.lang.OutOfMemoryError: GC overhead limit exceeded
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.copyInChunks(AbstractFetchDownloadService.kt:356)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.access$copyInChunks(AbstractFetchDownloadService.kt:68)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$performDownload$1$1.invoke(AbstractFetchDownloadService.kt:332)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$performDownload$1$1.invoke(AbstractFetchDownloadService.kt:68)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.useFileStreamLegacy(AbstractFetchDownloadService.kt:449)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.useFileStream$feature_downloads_debug(AbstractFetchDownloadService.kt:400)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$performDownload$1.invoke(AbstractFetchDownloadService.kt:331)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$performDownload$1.invoke(AbstractFetchDownloadService.kt:68)
  at mozilla.components.concept.fetch.Response$Body.useStream(Response.kt:79)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.performDownload$feature_downloads_debug(AbstractFetchDownloadService.kt:327)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService.startDownloadJob$feature_downloads_debug(AbstractFetchDownloadService.kt:282)
  at mozilla.components.feature.downloads.AbstractFetchDownloadService$broadcastReceiver$2$1$onReceive$7.invokeSuspend(AbstractFetchDownloadService.kt:156)
  at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
  at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
  at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:561)

It's a bit long, but if you follow along the call stack for each coroutine, we can see two exceptions taking place:

Exception in thread "DefaultDispatcher-worker-2 @coroutine#69" java.lang.OutOfMemoryError: GC overhead limit exceeded
at net.bytebuddy.description.type.TypeDescription$ForLoadedType.of(TypeDescription.java:8235)

and

Exception in thread "DefaultDispatcher-worker-3 @coroutine#81" java.lang.OutOfMemoryError: GC overhead limit exceeded
at mozilla.components.feature.downloads.AbstractFetchDownloadService.copyInChunks(AbstractFetchDownloadService.kt:356)

The latter one is our code, so I'm willing to bet it's something in our code vs the net.bytebuddy that's causing the crash.


If we take a look at that line in AbstractFetchDownloadService.copyInChunks(AbstractFetchDownloadService.kt:356):

while (downloadJobState.status == DownloadJobStatus.ACTIVE) {
    val data = ByteArray(CHUNK_SIZE) // <- this is the line in question.
    val bytesRead = inStream.read(data)

    if (bytesRead == -1) { break }

    downloadJobState.currentBytesCopied += bytesRead

    outStream.write(data, 0, bytesRead)
}

So we're allocating a 4Kb ByteArray, which should explain the memory allocations leading to the OOM, however, what's more interesting to me is that it's in a while block that loops until the status changes out of DownloadJobStatus.ACTIVE.

Looking at the test, we're trying to cancel a download but I don't see one being started before.

So without much knowledge of how the download feature works, my guess is either:

  1. The cancel button doesn't work in cancelling the download service (and we need to write better tests to cover this situation or decouple the service from the fragment, so avoid this from happening again).
  2. There is a download service attached to the test that was in an active state and the test doesn't cover that (which might be related to this change that you removed 251b7e..b16810)

The whole build passes for me locally

CI machines generally have less memory allocated to the VM so they fail faster. The same OOM error would probably also take place if you reduced the memory allocated for the VM on your local machine.


See my other comment as well that seems to register every download an a resumed one, I'm not sure what the implications for that are, but it doesn't sound like the right state to have.

ongoingDownloadNotification
)
}

@Suppress("ComplexCondition")
internal fun performDownload(download: DownloadState) {
val isResumingDownload = downloadJobs[download.id]?.currentBytesCopied ?: 0L > 0L
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed this while reading the code. Wouldn't 0L > 0L always be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0L > 0L would always be false, right?

The logic is: isResumingDownload is TRUE if the bytes copied is more than 0. if bytesCopied is null, we assume it's 0, and thus this would evaluate to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why evaluate instead of simplifying this to be false?

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'm confused. I want to compare currentBytesCopied to 0L so I can't just do

downloadJobs[download.id]?.currentBytesCopied ?: false > 0L

Is there some syntax I'm missing here? 😅

Copy link
Contributor

@jonalmeida jonalmeida Dec 27, 2019

Choose a reason for hiding this comment

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

Try this so avoid confusion:

(downloadJobs[download.id]?.currentBytesCopied ?: 0L) > 0L

Otherwise, it looks like:

downloadJobs[download.id]?.currentBytesCopied ?: (0L > 0L)

EDIT

Or better:

val currentBytes = downloadJobs[download.id]?.currentBytesCopied ?: 0L
val isResumingDownload = currentBytes > 0L

@sblatz sblatz force-pushed the progress-bar-notif branch 5 times, most recently from bc4143f to f8c378b Compare December 27, 2019 21:43
@sblatz
Copy link
Contributor Author

sblatz commented Dec 27, 2019

bors retry

bors bot pushed a commit that referenced this pull request Dec 27, 2019
5285: Closes #5284: Adds progress bar to download notification r=pocmo a=sblatz



Co-authored-by: Sawyer Blatz <[email protected]>
@sblatz sblatz force-pushed the progress-bar-notif branch from f8c378b to fb3847f Compare December 27, 2019 22:17
@bors
Copy link

bors bot commented Dec 27, 2019

Canceled

@sblatz
Copy link
Contributor Author

sblatz commented Dec 27, 2019

bors r=@pocmo

bors bot pushed a commit that referenced this pull request Dec 27, 2019
5285: Closes #5284: Adds progress bar to download notification r=pocmo a=sblatz



Co-authored-by: Sawyer Blatz <[email protected]>
@bors
Copy link

bors bot commented Dec 27, 2019

Build succeeded

  • complete-push

@bors bors bot merged commit fb3847f into mozilla-mobile:master Dec 27, 2019
@sblatz
Copy link
Contributor Author

sblatz commented Dec 27, 2019

TAKE THAT BORS.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants