Bug 221555 – PCM: expired reports get sent at the same time after a session restart

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

Multiple reports sent simultaneously to the same destination could link a user across sites. 

<rdar://problem/73724816>

[Kate Cheney]

Created attachment 419602 [details]
Patch

[Chris Dumez]

Comment on attachment 419602 [details]
Patch

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

> Source/WebCore/loader/PrivateClickMeasurement.h:46
> +    bool operator==(PrivateClickMeasurement const other) const

Did you mean `const PrivateClickMeasurement& other` ?

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1483
>          return;

BUG: You're not calling your completion handler here.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1487
> +            completionHandler();

BUG: Calling the completion handler on the wrong thread.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:197
> +    auto callbackAggregator = WTF::CallbackAggregator::create(WTFMove(completionHandler));

No WTF::

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:216
> +    resourceLoadStatistics->allAttributedPrivateClickMeasurement([this] (auto&& attributions) {

What guarantees |this| is still alive by the time the lambda runs.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:225
> +

Unnecessary blank line

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:234
> +        auto interval = debugModeEnabled() ? 120 : (900 + (cryptographicallyRandomNumber() % 900));

It would look nicer if you used the Second type here. E.g. 120 -> 2_min

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:251
> +    auto expiredAttribution = m_expiredAttributions.first();

takeFirst() ?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:253
> +    m_expiredAttributions.removeFirst(expiredAttribution);

Then we wouldn't need this.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:89
> +    Vector<PrivateClickMeasurement> m_expiredAttributions;

You likely want a Deque since you remove from the head. Vector is a very poor data structure for such operation.

[Kate Cheney]

Created attachment 419619 [details]
Patch

[Kate Cheney]

Thanks Chris, fixes are addressed in the latest patch.

[John Wilander]

Comment on attachment 419619 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        PCM: expired reports get sent at the same time after a session restart

I'd use capital E for Expired. :)

> Source/WebKit/ChangeLog:9
> +        Since PCM reports are now persisted, we need to address the

It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.

> Source/WebKit/ChangeLog:17
> +        remaining expired attributions are inserted back into the database to be

Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?

> Source/WebKit/ChangeLog:20
> +        this is probably unlikely.

We could also point out here that protecting the user's privacy is a hard requirement so even in the case of "starvation" where some reports never get sent out, we think that's the right tradeoff.

> Source/WebKit/ChangeLog:28
> +        confirm no regressions.

You could augment the existing dump function just like we do for checking the regular scheduling of reports, i.e. check that the trigger time is between certain timestamps.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:393
> +    if (!m_privateClickMeasurement)

Is m_privateClickMeasurement expected to be null under any normal conditions? If so, we may have to log something in Web Inspector to tell developers that something went wrong.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:194
> +void PrivateClickMeasurementManager::flushAttributions(CompletionHandler<void()>&& completionHandler)

flushAttributions() is a rather vague name. How would you explain it with more words? I think we can come up with a  more descriptive name. Looking at the code it seems like reinsertExpiredAttributions() would be better but that isn't clear either because I don't know what expired attributions are (see comment further down).

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:209
> +void PrivateClickMeasurementManager::scheduleExpiredAttributionRequests()

We should add some Web Inspector logging so that developers can know what's going on if their report is not sent out immediately at session restart.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:89
> +    Deque<PrivateClickMeasurement> m_expiredAttributions;

What are expired attributions? To me they sound like "should be discarded because they're too old to send." Is that it?

[Kate Cheney]

Comment on attachment 419619 [details]
Patch

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

>> Source/WebKit/ChangeLog:3
>> +        PCM: expired reports get sent at the same time after a session restart
> 
> I'd use capital E for Expired. :)

Will fix.

>> Source/WebKit/ChangeLog:9
>> +        Since PCM reports are now persisted, we need to address the
> 
> It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.

Ditto, will fix.

>> Source/WebKit/ChangeLog:17
>> +        remaining expired attributions are inserted back into the database to be
> 
> Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?

My inclination is to think didClose() is still called in a crash but I need to double check to confirm. If not, we would lose reports that were past due to send at session-start but not any other attribution reports, because those are stored on disk until they are ready to be sent.

>> Source/WebKit/ChangeLog:20
>> +        this is probably unlikely.
> 
> We could also point out here that protecting the user's privacy is a hard requirement so even in the case of "starvation" where some reports never get sent out, we think that's the right tradeoff.

Good idea, I'll add that.

>> Source/WebKit/ChangeLog:28
>> +        confirm no regressions.
> 
> You could augment the existing dump function just like we do for checking the regular scheduling of reports, i.e. check that the trigger time is between certain timestamps.

Good idea, I could probably do something like this that would at least test some functionality.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:393
>> +    if (!m_privateClickMeasurement)
> 
> Is m_privateClickMeasurement expected to be null under any normal conditions? If so, we may have to log something in Web Inspector to tell developers that something went wrong.

I am not sure of a case where m_privateClickMeasurement would be null. I added this because it is possible for m_privateClickMeasurement to be null now that is no longer a reference, so it seemed like the safe thing to do. Maybe Chris or Alex knows a case in which the NetworkSession object would exist but not a unique_ptr initialized in the constructor.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:194
>> +void PrivateClickMeasurementManager::flushAttributions(CompletionHandler<void()>&& completionHandler)
> 
> flushAttributions() is a rather vague name. How would you explain it with more words? I think we can come up with a  more descriptive name. Looking at the code it seems like reinsertExpiredAttributions() would be better but that isn't clear either because I don't know what expired attributions are (see comment further down).

I will update this to be "reinsert<betterNameForExpiredAttributions>". Maybe "pastDueToReport" or something along those lines.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:209
>> +void PrivateClickMeasurementManager::scheduleExpiredAttributionRequests()
> 
> We should add some Web Inspector logging so that developers can know what's going on if their report is not sent out immediately at session restart.

Ok.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:89
>> +    Deque<PrivateClickMeasurement> m_expiredAttributions;
> 
> What are expired attributions? To me they sound like "should be discarded because they're too old to send." Is that it?

No, in this case they are attributions that have reached their earliestTimeToSend value while the session was closed, and we don't want to send them in a burst on session-start. Maybe "overdueAttributionsToReport" or "pastDueAttributionsToSendOnSessionStart"?

[Chris Dumez]

Comment on attachment 419619 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkSession.cpp:393
>>> +    if (!m_privateClickMeasurement)
>> 
>> Is m_privateClickMeasurement expected to be null under any normal conditions? If so, we may have to log something in Web Inspector to tell developers that something went wrong.
> 
> I am not sure of a case where m_privateClickMeasurement would be null. I added this because it is possible for m_privateClickMeasurement to be null now that is no longer a reference, so it seemed like the safe thing to do. Maybe Chris or Alex knows a case in which the NetworkSession object would exist but not a unique_ptr initialized in the constructor.

Those null-checks are not needed. Just add such a getter in the header:
PrivateClickMeasurementManager& privateClickMeasurement() { return *m_privateClickMeasurement; }

[Chris Dumez]

Comment on attachment 419619 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1488
> +        if (!m_statisticsStore) {

This would look better (more concise) like so:
```
if (m_statisticsStore)
    m_statisticsStore->insertPrivateClickMeasurement(WTFMove(attribution), attributionType);
postTaskReply(WTFMove(completionHandler));
```

[Chris Dumez]

Comment on attachment 419619 [details]
Patch

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

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:198
> +        return;

BUG: CompletionHandler is not called...

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:206
> +#endif

#else
you need to call the completion handler..

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:252
> +    auto expiredAttribution = m_expiredAttributions.takeFirst();

We don't need this local variable.

[John Wilander]

(In reply to katherine_cheney from comment #6)
> Comment on attachment 419619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419619&action=review
> 
> >> Source/WebKit/ChangeLog:3
> >> +        PCM: expired reports get sent at the same time after a session restart
> > 
> > I'd use capital E for Expired. :)
> 
> Will fix.
> 
> >> Source/WebKit/ChangeLog:9
> >> +        Since PCM reports are now persisted, we need to address the
> > 
> > It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.
> 
> Ditto, will fix.
> 
> >> Source/WebKit/ChangeLog:17
> >> +        remaining expired attributions are inserted back into the database to be
> > 
> > Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?
> 
> My inclination is to think didClose() is still called in a crash but I need
> to double check to confirm. If not, we would lose reports that were past due
> to send at session-start but not any other attribution reports, because
> those are stored on disk until they are ready to be sent.

OK. My thinking here is that for all other cases, we keep data in the DB until it's time to send. My assumption was that we would do the same thing here and instead of putting all the pending data in memory, instead set timers to one-by-one send out overdue reports. Kind of like:

1. Mark all overdue reports as "overdue" in the DB.
2. If we have overdue reports, send one overdue report out and set a timer for N minutes, else done.
3. When the timer fires, run from step 2.

> >> Source/WebKit/ChangeLog:20
> >> +        this is probably unlikely.
> > 
> > We could also point out here that protecting the user's privacy is a hard requirement so even in the case of "starvation" where some reports never get sent out, we think that's the right tradeoff.
> 
> Good idea, I'll add that.
> 
> >> Source/WebKit/ChangeLog:28
> >> +        confirm no regressions.
> > 
> > You could augment the existing dump function just like we do for checking the regular scheduling of reports, i.e. check that the trigger time is between certain timestamps.
> 
> Good idea, I could probably do something like this that would at least test
> some functionality.
> 
> >> Source/WebKit/NetworkProcess/NetworkSession.cpp:393
> >> +    if (!m_privateClickMeasurement)
> > 
> > Is m_privateClickMeasurement expected to be null under any normal conditions? If so, we may have to log something in Web Inspector to tell developers that something went wrong.
> 
> I am not sure of a case where m_privateClickMeasurement would be null. I
> added this because it is possible for m_privateClickMeasurement to be null
> now that is no longer a reference, so it seemed like the safe thing to do.
> Maybe Chris or Alex knows a case in which the NetworkSession object would
> exist but not a unique_ptr initialized in the constructor.

No reason to add Web Inspector logging then.

> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:194
> >> +void PrivateClickMeasurementManager::flushAttributions(CompletionHandler<void()>&& completionHandler)
> > 
> > flushAttributions() is a rather vague name. How would you explain it with more words? I think we can come up with a  more descriptive name. Looking at the code it seems like reinsertExpiredAttributions() would be better but that isn't clear either because I don't know what expired attributions are (see comment further down).
> 
> I will update this to be "reinsert<betterNameForExpiredAttributions>". Maybe
> "pastDueToReport" or something along those lines.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:209
> >> +void PrivateClickMeasurementManager::scheduleExpiredAttributionRequests()
> > 
> > We should add some Web Inspector logging so that developers can know what's going on if their report is not sent out immediately at session restart.
> 
> Ok.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:89
> >> +    Deque<PrivateClickMeasurement> m_expiredAttributions;
> > 
> > What are expired attributions? To me they sound like "should be discarded because they're too old to send." Is that it?
> 
> No, in this case they are attributions that have reached their
> earliestTimeToSend value while the session was closed, and we don't want to
> send them in a burst on session-start. Maybe "overdueAttributionsToReport"
> or "pastDueAttributionsToSendOnSessionStart"?

I like overdue. Much clearer!

[Kate Cheney]

(In reply to John Wilander from comment #10)
> (In reply to katherine_cheney from comment #6)
> > Comment on attachment 419619 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=419619&action=review
> > 
> > >> Source/WebKit/ChangeLog:3
> > >> +        PCM: expired reports get sent at the same time after a session restart
> > > 
> > > I'd use capital E for Expired. :)
> > 
> > Will fix.
> > 
> > >> Source/WebKit/ChangeLog:9
> > >> +        Since PCM reports are now persisted, we need to address the
> > > 
> > > It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.
> > 
> > Ditto, will fix.
> > 
> > >> Source/WebKit/ChangeLog:17
> > >> +        remaining expired attributions are inserted back into the database to be
> > > 
> > > Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?
> > 
> > My inclination is to think didClose() is still called in a crash but I need
> > to double check to confirm. If not, we would lose reports that were past due
> > to send at session-start but not any other attribution reports, because
> > those are stored on disk until they are ready to be sent.
> 
> OK. My thinking here is that for all other cases, we keep data in the DB
> until it's time to send. My assumption was that we would do the same thing
> here and instead of putting all the pending data in memory, instead set
> timers to one-by-one send out overdue reports. Kind of like:
> 
> 1. Mark all overdue reports as "overdue" in the DB.
> 2. If we have overdue reports, send one overdue report out and set a timer
> for N minutes, else done.
> 3. When the timer fires, run from step 2.
> 

Ah! This is a good idea, and will make things simpler. To avoid a schema change we can maybe use "-1.0" in the earliestTimeToSend column to indicate an overdue report. This will also make it more reasonable to test.

[John Wilander]

(In reply to katherine_cheney from comment #11)
> (In reply to John Wilander from comment #10)
> > (In reply to katherine_cheney from comment #6)
> > > Comment on attachment 419619 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=419619&action=review
> > > 
> > > >> Source/WebKit/ChangeLog:3
> > > >> +        PCM: expired reports get sent at the same time after a session restart
> > > > 
> > > > I'd use capital E for Expired. :)
> > > 
> > > Will fix.
> > > 
> > > >> Source/WebKit/ChangeLog:9
> > > >> +        Since PCM reports are now persisted, we need to address the
> > > > 
> > > > It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.
> > > 
> > > Ditto, will fix.
> > > 
> > > >> Source/WebKit/ChangeLog:17
> > > >> +        remaining expired attributions are inserted back into the database to be
> > > > 
> > > > Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?
> > > 
> > > My inclination is to think didClose() is still called in a crash but I need
> > > to double check to confirm. If not, we would lose reports that were past due
> > > to send at session-start but not any other attribution reports, because
> > > those are stored on disk until they are ready to be sent.
> > 
> > OK. My thinking here is that for all other cases, we keep data in the DB
> > until it's time to send. My assumption was that we would do the same thing
> > here and instead of putting all the pending data in memory, instead set
> > timers to one-by-one send out overdue reports. Kind of like:
> > 
> > 1. Mark all overdue reports as "overdue" in the DB.
> > 2. If we have overdue reports, send one overdue report out and set a timer
> > for N minutes, else done.
> > 3. When the timer fires, run from step 2.
> > 
> 
> Ah! This is a good idea, and will make things simpler. To avoid a schema
> change we can maybe use "-1.0" in the earliestTimeToSend column to indicate
> an overdue report. This will also make it more reasonable to test.

Marking them as overdue may even be premature optimization. You might get away with:

1. Check if we have any overdue reports.
2. At the first found overdue report, send it out and set a timer for N minutes to start over. (I.e. no need to continues looking in the DB until the timer fires.)

[John Wilander]

(In reply to John Wilander from comment #12)
> (In reply to katherine_cheney from comment #11)
> > (In reply to John Wilander from comment #10)
> > > (In reply to katherine_cheney from comment #6)
> > > > Comment on attachment 419619 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=419619&action=review
> > > > 
> > > > >> Source/WebKit/ChangeLog:3
> > > > >> +        PCM: expired reports get sent at the same time after a session restart
> > > > > 
> > > > > I'd use capital E for Expired. :)
> > > > 
> > > > Will fix.
> > > > 
> > > > >> Source/WebKit/ChangeLog:9
> > > > >> +        Since PCM reports are now persisted, we need to address the
> > > > > 
> > > > > It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.
> > > > 
> > > > Ditto, will fix.
> > > > 
> > > > >> Source/WebKit/ChangeLog:17
> > > > >> +        remaining expired attributions are inserted back into the database to be
> > > > > 
> > > > > Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?
> > > > 
> > > > My inclination is to think didClose() is still called in a crash but I need
> > > > to double check to confirm. If not, we would lose reports that were past due
> > > > to send at session-start but not any other attribution reports, because
> > > > those are stored on disk until they are ready to be sent.
> > > 
> > > OK. My thinking here is that for all other cases, we keep data in the DB
> > > until it's time to send. My assumption was that we would do the same thing
> > > here and instead of putting all the pending data in memory, instead set
> > > timers to one-by-one send out overdue reports. Kind of like:
> > > 
> > > 1. Mark all overdue reports as "overdue" in the DB.
> > > 2. If we have overdue reports, send one overdue report out and set a timer
> > > for N minutes, else done.
> > > 3. When the timer fires, run from step 2.
> > > 
> > 
> > Ah! This is a good idea, and will make things simpler. To avoid a schema
> > change we can maybe use "-1.0" in the earliestTimeToSend column to indicate
> > an overdue report. This will also make it more reasonable to test.
> 
> Marking them as overdue may even be premature optimization. You might get
> away with:
> 
> 1. Check if we have any overdue reports.
> 2. At the first found overdue report, send it out and set a timer for N
> minutes to start over. (I.e. no need to continues looking in the DB until
> the timer fires.)

The nice part of doing it as described above, is that you'll never end up in "I have have some reports already marked as overdue but I was suspended so I need to check anyway."

[Kate Cheney]

(In reply to John Wilander from comment #12)
> (In reply to katherine_cheney from comment #11)
> > (In reply to John Wilander from comment #10)
> > > (In reply to katherine_cheney from comment #6)
> > > > Comment on attachment 419619 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=419619&action=review
> > > > 
> > > > >> Source/WebKit/ChangeLog:3
> > > > >> +        PCM: expired reports get sent at the same time after a session restart
> > > > > 
> > > > > I'd use capital E for Expired. :)
> > > > 
> > > > Will fix.
> > > > 
> > > > >> Source/WebKit/ChangeLog:9
> > > > >> +        Since PCM reports are now persisted, we need to address the
> > > > > 
> > > > > It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.
> > > > 
> > > > Ditto, will fix.
> > > > 
> > > > >> Source/WebKit/ChangeLog:17
> > > > >> +        remaining expired attributions are inserted back into the database to be
> > > > > 
> > > > > Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?
> > > > 
> > > > My inclination is to think didClose() is still called in a crash but I need
> > > > to double check to confirm. If not, we would lose reports that were past due
> > > > to send at session-start but not any other attribution reports, because
> > > > those are stored on disk until they are ready to be sent.
> > > 
> > > OK. My thinking here is that for all other cases, we keep data in the DB
> > > until it's time to send. My assumption was that we would do the same thing
> > > here and instead of putting all the pending data in memory, instead set
> > > timers to one-by-one send out overdue reports. Kind of like:
> > > 
> > > 1. Mark all overdue reports as "overdue" in the DB.
> > > 2. If we have overdue reports, send one overdue report out and set a timer
> > > for N minutes, else done.
> > > 3. When the timer fires, run from step 2.
> > > 
> > 
> > Ah! This is a good idea, and will make things simpler. To avoid a schema
> > change we can maybe use "-1.0" in the earliestTimeToSend column to indicate
> > an overdue report. This will also make it more reasonable to test.
> 
> Marking them as overdue may even be premature optimization. You might get
> away with:
> 
> 1. Check if we have any overdue reports.
> 2. At the first found overdue report, send it out and set a timer for N
> minutes to start over. (I.e. no need to continues looking in the DB until
> the timer fires.)

Do you envision this being separate from the m_firePendingAttributionRequestsTimer, which sends pending reports normally? Right now we send all attributions that are past due whenever the m_firePendingAttributionRequestsTimer goes off. We would have to somehow mark overdue ones so they don't get sent along with those, or make sure that call only sends one at a time instead of all overdue ones at once.

[John Wilander]

(In reply to katherine_cheney from comment #14)
> (In reply to John Wilander from comment #12)
> > (In reply to katherine_cheney from comment #11)
> > > (In reply to John Wilander from comment #10)
> > > > (In reply to katherine_cheney from comment #6)
> > > > > Comment on attachment 419619 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=419619&action=review
> > > > > 
> > > > > >> Source/WebKit/ChangeLog:3
> > > > > >> +        PCM: expired reports get sent at the same time after a session restart
> > > > > > 
> > > > > > I'd use capital E for Expired. :)
> > > > > 
> > > > > Will fix.
> > > > > 
> > > > > >> Source/WebKit/ChangeLog:9
> > > > > >> +        Since PCM reports are now persisted, we need to address the
> > > > > > 
> > > > > > It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.
> > > > > 
> > > > > Ditto, will fix.
> > > > > 
> > > > > >> Source/WebKit/ChangeLog:17
> > > > > >> +        remaining expired attributions are inserted back into the database to be
> > > > > > 
> > > > > > Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?
> > > > > 
> > > > > My inclination is to think didClose() is still called in a crash but I need
> > > > > to double check to confirm. If not, we would lose reports that were past due
> > > > > to send at session-start but not any other attribution reports, because
> > > > > those are stored on disk until they are ready to be sent.
> > > > 
> > > > OK. My thinking here is that for all other cases, we keep data in the DB
> > > > until it's time to send. My assumption was that we would do the same thing
> > > > here and instead of putting all the pending data in memory, instead set
> > > > timers to one-by-one send out overdue reports. Kind of like:
> > > > 
> > > > 1. Mark all overdue reports as "overdue" in the DB.
> > > > 2. If we have overdue reports, send one overdue report out and set a timer
> > > > for N minutes, else done.
> > > > 3. When the timer fires, run from step 2.
> > > > 
> > > 
> > > Ah! This is a good idea, and will make things simpler. To avoid a schema
> > > change we can maybe use "-1.0" in the earliestTimeToSend column to indicate
> > > an overdue report. This will also make it more reasonable to test.
> > 
> > Marking them as overdue may even be premature optimization. You might get
> > away with:
> > 
> > 1. Check if we have any overdue reports.
> > 2. At the first found overdue report, send it out and set a timer for N
> > minutes to start over. (I.e. no need to continues looking in the DB until
> > the timer fires.)
> 
> Do you envision this being separate from the
> m_firePendingAttributionRequestsTimer, which sends pending reports normally?
> Right now we send all attributions that are past due whenever the
> m_firePendingAttributionRequestsTimer goes off. We would have to somehow
> mark overdue ones so they don't get sent along with those, or make sure that
> call only sends one at a time instead of all overdue ones at once.

You're right. With zero memory of what we've done we have to make sure we don't send them prematurely.

You could make that timer send one report at a time and if there is at least one more (over)due report, set it to N minutes.

I think the goal should be to make it as simple as possible, both in code and for a human to understand. Whichever solution you think gets us there is probably the right one.

[Kate Cheney]

Created attachment 419655 [details]
Patch

[Kate Cheney]

(In reply to John Wilander from comment #15)
> (In reply to katherine_cheney from comment #14)
> > (In reply to John Wilander from comment #12)
> > > (In reply to katherine_cheney from comment #11)
> > > > (In reply to John Wilander from comment #10)
> > > > > (In reply to katherine_cheney from comment #6)
> > > > > > Comment on attachment 419619 [details]
> > > > > > Patch
> > > > > > 
> > > > > > View in context:
> > > > > > https://bugs.webkit.org/attachment.cgi?id=419619&action=review
> > > > > > 
> > > > > > >> Source/WebKit/ChangeLog:3
> > > > > > >> +        PCM: expired reports get sent at the same time after a session restart
> > > > > > > 
> > > > > > > I'd use capital E for Expired. :)
> > > > > > 
> > > > > > Will fix.
> > > > > > 
> > > > > > >> Source/WebKit/ChangeLog:9
> > > > > > >> +        Since PCM reports are now persisted, we need to address the
> > > > > > > 
> > > > > > > It's not the reports that are persisted. It's all PCM data. Reports only exist when they are being sent.
> > > > > > 
> > > > > > Ditto, will fix.
> > > > > > 
> > > > > > >> Source/WebKit/ChangeLog:17
> > > > > > >> +        remaining expired attributions are inserted back into the database to be
> > > > > > > 
> > > > > > > Does this reflect a real insert back? If so, do we risk losing pending reports at a crash?
> > > > > > 
> > > > > > My inclination is to think didClose() is still called in a crash but I need
> > > > > > to double check to confirm. If not, we would lose reports that were past due
> > > > > > to send at session-start but not any other attribution reports, because
> > > > > > those are stored on disk until they are ready to be sent.
> > > > > 
> > > > > OK. My thinking here is that for all other cases, we keep data in the DB
> > > > > until it's time to send. My assumption was that we would do the same thing
> > > > > here and instead of putting all the pending data in memory, instead set
> > > > > timers to one-by-one send out overdue reports. Kind of like:
> > > > > 
> > > > > 1. Mark all overdue reports as "overdue" in the DB.
> > > > > 2. If we have overdue reports, send one overdue report out and set a timer
> > > > > for N minutes, else done.
> > > > > 3. When the timer fires, run from step 2.
> > > > > 
> > > > 
> > > > Ah! This is a good idea, and will make things simpler. To avoid a schema
> > > > change we can maybe use "-1.0" in the earliestTimeToSend column to indicate
> > > > an overdue report. This will also make it more reasonable to test.
> > > 
> > > Marking them as overdue may even be premature optimization. You might get
> > > away with:
> > > 
> > > 1. Check if we have any overdue reports.
> > > 2. At the first found overdue report, send it out and set a timer for N
> > > minutes to start over. (I.e. no need to continues looking in the DB until
> > > the timer fires.)
> > 
> > Do you envision this being separate from the
> > m_firePendingAttributionRequestsTimer, which sends pending reports normally?
> > Right now we send all attributions that are past due whenever the
> > m_firePendingAttributionRequestsTimer goes off. We would have to somehow
> > mark overdue ones so they don't get sent along with those, or make sure that
> > call only sends one at a time instead of all overdue ones at once.
> 
> You're right. With zero memory of what we've done we have to make sure we
> don't send them prematurely.
> 
> You could make that timer send one report at a time and if there is at least
> one more (over)due report, set it to N minutes.
> 
> I think the goal should be to make it as simple as possible, both in code
> and for a human to understand. Whichever solution you think gets us there is
> probably the right one.

OK, much smaller patch uploaded. This one sends one report at a time and if more overdue reports exists it will start the timer to send again at a random time in the future between 15-30 minutes. It sorts by earliestTimeToSend so the most overdue reports will get sent first.

[John Wilander]

Comment on attachment 419655 [details]
Patch

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

I really like the simplicity of this design! Have a look at my comments on clearSentAttributions() etc though.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:223
> +                    clearSentAttributions(WTFMove(sentAttributions));

We call clearSentAttributions() here and move sentAttributions ...

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:225
> +                    break;

... the we don't return ...

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:236
>          clearSentAttributions(WTFMove(sentAttributions));

... and finally we call clearSentAttributions() and move sentAttributions again. That looks like a problem. If it is intended and works, please make a comment so that others understand.

[Kate Cheney]

Created attachment 419755 [details]
Patch

[Kate Cheney]

(In reply to John Wilander from comment #18)
> Comment on attachment 419655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419655&action=review
> 
> I really like the simplicity of this design! Have a look at my comments on
> clearSentAttributions() etc though.
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:223
> > +                    clearSentAttributions(WTFMove(sentAttributions));
> 
> We call clearSentAttributions() here and move sentAttributions ...
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:225
> > +                    break;
> 
> ... the we don't return ...
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:236
> >          clearSentAttributions(WTFMove(sentAttributions));
> 
> ... and finally we call clearSentAttributions() and move sentAttributions
> again. That looks like a problem. If it is intended and works, please make a
> comment so that others understand.

Yikes, you're right. The break should be a return -- new patch uploaded.

[John Wilander]

Comment on attachment 419755 [details]
Patch

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

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:207
>          Vector<PrivateClickMeasurement> sentAttributions;

Does this have to be a Vector now that we always just send zero or one report?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:218
> +                if (!sentAttributions.isEmpty()) {

This could now just be a boolean flag haveAlreadySentOneAttribution.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:228
>                  sentAttributions.append(WTFMove(attribution));

This could now instead be the call to clearSentAttribution(). Note singular.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:236
>          clearSentAttributions(WTFMove(sentAttributions));

This is probably not needed anymore.

[Kate Cheney]

Created attachment 419795 [details]
Patch

[John Wilander]

Comment on attachment 419795 [details]
Patch

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

I think we arrived at a neat and simple solution. r=me, bar EWS and the two minor comments.

> Source/WebKit/ChangeLog:1
> [phone]  Kate Cheney  <[e-mail]>

Nit: We've discussing this one so long that it's now Feb 9th. :) Maybe the cq takes care of that? I've never checked.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:233
> +            // Attributions are sorted by earliestTimeToSend, so the first time we hit this will always be the min value of nextTimeToFire.

I think I understand what you're saying here but maybe it can be said more clearly? I'm thinking "... the first time we hit this we know there can be no further attributions due for reporting."

[Kate Cheney]

Comment on attachment 419795 [details]
Patch

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

Thanks for the comments! This patch got a lot simpler with your suggestions.

>> Source/WebKit/ChangeLog:1
>> [phone]  Kate Cheney  <[e-mail]>
> 
> Nit: We've discussing this one so long that it's now Feb 9th. :) Maybe the cq takes care of that? I've never checked.

Will fix just in case.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:233
>> +            // Attributions are sorted by earliestTimeToSend, so the first time we hit this will always be the min value of nextTimeToFire.
> 
> I think I understand what you're saying here but maybe it can be said more clearly? I'm thinking "... the first time we hit this we know there can be no further attributions due for reporting."

That is also true, but the main point I am trying to get across with this comment is that in the past we iterated over all attributions to find the minimum earliestTimeToSend so that nextTimeToFire was as soon as possible. Since they are now sorted, we know that nextTimeToFire will be at its minimum value the first time we hit a report that is not yet overdue.

Maybe "Attributions are sorted by earliestTimeToSend, so the first time we hit this we know there can be no further attributions due for reporting, and nextTimeToFire is the minimum earliestTimeToSend value for any attribution."

[Kate Cheney]

Created attachment 419848 [details]
Patch for landing

[EWS]

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

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