Bug 229551

Summary: PrivateClickMeasurementManager::firePendingAttributionRequests() is crashing in debug
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebKit Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: katherine_cheney, webkit-bot-watchers-bugzilla, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=229550
Attachments:
Description Flags
Patch
none
Patch none

Description youenn fablet 2021-08-26 05:29:28 PDT Comment hidden (obsolete)
Comment 1 youenn fablet 2021-08-26 05:31:35 PDT
Created attachment 436501 [details]
Patch
Comment 2 youenn fablet 2021-08-26 05:31:45 PDT
<rdar://81997710>
Comment 3 youenn fablet 2021-08-26 05:32:57 PDT
Bug 229550 might be explained by this crash.
Comment 4 Kate Cheney 2021-08-26 07:57:12 PDT
Comment on attachment 436501 [details]
Patch

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

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:423
> +                // Attribution has already been sent.

I am not sure this is the right solution. I added an ASSERT here because I thought this code path was not possible. We should figure out why it is being hit instead of removing the ASSERT.
Comment 5 youenn fablet 2021-08-26 08:36:46 PDT
(In reply to Kate Cheney from comment #4)
> Comment on attachment 436501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436501&action=review
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:423
> > +                // Attribution has already been sent.
> 
> I am not sure this is the right solution. I added an ASSERT here because I
> thought this code path was not possible. We should figure out why it is
> being hit instead of removing the ASSERT.

Agreed it might be fine to investigate why this assert is buggy.
But this disrupts tests so it seems good to remove this assert as soon as possible.
I am fine adding release logging instead for now.
Comment 6 Kate Cheney 2021-08-26 15:47:14 PDT
Created attachment 436579 [details]
Patch
Comment 7 John Wilander 2021-08-26 16:06:56 PDT
r=me
Comment 8 EWS 2021-08-27 09:30:53 PDT
Committed r281697 (241047@main): <https://commits.webkit.org/241047@main>

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