Bug 198945

Summary: Handle NSProgress calling our cancellation handler on background threads (and calling it more than once)
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, beidson, commit-queue, david_quesada
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Brady Eidson
Reported 2019-06-17 17:21:20 PDT
Handle NSProgress calling our cancellation handler on background threads (and calling it more than once)
Attachments
Patch (3.37 KB, patch)
2019-06-17 17:34 PDT, Brady Eidson 		no flags
DetailsFormatted DiffDiff
Patch (4.21 KB, patch)
2019-06-18 12:59 PDT, Brady Eidson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Brady Eidson
Comment 1 2019-06-17 17:21:33 PDT
<rdar://problem/51392926>
Brady Eidson
Comment 2 2019-06-17 17:34:31 PDT
Created attachment 372300 [details] Patch
David Quesada
Comment 3 2019-06-17 23:05:42 PDT
Is it actually important to prevent the progress cancellation handler from calling cancel() twice? This could still technically happen if the NSProgress is canceled while the UI process cancels the download simultaneously - after one call to cancel() is made and the download is waiting for CFNetwork to deliver the resume data, the other call to cancel() can come in. Previously, this would have caused issues due to the Download's didCancel() being called multiple times, but as of r245901, it's safe to call Download::platformCancelNetworkLoad() multiple times. Technically we could add the "don't allow canceling multiple times" check to Download::cancel(), using the value of m_wasCanceled.
Brady Eidson
Comment 4 2019-06-18 07:26:51 PDT
I thought we’d stopped using the SPI path for cancel. Since we haven’t, you’re right. Also want to add some locking because while sleeping on it I realize this isn’t thread safe the way Foundation makes concurrent calls. New patch coming soon.
Brady Eidson
Comment 5 2019-06-18 12:59:34 PDT
Created attachment 372366 [details] Patch
Brady Eidson
Comment 6 2019-06-18 13:01:15 PDT
This new patch: - Handles the NSProgress cancellation handler being called on any thread - Handles it being called any number of times - Handles it being called *concurrently* on different threads - Handles WebKit::Download::cancel() being called multiple times (e.g. once from an NSProgress cancel and once from WebKit download SPI
WebKit Commit Bot
Comment 7 2019-06-18 14:30:24 PDT
Comment on attachment 372366 [details] Patch Clearing flags on attachment: 372366 Committed r246568: <https://trac.webkit.org/changeset/246568>
WebKit Commit Bot
Comment 8 2019-06-18 14:30:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.