Bug 194239 – Take additional process assertion while downloading

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Take additional process assertion while downloading

<rdar://problem/47741356>

[Brady Eidson]

Created attachment 361103 [details]
Patch

[Chris Dumez]

Comment on attachment 361103 [details]
Patch

r=me

[Brady Eidson]

Created attachment 361105 [details]
Patch

[Brady Eidson]

Created attachment 361108 [details]
Patch

[Brady Eidson]

Created attachment 361113 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361113 [details]
Patch

Clearing flags on attachment: 361113

Committed r240954: <https://trac.webkit.org/changeset/240954>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Quesada]

Don't we also need to take the assertion in DownloadManager::resumeDownload()?

[Chris Dumez]

(In reply to David Quesada from comment #8)
> Don't we also need to take the assertion in
> DownloadManager::resumeDownload()?

Well, if you want to resume downloads, I guess that makes sense :P

[Brady Eidson]

Yup, I could've sworn I searched for all the m_downloads.add calls but I clearly missed that one.

I'm going to refactor that map to make it impossible to miss in the future.

[Brady Eidson]

Following up in https://bugs.webkit.org/show_bug.cgi?id=194294

[Geoffrey Garen]

Comment on attachment 361113 [details]
Patch

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

> Source/WebKit/NetworkProcess/Downloads/DownloadManager.cpp:79
> +    if (m_downloads.size() == 1) {
> +        ASSERT(!m_downloadAssertion);
> +        m_downloadAssertion = std::make_unique<ProcessAssertion>(getpid(), "WebKit downloads"_s, AssertionState::Download);
> +    }

I think this would be clearer and more robust if you just tested !m_downloadAssertion.

It's true that m_downloads.size() should be 1 if and only if m_downloadAssertion is null. But there's no need to assert that relationship or depend on it.

> Source/WebKit/NetworkProcess/Downloads/DownloadManager.cpp:185
> +        ASSERT(m_downloadAssertion);

Here, we can ASSERT(m_downloadAssertion) regardless of whether m_downloads is empty. We believe that we should always have an assertion while downloading.