Bug 200897 - NetworkDataTask is being ref'd / deref'd from several threads and is not ThreadSafeRefCounted
Summary: NetworkDataTask is being ref'd / deref'd from several threads and is not Thre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 198318
  Show dependency treegraph
 
Reported: 2019-08-19 13:57 PDT by Chris Dumez
Modified: 2019-08-19 15:51 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.32 KB, patch)
2019-08-19 15:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-08-19 13:57:41 PDT
NetworkDataTask is being ref'd / deref'd from several threads and is not ThreadSafeRefCounted.

See the makeRef() calls below:

void NetworkDataTaskCocoa::resume()
{
    if (m_scheduledFailureType != NoFailure)
        m_failureTimer.startOneShot(0_s);

    auto& cocoaSession = static_cast<NetworkSessionCocoa&>(*m_session);
    if (cocoaSession.deviceManagementRestrictionsEnabled() && m_isForMainResourceNavigationForAnyFrame) {
        auto didDetermineDeviceRestrictionPolicyForURL = makeBlockPtr([this, protectedThis = makeRef(*this)](BOOL isBlocked) {
            callOnMainThread([this, protectedThis = makeRef(*this), isBlocked] {
                if (isBlocked) {
                    scheduleFailure(RestrictedURLFailure);
                    return;
                }

                [m_task resume];
            });
        });
Comment 1 Chris Dumez 2019-08-19 14:41:00 PDT
cc'ing Tim as he added this code. I am still confirming if this can really get called on a background thread or if the callOnMainThread() call is merely to make things asynchronous.
Comment 2 Chris Dumez 2019-08-19 15:14:01 PDT
(In reply to Chris Dumez from comment #1)
> cc'ing Tim as he added this code. I am still confirming if this can really
> get called on a background thread or if the callOnMainThread() call is
> merely to make things asynchronous.

Ouch, definitely not called on the main thread. There is indeed a bug here.
Comment 3 Chris Dumez 2019-08-19 15:17:56 PDT
Created attachment 376712 [details]
Patch
Comment 4 Geoffrey Garen 2019-08-19 15:23:02 PDT
Comment on attachment 376712 [details]
Patch

r=me
Comment 5 WebKit Commit Bot 2019-08-19 15:50:47 PDT
Comment on attachment 376712 [details]
Patch

Clearing flags on attachment: 376712

Committed r248874: <https://trac.webkit.org/changeset/248874>
Comment 6 WebKit Commit Bot 2019-08-19 15:50:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-08-19 15:51:19 PDT
<rdar://problem/54487810>