Bug 222201 - CrashTracer: com.apple.WebKit.Networking at WebKit: WTF::Detail::CallableWrapper<WebKit::PrivateClickMeasurementManager::firePendingAttributionRequests()
Summary: CrashTracer: com.apple.WebKit.Networking at WebKit: WTF::Detail::CallableWrap...
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: katherine_cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-19 14:32 PST by katherine_cheney
Modified: 2021-02-19 16:19 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.42 KB, patch)
2021-02-19 14:33 PST, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (2.43 KB, patch)
2021-02-19 15:19 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description katherine_cheney 2021-02-19 14:32:10 PST
There is a crash happening in PrivateClickMeasurementManager::firePendingAttributionRequests()
Comment 1 katherine_cheney 2021-02-19 14:33:53 PST
Created attachment 421033 [details]
Patch
Comment 2 katherine_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 katherine_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 katherine_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>