Bug 195785 - Implement DownloadMonitor to prevent long-running slow downloads from background apps
Summary: Implement DownloadMonitor to prevent long-running slow downloads from backgro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-14 17:34 PDT by Alex Christensen
Modified: 2019-03-20 12:33 PDT (History)
9 users (show)

See Also:


Attachments
Patch (20.38 KB, patch)
2019-03-14 17:36 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (76.86 KB, patch)
2019-03-15 21:55 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (78.91 KB, patch)
2019-03-16 09:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (81.70 KB, patch)
2019-03-16 09:58 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (82.50 KB, patch)
2019-03-16 20:36 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (83.68 KB, patch)
2019-03-18 10:25 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (83.74 KB, patch)
2019-03-18 14:59 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-03-14 17:34:16 PDT
Implement DownloadMonitor to prevent long-running slow downloads from background apps
Comment 1 Alex Christensen 2019-03-14 17:36:05 PDT
Created attachment 364730 [details]
Patch
Comment 2 Geoffrey Garen 2019-03-14 20:06:43 PDT
Comment on attachment 364730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364730&action=review

Please fix EWS build failures.

> Source/WebKit/NetworkProcess/Downloads/Download.h:103
> +    friend class DownloadMonitor;
>      bool isAlwaysOnLoggingAllowed() const;

Can we just make this interface public? Seems less bad than making all our data members public to DownloadMonitor.

> Source/WebKit/NetworkProcess/Downloads/DownloadMonitor.cpp:86
> +    while (m_timestamps.size() > maxTimestampsSize)
> +        m_timestamps.removeFirst();

Can we do this before the append, to avoid out-of-line storage?

Does this need to be a loop, or can we just remove one when we're at capacity?

> Source/WebKit/NetworkProcess/Downloads/DownloadMonitor.cpp:97
> +void DownloadMonitor::applicationNoLongerInForeground()

Let's call this applicationEnteredBackground.

> Source/WebKit/NetworkProcess/Downloads/DownloadMonitor.cpp:116
> +        RELEASE_LOG_IF_ALLOWED("timerFired: Download reached threshold to not be terminated (id = %" PRIu64 ")", m_download.downloadID().downloadID());

So, if you survive past 60 minutes, you survive forever? Is that how other downloads behave?

> Source/WebKit/NetworkProcess/Downloads/DownloadMonitor.h:53
> +    static constexpr size_t maxTimestampsSize = 10;

Let's call this timestampCapacity.
Comment 3 Alex Christensen 2019-03-15 21:55:50 PDT
Created attachment 364915 [details]
Patch
Comment 4 Alex Christensen 2019-03-16 09:40:24 PDT
Created attachment 364933 [details]
Patch
Comment 5 Alex Christensen 2019-03-16 09:58:19 PDT
Created attachment 364934 [details]
Patch
Comment 6 Build Bot 2019-03-16 09:59:42 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 7 Alex Christensen 2019-03-16 20:36:07 PDT
Created attachment 364957 [details]
Patch
Comment 8 Alex Christensen 2019-03-18 10:25:49 PDT
Created attachment 365025 [details]
Patch
Comment 9 Geoffrey Garen 2019-03-18 14:04:45 PDT
Comment on attachment 365025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365025&action=review

r=me

> Source/WebKit/NetworkProcess/Downloads/DownloadManager.cpp:208
> +void DownloadManager::applicationDidEnterBackground()
> +{
> +    for (auto& download : m_downloads.values())
> +        download->applicationEnteredBackground();
> +}
> +
> +void DownloadManager::applicationWillEnterForeground()
> +{
> +    for (auto& download : m_downloads.values())
> +        download->applicationEnteredForeground();
> +}

Nit: You can resolve the small differences in these names.

applicationEnteredForeground => applicationWillEnterForeground
applicationEnteredBackground => applicationDidEnterBackground

> Source/WebKit/UIProcess/API/Cocoa/_WKProcessPoolConfiguration.h:72
> +@property (nonatomic) NSUInteger downloadMonitorSpeedMultiplier WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Maybe put "ForDebugging" on the ends of this name and its related names, to clarify the purpose.
Comment 10 Alex Christensen 2019-03-18 14:59:56 PDT
Created attachment 365070 [details]
Patch
Comment 11 WebKit Commit Bot 2019-03-18 15:39:01 PDT
Comment on attachment 365070 [details]
Patch

Clearing flags on attachment: 365070

Committed r243110: <https://trac.webkit.org/changeset/243110>
Comment 12 WebKit Commit Bot 2019-03-18 15:39:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-03-18 15:40:34 PDT
<rdar://problem/48997000>
Comment 14 Alex Christensen 2019-03-18 22:47:21 PDT
http://trac.webkit.org/r243131
Comment 15 Alex Christensen 2019-03-20 12:33:35 PDT
rdar://problem/45777358