Implement DownloadMonitor to prevent long-running slow downloads from background apps

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Implement DownloadMonitor to prevent long-running slow downloads from background apps

[Alex Christensen]

Created attachment 364730 [details]
Patch

[Geoffrey Garen]

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.

[Alex Christensen]

Created attachment 364915 [details]
Patch

[Alex Christensen]

Created attachment 364933 [details]
Patch

[Alex Christensen]

Created attachment 364934 [details]
Patch

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 364957 [details]
Patch

[Alex Christensen]

Created attachment 365025 [details]
Patch

[Geoffrey Garen]

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.

[Alex Christensen]

Created attachment 365070 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365070 [details]
Patch

Clearing flags on attachment: 365070

Committed r243110: <https://trac.webkit.org/changeset/243110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48997000>

[Alex Christensen]

http://trac.webkit.org/r243131

[Alex Christensen]

rdar://problem/45777358