Bug 198945 - Handle NSProgress calling our cancellation handler on background threads (and calling it more than once)
Summary: Handle NSProgress calling our cancellation handler on background threads (and...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-17 17:21 PDT by Brady Eidson
Modified: 2019-06-18 14:30 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.37 KB, patch)
2019-06-17 17:34 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2019-06-18 12:59 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2019-06-17 17:21:20 PDT
Handle NSProgress calling our cancellation handler on background threads (and calling it more than once)
Comment 1 Brady Eidson 2019-06-17 17:21:33 PDT
<rdar://problem/51392926>
Comment 2 Brady Eidson 2019-06-17 17:34:31 PDT
Created attachment 372300 [details]
Patch
Comment 3 David Quesada 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.
Comment 4 Brady Eidson 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.
Comment 5 Brady Eidson 2019-06-18 12:59:34 PDT
Created attachment 372366 [details]
Patch
Comment 6 Brady Eidson 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
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2019-06-18 14:30:25 PDT
All reviewed patches have been landed.  Closing bug.