Bug 229551 - PrivateClickMeasurementManager::firePendingAttributionRequests() is crashing in debug
Summary: PrivateClickMeasurementManager::firePendingAttributionRequests() is crashing ...
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: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-26 05:29 PDT by youenn fablet
Modified: 2021-08-27 09:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.15 KB, patch)
2021-08-26 05:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (1.50 KB, patch)
2021-08-26 15:47 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].