Spec: Private Aggregation error reporting #229
Conversation
alexmturner
left a comment
alexmturner
left a comment
There was a problem hiding this comment.
@xyaoinum, could you PTAL? Had a question about how best to write this (see comment). Thanks!
Also note that the spec build is failing due to some missing exports, but I have a different PR to fix that in the PAA repo that I'll land first.
spec.bs
Outdated
|
|
||
| Note: This indicates that either |operationCtor|'s run() method encounters an error (where |operationCtor| is the parameter in {{SharedStorageWorkletGlobalScope/register()}}), or the result |index| is a non-integer value, which violates the selectURL() protocol, and we don't know which url should be selected. | ||
| 1. Run |privateAggregationCompletionTask|. | ||
| 1. If the promise was rejected as the operation was completed abruptly due to an uncaught exception: |
There was a problem hiding this comment.
Wasn't sure how to evaluate this properly (in spec language) unless we decide that returning a non-integer should count as an uncaught exception for aggregate error reporting
There was a problem hiding this comment.
A non-integer will often be implicitly converted to an integer without error, see test case SelectURL_NonNumericStringConvertedTo0. AFAICT, the only possible integer conversion failure is to return an object that defines a custom toString() method that throws, e.g. SelectURL_ReturnValueToUint32Error. I think it's fine to simply assume 'uncaught exception' here.
There was a problem hiding this comment.
Ah interesting -- I've updated this PR to reflect that / remove the unnecessary if clause. I'll try to add a test case for this situation in the implementation too. Thanks!
spec.bs
Outdated
|
|
||
| Note: This indicates that either |operationCtor|'s run() method encounters an error (where |operationCtor| is the parameter in {{SharedStorageWorkletGlobalScope/register()}}), or the result |index| is a non-integer value, which violates the selectURL() protocol, and we don't know which url should be selected. | ||
| 1. Run |privateAggregationCompletionTask|. | ||
| 1. If the promise was rejected as the operation was completed abruptly due to an uncaught exception: |
There was a problem hiding this comment.
A non-integer will often be implicitly converted to an integer without error, see test case SelectURL_NonNumericStringConvertedTo0. AFAICT, the only possible integer conversion failure is to return an object that defines a custom toString() method that throws, e.g. SelectURL_ReturnValueToUint32Error. I think it's fine to simply assume 'uncaught exception' here.
Adds support for the new Private Aggregation error reporting feature to Shared Storage. See the related PAA spec change: patcg-individual-drafts/private-aggregation-api#172 Also see the explainer: https://github.com/patcg-individual-drafts/private-aggregation-api/blob/main/error_reporting.md Slightly reorganizes the PAA integration with this spec for readability.
alexmturner
force-pushed
the
aggregate_error_reporting
branch
from
de7b139 to
80e2d7c
Compare
|
Thanks for the review! Fixed the build and updated the 'uncaught exception' test. |
Adds support for the new feature and its contributeToHistogramOnEvent() method. Requires plumbing the cause of a Shared Storage operation completing to the PrivateAggregation object to support contributions conditional on an uncaught exception. Spec PR: WICG/shared-storage#229 Bug: 381788013 Change-Id: I42a3a2e21a948263a22f5b439c0ec92ef9791932 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6334249 Commit-Queue: Alex Turner <alexmt@chromium.org> Reviewed-by: Yao Xiao <yaoxia@chromium.org> Cr-Commit-Position: refs/heads/main@{#1434576}
Adds support for the new Private Aggregation error reporting feature to Shared Storage. See the related PAA spec change:
patcg-individual-drafts/private-aggregation-api#172. Also see the explainer:
https://github.com/patcg-individual-drafts/private-aggregation-api/blob/main/error_reporting.md
Slightly reorganizes the PAA integration with this spec for readability.
Preview | Diff