Bug 222201 – CrashTracer: com.apple.WebKit.Networking at WebKit: WTF::Detail::CallableWrapper<WebKit::PrivateClickMeasurementManager::firePendingAttributionRequests()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

There is a crash happening in PrivateClickMeasurementManager::firePendingAttributionRequests()

[Kate Cheney]

Created attachment 421033 [details]
Patch

[Kate Cheney]

<rdar://problem/74410516>

[Alex Christensen]

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?

[Kate Cheney]

(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().

[Brent Fulgham]

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.

[John Wilander]

(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.

[Alex Christensen]

I don't think this will fix your crash

[Alex Christensen]

Created attachment 421043 [details]
Patch

[Kate Cheney]

(In reply to Alex Christensen from comment #8)
> Created attachment 421043 [details]
> Patch

This makes so much sense.

[Chris Dumez]

(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.

[EWS]

Committed r273180: <https://commits.webkit.org/r273180>

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

[Radar WebKit Bug Importer]

<rdar://problem/74542106>