Bug 219860

Summary: CrashTracer: com.apple.WebKit.Networking at WebKit: WTF::Detail::CallableWrapper<WebKit::PrivateClickMeasurementManager::firePendingAttributionRequests()
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ddkilzer, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Kate Cheney 2020-12-14 10:22:29 PST 
We are hitting this crash after PCM code was migrated to use SQLite.
Comment 1 Kate Cheney 2020-12-14 10:55:52 PST 
Created attachment 416172 [details]
Patch
Comment 2 Kate Cheney 2020-12-14 10:56:28 PST 
<rdar://problem/72254207>
Comment 3 Alex Christensen 2020-12-14 11:53:23 PST 
Comment on attachment 416172 [details]
Patch

From the description, I think this should just add this:
if (!m_networkSession)
    return;
Comment 4 Kate Cheney 2020-12-14 11:58:27 PST 
(In reply to Alex Christensen from comment #3)
> Comment on attachment 416172 [details]
> Patch
> 
> From the description, I think this should just add this:
> if (!m_networkSession)
>     return;

I thought if (auto* resourceLoadStatistics = m_networkSession->resourceLoadStatistics()) would check m_networkSession but maybe I am wrong? If so I should check m_networkSession in other PrivateClickMeasurementManager functions to be consistent.
Comment 5 Alex Christensen 2020-12-14 11:59:19 PST 
if (auto* resourceLoadStatistics = m_networkSession ? m_networkSession->resourceLoadStatistics() : nullptr) checks m_networkSession, but what you have doesn't.
Comment 6 Kate Cheney 2020-12-14 12:10:19 PST 
Created attachment 416180 [details]
Patch
Comment 7 EWS 2020-12-14 12:59:14 PST 
Committed r270802: <https://trac.webkit.org/changeset/270802>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416180 [details].
Comment 8 Radar WebKit Bug Importer 2020-12-14 13:01:44 PST 
<rdar://problem/72310410>
Comment 9 John Wilander 2020-12-14 17:34:23 PST 
Comment on attachment 416180 [details]
Patch

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

An easier way to fix this is to move the null check into PrivateClickMeasurementManager::featureEnabled().

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:302
> +        return;

We need to also call the completion handler here.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:342
> +        return;

We need to also call the completion handler here.
Comment 10 Alex Christensen 2020-12-14 18:25:13 PST 
Comment on attachment 416180 [details]
Patch

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

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:302
>> +        return;
> 
> We need to also call the completion handler here.

yes we certainly do.  Also in the case before if resourceLoadStatistics is null.
Comment 11 Kate Cheney 2020-12-15 08:54:02 PST 
Reopening to attach new patch.
Comment 12 Kate Cheney 2020-12-15 08:54:03 PST 
Created attachment 416253 [details]
Patch
Comment 13 EWS 2020-12-15 10:28:21 PST 
Committed r270852: <https://trac.webkit.org/changeset/270852>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416253 [details].