Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec: Define per-context contribution limits #164

Conversation

dmcardle
Copy link
Contributor

@dmcardle dmcardle commented Oct 16, 2024

This change adds the web-visible maxContributions field, which enables some callers to request different numbers of contributions per report.

Per-context limits are being added to the explainer in #146.


Preview | Diff

spec.bs Outdated Show resolved Hide resolved
@dmcardle dmcardle marked this pull request as ready for review November 4, 2024 14:54
Copy link
Collaborator

@alexmturner alexmturner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Dan! Looks good -- just some nits

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated
@@ -1341,16 +1377,21 @@ WebIDL modifications {#protected-audience-api-webidl-modifications}
The {{AuctionAdConfig}} and {{AuctionAdInterestGroup}} dictionaries are
modified to add a new field:
<xmp class="idl">
dictionary ProtectedAudiencePrivateAggregationConfig {
dictionary ProtectedAudienceAuctionPrivateAggregationConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to rebase this PR after #166. Although as a general point -- wondering if we should make this change at the same time as we add contextId and filteringIdMaxBytes given they all require activating deterministic counts.

That being said, we'll probably want to make the Shared Storage spec changes soon

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up.

I'm planning to make the Protected Audience and Shared Storage spec PRs tomorrow. Let's merge those before this PR.

I'm rebasing now, so I'll have to delete this hunk of the diff. This is the relevant part for the Protected Audience PR:

@@ -1341,16 +1377,21 @@ WebIDL modifications {#protected-audience-api-webidl-modifications}
 The {{AuctionAdConfig}} and {{AuctionAdInterestGroup}} dictionaries are
 modified to add a new field:
 <xmp class="idl">
-dictionary ProtectedAudiencePrivateAggregationConfig {
+dictionary ProtectedAudienceAuctionPrivateAggregationConfig {
+  USVString aggregationCoordinatorOrigin;
+  [EnforceRange] unsigned long long maxContributions;
+};
+
+dictionary ProtectedAudienceInterestGroupPrivateAggregationConfig {
   USVString aggregationCoordinatorOrigin;
 };

 partial dictionary AuctionAdConfig {
-  ProtectedAudiencePrivateAggregationConfig privateAggregationConfig;
+  ProtectedAudienceAuctionPrivateAggregationConfig privateAggregationConfig;
 };

 partial dictionary AuctionAdInterestGroup {
-  ProtectedAudiencePrivateAggregationConfig privateAggregationConfig;
+  ProtectedAudienceInterestGroupPrivateAggregationConfig privateAggregationConfig;
 };
 </xmp>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just uploaded a rough draft of the Protected Audience spec change in WICG/turtledove#1378.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here's the Shared Storage spec change: WICG/shared-storage#216.

spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
@dmcardle dmcardle force-pushed the dmcardle-per-call-report-sizing-spec branch from 1df696c to ff6bee4 Compare January 7, 2025 22:07
spec.bs Outdated
Comment on lines 1308 to 1309
Issue: Shouldn't the paragraph above refer to fields of [=pre-specified report
parameters=] instead of [=aggregatable report=]?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexmturner WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, switching over sgtm. (I think this was written before pre-specified report parameters existed.)

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated
report is sent, even if there are no contributions or there is insufficent
budget for the requested contributions. See [Protecting against leaks via
the number of reports](#protecting-against-leaks-via-the-number-of-reports).
Note: Even when budget is insufficient for the requested contributions, a report
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include the "no contributions requested" case in this note?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll give this another pass -- let me know if you preferred the original.

spec.bs Outdated
Comment on lines 1308 to 1309
Issue: Shouldn't the paragraph above refer to fields of [=pre-specified report
parameters=] instead of [=aggregatable report=]?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, switching over sgtm. (I think this was written before pre-specified report parameters existed.)

dmcardle added a commit to dmcardle/shared-storage that referenced this pull request Jan 9, 2025
The goal is to enable Shared Storage embedders to override the default
number of contributions per Private Aggregation report.

To that end, this change adds the `maxContributions` field to the
web-visible Private Aggregation config dictionary and plumbs its value
into Private Aggregation's "pre-specified report parameters".

Context:
* Explainer: patcg-individual-drafts/private-aggregation-api#146
* Spec change: patcg-individual-drafts/private-aggregation-api#164
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexmturner alexmturner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

@alexmturner
Copy link
Collaborator

Not sure what's going on with the validate spec check. I wonder if we need to update something? Will take another look.

@dmcardle
Copy link
Contributor Author

Great, I'll squash and push again, let's see if the validation still fails.

@dmcardle dmcardle force-pushed the dmcardle-per-call-report-sizing-spec branch from c96162a to 5aeb65c Compare January 15, 2025 17:17
@dmcardle
Copy link
Contributor Author

Still failing. Amusingly, the "Validate Web IDL" step from pythagoraskitty/[email protected] fails with a Chrome stacktrace...

  Failed to launch the browser process!
  [0115/172657.690711:FATAL:zygote_host_impl_linux.cc(117)] No usable sandbox! Update your kernel or see https://chromium.googlesource.com/chromium/src/+/main/docs/linux/suid_sandbox_development.md for more information on developing with the SUID sandbox. If you want to live dangerously and need an immediate workaround, you can try using --no-sandbox.
  #0 0x557a80362f49 base::debug::CollectStackTrace()
  #1 0x557a802cc933 base::debug::StackTrace::StackTrace()
  #2 0x557a802e0500 logging::LogMessage::~LogMessage()
  #3 0x557a7e4e213b content::ZygoteHostImpl::Init()
  #4 0x557a7fe7cb9f content::ContentMainRunnerImpl::Initialize()
  #5 0x557a7fe7afa5 content::RunContentProcess()
  #6 0x557a7fe7b0cd content::ContentMain()
  #7 0x557a7fed9fd9 headless::HeadlessBrowserMain()
  #8 0x557a7fed9cf8 headless::HeadlessShellMain()
  #9 0x557a7ccedae1 ChromeMain
  #10 0x7f905e62a1ca (/usr/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9)
  #11 0x7f905e62a28b __libc_start_main
  #12 0x557a7cced92a _start

@alexmturner
Copy link
Collaborator

Just landed a change to the yaml to use w3c/spec-prod@v2, so maybe one more rebase might fix it? (or at least cause a different error)

This change adds the web-visible `maxContributions` field, which enables
some callers to request different numbers of contributions per report.
@dmcardle dmcardle force-pushed the dmcardle-per-call-report-sizing-spec branch from 5aeb65c to 8472d3b Compare January 15, 2025 17:58
@alexmturner
Copy link
Collaborator

yay, it passed -- thanks!

@alexmturner alexmturner merged commit 3433a3e into patcg-individual-drafts:main Jan 15, 2025
1 check passed
github-actions bot added a commit that referenced this pull request Jan 15, 2025
SHA: 3433a3e
Reason: push, by alexmturner

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
dmcardle added a commit to dmcardle/shared-storage that referenced this pull request Jan 16, 2025
The goal is to enable Shared Storage embedders to override the default
number of contributions per Private Aggregation report.

To that end, this change adds the `maxContributions` field to the
web-visible Private Aggregation config dictionary and plumbs its value
into Private Aggregation's "pre-specified report parameters".

Context:
* Explainer: patcg-individual-drafts/private-aggregation-api#146
* Spec change: patcg-individual-drafts/private-aggregation-api#164
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants