Bug 222201

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, bfulgham, cdumez, ddkilzer, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Kate Cheney 2021-02-19 14:32:10 PST
There is a crash happening in PrivateClickMeasurementManager::firePendingAttributionRequests()
Comment 1 Kate Cheney 2021-02-19 14:33:53 PST
Created attachment 421033 [details]
Patch
Comment 2 Kate Cheney 2021-02-19 14:34:40 PST
<rdar://problem/74410516>
Comment 3 Alex Christensen 2021-02-19 14:36:47 PST
Comment on attachment 421033 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +

Could you put why you think this will fix the crash?
Comment 4 Kate Cheney 2021-02-19 14:48:05 PST
(In reply to Alex Christensen from comment #3)
> Comment on attachment 421033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421033&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +
> 
> Could you put why you think this will fix the crash?

Yes, will update in the next patch. TL;DR: it is crashing here:

bool PrivateClickMeasurementManager::featureEnabled() const
{
    return m_networkSession && m_networkProcess->privateClickMeasurementEnabled() && !m_sessionID.isEphemeral();
}

thinking it's possible that m_networkProcess is dead when we try to check for m_networkProcess->privateClickMeasurementEnabled().
Comment 5 Brent Fulgham 2021-02-19 14:54:56 PST
Comment on attachment 421033 [details]
Patch

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

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:57
> +    , m_networkProcess(makeWeakPtr(networkProcess))

This is probably the right architecture -- we don't own the network process (i.e. we shouldn't be the thing keeping it alive), so a weakptr makes sense.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:-426
> -    return m_networkSession && m_networkProcess->privateClickMeasurementEnabled() && !m_sessionID.isEphemeral();

It's a bummer that we can't figure out which of these things is the source of the issue. Perhaps it would be useful to early return if !m_networkSession and !m_networkProcess.
Comment 6 John Wilander 2021-02-19 15:18:15 PST
(In reply to Brent Fulgham from comment #5)
> Comment on attachment 421033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421033&action=review
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:57
> > +    , m_networkProcess(makeWeakPtr(networkProcess))
> 
> This is probably the right architecture -- we don't own the network process
> (i.e. we shouldn't be the thing keeping it alive), so a weakptr makes sense.
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:-426
> > -    return m_networkSession && m_networkProcess->privateClickMeasurementEnabled() && !m_sessionID.isEphemeral();
> 
> It's a bummer that we can't figure out which of these things is the source
> of the issue. Perhaps it would be useful to early return if
> !m_networkSession and !m_networkProcess.

Yeah, I'm in favor of splitting it into three separate conditionals just to give ourselves the information if it keeps crashing.
Comment 7 Alex Christensen 2021-02-19 15:18:46 PST
I don't think this will fix your crash
Comment 8 Alex Christensen 2021-02-19 15:19:53 PST
Created attachment 421043 [details]
Patch
Comment 9 Kate Cheney 2021-02-19 15:21:13 PST
(In reply to Alex Christensen from comment #8)
> Created attachment 421043 [details]
> Patch

This makes so much sense.
Comment 10 Chris Dumez 2021-02-19 15:26:03 PST
(In reply to katherine_cheney from comment #9)
> (In reply to Alex Christensen from comment #8)
> > Created attachment 421043 [details]
> > Patch
> 
> This makes so much sense.

Yes, capturing |this| in a lambda without any kind of lifetime management rarely works out.
Comment 11 EWS 2021-02-19 16:19:00 PST
Committed r273180: <https://commits.webkit.org/r273180>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421043 [details].
Comment 12 Radar WebKit Bug Importer 2021-02-19 16:19:15 PST
<rdar://problem/74542106>