Bug 163006 – Network Session: PendingDownload is always nullptr in DownloadManager::dataTaskBecameDownloadTask

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

[Carlos Garcia Campos]

I'm not sure when dataTaskBecameDownloadTask() is called exactly in Mac, but it seems to me that PendingDownload is always nullptr at that point. Here are the steps for downloads started with startDownload and convertTaskToDownload:

1- DownloadManager::startDownload()
  m_pendingDownloads.add(downloadID, PendingDownload())

2- PendingDownload()
  m_networkLoad(NetworkLoad())
  set pending download ID, pending download and suggested filename on new NetworkLoad

3- NetworkLoad::resume()
  Starts the load normally until receiving the response

4- NetworkLoad::didReceiveResponse()
  The task has pending download ID

5- NetworkProcess::findPendingDownloadLocation(task, completionHandler)
  Notifies the download manager that is waiting for the location

6- DownloadManager::willDecidePendingDownloadDestination()
  m_pendingDownloads.take(downloadID)
  m_downloadsWaitingForDestination.set(<task, completionHandler>)
  The pending download is now deleted and its NetworkLoad too.
  Once it has the loation it either cancels the download or continues

7- DownloadManager::continueDecidePendingDownloadDestination
  <task, completionHandler> = m_downloadsWaitingForDestination.take(downloadID)
  completionHandler(PolicyDownload)
  m_downloadsAfterDestinationDecided.set(downloadID, task)

8- DownloadManager::dataTaskBecameDownloadTask(downloadID, download)
  The completionHandler() ended up converting the task to a download, so at some point
  dataTaskBecameDownloadTask is called right after creating the download.
  // This is needed for downloads started with startDownload, otherwise it will return nullptr.
  m_pendingDownloads.take(downloadID)
  // This is needed for downloads started with convertTaskToDownload, otherwise it will return nullptr.
  m_downloadsAfterDestinationDecided.take(downloadID)

At this point m_pendingDownloads is supposed to contain the download, because it was started by DownloadManager::startDownload(), but the download was removed from m_pendingDownloads at step 6.
If the completionHandler calls dataTaskBecameDownloadTask() then m_downloadsAfterDestinationDecided doesn't contain the download yet either, so both are indeed nullptr. If it's supposed to happen asynchronously then we indeed have a task even when this was not started by convertTaskToDownload.

1- NetworkLoad()::resume()
  Starts the load normally until policy checker converts the load into a download

2- NetworkLoad::convertTaskToDownload(downloadID, request, response)
  Sets pending download ID to the the task

3- NetworkProcess::findPendingDownloadLocation(task, completionHandler)
  Notifies the download manager that is waiting for the location

The next steps are exactly the same, with the difference that in this case m_pendingDownloads will not contain a PendingDownload for the downloadID, but in both cases we have a NetworkDataTask.

[Carlos Garcia Campos]

Created attachment 290813 [details]
Patch

[Alex Christensen]

Comment on attachment 290813 [details]
Patch

This seems like it might be right, but it needs to be tested on Sierra.  My Sierra machine is being upgraded right now, so I'll have to look at this again in the morning.  Sorry.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 290813 [details]
> Patch
> 
> This seems like it might be right, but it needs to be tested on Sierra.  My
> Sierra machine is being upgraded right now, so I'll have to look at this
> again in the morning.  Sorry.

No problem, I'm not in a hurry. I was also thinking that maybe it makes sense to keep the pending download alive right until it's converted into a real download.

[Carlos Garcia Campos]

Created attachment 290932 [details]
Different patch

I changed the approach in the end, to make sure the pending download is alive until the didReceiveResponse completion handler is called. That way we can be sure that our NetowkrDataTask always have a valid client at that point, no matter if the download was started by startDownload or convertTasktoDownload. It allows us to report errors while creating the destination before the Download object is created.

[Carlos Garcia Campos]

Created attachment 290934 [details]
Re-submit for EWS

I think it should apply now

[WebKit Commit Bot]

Attachment 290934 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:298:  'protectNetworkDataTask' is incorrectly named. It should be named 'protector' or 'protectedNetworkDataTask'.  [readability/naming/protected] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 290936 [details]
Fix coding style

[Darin Adler]

Comment on attachment 290936 [details]
Fix coding style

Can we create a test case?

[Alex Christensen]

Comment on attachment 290936 [details]
Fix coding style

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

> Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:123
> +        ASSERT(!pendingDownload || pendingDownload.get() == networkDataTask.pendingDownload());

Debug builds using network session fail here.  Should be networkDataTask->

[Alex Christensen]

Comment on attachment 290936 [details]
Fix coding style

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

This should have no change in behavior.  There are already some download tests, and I just verified that this doesn't cause a problem on Mac.

> Source/WebKit2/ChangeLog:25
> +        return eearly if both are nullptr to avoid crashes. However, we are checking that the download is in the

early

[Carlos Garcia Campos]

(In reply to comment #9)
> Comment on attachment 290936 [details]
> Fix coding style
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290936&action=review
> 
> > Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp:123
> > +        ASSERT(!pendingDownload || pendingDownload.get() == networkDataTask.pendingDownload());
> 
> Debug builds using network session fail here.  Should be networkDataTask->

Indeed!

[Carlos Garcia Campos]

Committed r206987: <http://trac.webkit.org/changeset/206987>