From d65fff6c2478ad67f81c09f15e710f73acb24170 Mon Sep 17 00:00:00 2001 From: Nan Lin Date: Wed, 25 Jan 2023 21:09:37 +0000 Subject: [PATCH] Skip verbose debug report if no event trigger data is provided https://github.com/WICG/attribution-reporting-api/pull/682 Bug: 1409148 Change-Id: I2862e2bc01425499bf2e07692a2824f9f9fac9de Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189318 Commit-Queue: Nan Lin Reviewed-by: Robert Sesek Reviewed-by: Andrew Paseltiner Reviewed-by: Charlie Harrison Cr-Commit-Position: refs/heads/main@{#1096985} --- .../attribution_debug_report.cc | 1 + .../attribution_debug_report_unittest.cc | 7 +++ .../attribution_internals.mojom | 2 +- .../attribution_internals_handler_impl.cc | 2 + .../attribution_manager_impl.cc | 6 +-- .../attribution_manager_impl_unittest.cc | 2 +- .../attribution_observer_types.cc | 3 +- .../attribution_storage_sql.cc | 22 +++++++--- .../attribution_storage_unittest.cc | 43 +++++++++++++------ .../attribution_test_utils.cc | 2 + .../attribution_trigger.h | 3 +- ...ers_debug_reporting.html.mock-http-headers | 2 +- ...atable-report-deduplication.sub.https.html | 3 -- ...-report-insufficient-budget.sub.https.html | 3 -- ...ble-report-no-contributions.sub.https.html | 3 -- ...simple-verbose-debug-report.sub.https.html | 1 + ...el-filter-data-debug-report.sub.https.html | 1 + tools/metrics/histograms/enums.xml | 1 + .../histograms/metadata/others/histograms.xml | 2 +- 19 files changed, 73 insertions(+), 36 deletions(-) diff --git a/content/browser/attribution_reporting/attribution_debug_report.cc b/content/browser/attribution_reporting/attribution_debug_report.cc index b2a788d02bf3d..4069fe3be5353 100644 --- a/content/browser/attribution_reporting/attribution_debug_report.cc +++ b/content/browser/attribution_reporting/attribution_debug_report.cc @@ -86,6 +86,7 @@ absl::optional GetReportDataType(EventLevelResult result, case EventLevelResult::kSuccess: case EventLevelResult::kProhibitedByBrowserPolicy: case EventLevelResult::kSuccessDroppedLowerPriority: + case EventLevelResult::kNotRegistered: return absl::nullopt; case EventLevelResult::kInternalError: return DataTypeIfCookieSet(DebugDataType::kTriggerUnknownError, diff --git a/content/browser/attribution_reporting/attribution_debug_report_unittest.cc b/content/browser/attribution_reporting/attribution_debug_report_unittest.cc index b18c0e7b6e984..4d2b82d5247e7 100644 --- a/content/browser/attribution_reporting/attribution_debug_report_unittest.cc +++ b/content/browser/attribution_reporting/attribution_debug_report_unittest.cc @@ -600,6 +600,13 @@ TEST(AttributionDebugReportTest, EventLevelAttributionDebugging) { }, "type": "trigger-event-report-window-passed" }])json"}, + {EventLevelResult::kNotRegistered, + /*replaced_event_level_report=*/absl::nullopt, + /*new_event_level_report=*/absl::nullopt, + /*source=*/absl::nullopt, CreateReportResult::Limits(), + /*dropped_event_level_report=*/absl::nullopt, + /*trigger_debug_key=*/absl::nullopt, + /*expected_report_body=*/nullptr}, }; for (bool is_debug_cookie_set : {false, true}) { diff --git a/content/browser/attribution_reporting/attribution_internals.mojom b/content/browser/attribution_reporting/attribution_internals.mojom index fe70567c395c5..2a6d94e7cdd87 100644 --- a/content/browser/attribution_reporting/attribution_internals.mojom +++ b/content/browser/attribution_reporting/attribution_internals.mojom @@ -170,6 +170,7 @@ struct WebUITrigger { kProhibitedByBrowserPolicy, kDeduplicated, kReportWindowPassed, + kNotRegistered, // Event-level statuses: kLowPriority, @@ -180,7 +181,6 @@ struct WebUITrigger { // Aggregatable statuses: kNoHistograms, kInsufficientBudget, - kNotRegistered, }; Status event_level_status; diff --git a/content/browser/attribution_reporting/attribution_internals_handler_impl.cc b/content/browser/attribution_reporting/attribution_internals_handler_impl.cc index 002d447afc247..3f7ac7a509b22 100644 --- a/content/browser/attribution_reporting/attribution_internals_handler_impl.cc +++ b/content/browser/attribution_reporting/attribution_internals_handler_impl.cc @@ -467,6 +467,8 @@ WebUITriggerStatus GetWebUITriggerStatus(EventLevelStatus status) { return WebUITriggerStatus::kExcessiveEventLevelReports; case EventLevelStatus::kReportWindowPassed: return WebUITriggerStatus::kReportWindowPassed; + case EventLevelStatus::kNotRegistered: + return WebUITriggerStatus::kNotRegistered; } } diff --git a/content/browser/attribution_reporting/attribution_manager_impl.cc b/content/browser/attribution_reporting/attribution_manager_impl.cc index 19cd9e0fc3529..bd9a52692a47e 100644 --- a/content/browser/attribution_reporting/attribution_manager_impl.cc +++ b/content/browser/attribution_reporting/attribution_manager_impl.cc @@ -169,9 +169,9 @@ void RecordStoreSourceStatus(AttributionStorage::StoreSourceResult result) { void RecordCreateReportStatus(CreateReportResult result) { static_assert(AttributionTrigger::EventLevelResult::kMaxValue == - AttributionTrigger::EventLevelResult::kReportWindowPassed, - "Bump version of Conversions.CreateReportStatus6 histogram."); - base::UmaHistogramEnumeration("Conversions.CreateReportStatus6", + AttributionTrigger::EventLevelResult::kNotRegistered, + "Bump version of Conversions.CreateReportStatus7 histogram."); + base::UmaHistogramEnumeration("Conversions.CreateReportStatus7", result.event_level_status()); static_assert( AttributionTrigger::AggregatableResult::kMaxValue == diff --git a/content/browser/attribution_reporting/attribution_manager_impl_unittest.cc b/content/browser/attribution_reporting/attribution_manager_impl_unittest.cc index db6c0e0f6c858..a2695e992bb80 100644 --- a/content/browser/attribution_reporting/attribution_manager_impl_unittest.cc +++ b/content/browser/attribution_reporting/attribution_manager_impl_unittest.cc @@ -1171,7 +1171,7 @@ TEST_F(AttributionManagerImplTest, HandleTrigger_RecordsMetric) { attribution_manager_->HandleTrigger(DefaultTrigger()); EXPECT_THAT(StoredReports(), IsEmpty()); histograms.ExpectUniqueSample( - "Conversions.CreateReportStatus6", + "Conversions.CreateReportStatus7", AttributionTrigger::EventLevelResult::kNoMatchingImpressions, 1); histograms.ExpectUniqueSample( "Conversions.AggregatableReport.CreateReportStatus3", diff --git a/content/browser/attribution_reporting/attribution_observer_types.cc b/content/browser/attribution_reporting/attribution_observer_types.cc index dec39912bf029..d56086687c1e2 100644 --- a/content/browser/attribution_reporting/attribution_observer_types.cc +++ b/content/browser/attribution_reporting/attribution_observer_types.cc @@ -58,7 +58,8 @@ CreateReportResult::CreateReportResult( replaced_event_level_report_.has_value(), event_level_status_ == EventLevelResult::kSuccessDroppedLowerPriority); - if (event_level_status_ != EventLevelResult::kInternalError) { + if (event_level_status_ != EventLevelResult::kInternalError && + event_level_status_ != EventLevelResult::kNotRegistered) { DCHECK_EQ(source_.has_value(), event_level_status_ != EventLevelResult::kNoMatchingImpressions && event_level_status_ != diff --git a/content/browser/attribution_reporting/attribution_storage_sql.cc b/content/browser/attribution_reporting/attribution_storage_sql.cc index 8b1267a7e0528..1617c8ecaa288 100644 --- a/content/browser/attribution_reporting/attribution_storage_sql.cc +++ b/content/browser/attribution_reporting/attribution_storage_sql.cc @@ -822,11 +822,20 @@ CreateReportResult AttributionStorageSql::MaybeCreateAndStoreReport( const attribution_reporting::TriggerRegistration& trigger_registration = trigger.registration(); + if (trigger_registration.event_triggers.vec().empty()) { + event_level_status = EventLevelResult::kNotRegistered; + } + if (trigger_registration.aggregatable_trigger_data.vec().empty() && trigger_registration.aggregatable_values.values().empty()) { aggregatable_status = AggregatableResult::kNotRegistered; } + if (event_level_status.has_value() && aggregatable_status.has_value()) { + return assemble_report_result(/*new_event_level_status=*/absl::nullopt, + /*new_aggregaable_status=*/absl::nullopt); + } + // We don't bother creating the DB here if it doesn't exist, because it's not // possible for there to be a matching source if there's no DB. if (!LazyInit(DbCreationPolicy::kIgnoreIfAbsent)) { @@ -871,11 +880,14 @@ CreateReportResult AttributionStorageSql::MaybeCreateAndStoreReport( } absl::optional dedup_key; - if (EventLevelResult create_event_level_status = MaybeCreateEventLevelReport( - *attribution_info, trigger, new_event_level_report, dedup_key, - limits.max_event_level_reports_per_destination); - create_event_level_status != EventLevelResult::kSuccess) { - event_level_status = create_event_level_status; + if (!event_level_status.has_value()) { + if (EventLevelResult create_event_level_status = + MaybeCreateEventLevelReport( + *attribution_info, trigger, new_event_level_report, dedup_key, + limits.max_event_level_reports_per_destination); + create_event_level_status != EventLevelResult::kSuccess) { + event_level_status = create_event_level_status; + } } if (!aggregatable_status.has_value()) { diff --git a/content/browser/attribution_reporting/attribution_storage_unittest.cc b/content/browser/attribution_reporting/attribution_storage_unittest.cc index d92d3103102cc..a68025801f491 100644 --- a/content/browser/attribution_reporting/attribution_storage_unittest.cc +++ b/content/browser/attribution_reporting/attribution_storage_unittest.cc @@ -2814,6 +2814,14 @@ TEST_F(AttributionStorageTest, MatchingTriggerData_UsesCorrectData) { TEST_F(AttributionStorageTest, TopLevelTriggerFiltering) { const auto origin = *SuitableOrigin::Deserialize("https://r.test"); + auto event_triggers = *attribution_reporting::EventTriggerDataList::Create( + {attribution_reporting::EventTriggerData( + /*data=*/11, + /*priority=*/12, + /*dedup_key=*/13, + /*filters=*/AttributionFilters(), + /*not_filters=*/AttributionFilters())}); + std::vector aggregatable_trigger_data{ *attribution_reporting::AggregatableTriggerData::Create( @@ -2843,8 +2851,7 @@ TEST_F(AttributionStorageTest, TopLevelTriggerFiltering) { }), /*not_filters=*/AttributionFilters(), /*debug_key=*/absl::nullopt, - /*aggregatable_dedup_key=*/absl::nullopt, - /*event_triggers=*/attribution_reporting::EventTriggerDataList(), + /*aggregatable_dedup_key=*/absl::nullopt, event_triggers, *attribution_reporting::AggregatableTriggerDataList::Create( aggregatable_trigger_data), aggregatable_values, @@ -2862,8 +2869,7 @@ TEST_F(AttributionStorageTest, TopLevelTriggerFiltering) { }), /*not_filters=*/AttributionFilters(), /*debug_key=*/absl::nullopt, - /*aggregatable_dedup_key=*/absl::nullopt, - /*event_triggers=*/attribution_reporting::EventTriggerDataList(), + /*aggregatable_dedup_key=*/absl::nullopt, event_triggers, *attribution_reporting::AggregatableTriggerDataList::Create( aggregatable_trigger_data), aggregatable_values, @@ -2879,8 +2885,7 @@ TEST_F(AttributionStorageTest, TopLevelTriggerFiltering) { /*not_filters=*/ AttributionFiltersForSourceType(AttributionSourceType::kNavigation), /*debug_key=*/absl::nullopt, - /*aggregatable_dedup_key=*/absl::nullopt, - /*event_triggers=*/attribution_reporting::EventTriggerDataList(), + /*aggregatable_dedup_key=*/absl::nullopt, event_triggers, *attribution_reporting::AggregatableTriggerDataList::Create( aggregatable_trigger_data), aggregatable_values, @@ -2898,13 +2903,11 @@ TEST_F(AttributionStorageTest, TopLevelTriggerFiltering) { AttributionTrigger::AggregatableResult:: kNoMatchingSourceFilterData))); - EXPECT_THAT( - storage()->MaybeCreateAndStoreReport(trigger2), - AllOf( - CreateReportEventLevelStatusIs( - AttributionTrigger::EventLevelResult::kNoMatchingConfigurations), - CreateReportAggregatableStatusIs( - AttributionTrigger::AggregatableResult::kSuccess))); + EXPECT_THAT(storage()->MaybeCreateAndStoreReport(trigger2), + AllOf(CreateReportEventLevelStatusIs( + AttributionTrigger::EventLevelResult::kSuccess), + CreateReportAggregatableStatusIs( + AttributionTrigger::AggregatableResult::kSuccess))); EXPECT_THAT(storage()->MaybeCreateAndStoreReport(trigger3), AllOf(CreateReportEventLevelStatusIs( @@ -3156,4 +3159,18 @@ TEST_F(AttributionStorageTest, MaxAttributions_BoundedBySourceTimeWindow) { MaybeCreateAndStoreEventLevelReport(trigger)); } +TEST_F(AttributionStorageTest, NoEventTriggerData_NotRegisteredReturned) { + EXPECT_THAT( + storage()->MaybeCreateAndStoreReport( + DefaultAggregatableTriggerBuilder().Build( + /*generate_event_trigger_data=*/false)), + AllOf(CreateReportEventLevelStatusIs( + AttributionTrigger::EventLevelResult::kNotRegistered), + CreateReportAggregatableStatusIs( + AttributionTrigger::AggregatableResult::kNoMatchingImpressions), + NewEventLevelReportIs(absl::nullopt), + NewAggregatableReportIs(absl::nullopt))); + EXPECT_THAT(storage()->GetAttributionReports(base::Time::Now()), IsEmpty()); +} + } // namespace content diff --git a/content/browser/attribution_reporting/attribution_test_utils.cc b/content/browser/attribution_reporting/attribution_test_utils.cc index 012107dee6ce7..cc25d37a36bea 100644 --- a/content/browser/attribution_reporting/attribution_test_utils.cc +++ b/content/browser/attribution_reporting/attribution_test_utils.cc @@ -1022,6 +1022,8 @@ std::ostream& operator<<(std::ostream& out, return out << "falselyAttributedSource"; case AttributionTrigger::EventLevelResult::kReportWindowPassed: return out << "reportWindowPassed"; + case AttributionTrigger::EventLevelResult::kNotRegistered: + return out << "notRegistered"; } } diff --git a/content/browser/attribution_reporting/attribution_trigger.h b/content/browser/attribution_reporting/attribution_trigger.h index 17f5fa04abeef..11d9fb75658d8 100644 --- a/content/browser/attribution_reporting/attribution_trigger.h +++ b/content/browser/attribution_reporting/attribution_trigger.h @@ -41,7 +41,8 @@ class CONTENT_EXPORT AttributionTrigger { kExcessiveReports = 13, kFalselyAttributedSource = 14, kReportWindowPassed = 15, - kMaxValue = kReportWindowPassed, + kNotRegistered = 16, + kMaxValue = kNotRegistered, }; // Represents the potential aggregatable outcomes from attempting to register diff --git a/content/test/data/attribution_reporting/register_trigger_headers_debug_reporting.html.mock-http-headers b/content/test/data/attribution_reporting/register_trigger_headers_debug_reporting.html.mock-http-headers index e7919114b1db1..d901c1baa963f 100644 --- a/content/test/data/attribution_reporting/register_trigger_headers_debug_reporting.html.mock-http-headers +++ b/content/test/data/attribution_reporting/register_trigger_headers_debug_reporting.html.mock-http-headers @@ -1,2 +1,2 @@ HTTP/1.1 200 OK -Attribution-Reporting-Register-Trigger:{"debug_reporting": true} +Attribution-Reporting-Register-Trigger:{"event_trigger_data":[{"trigger_data":"7"}], "debug_reporting":true} diff --git a/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-deduplication.sub.https.html b/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-deduplication.sub.https.html index 1b99d10c70f02..75b371d47b6a7 100644 --- a/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-deduplication.sub.https.html +++ b/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-deduplication.sub.https.html @@ -25,9 +25,6 @@ }); const trigger = { - // Create event-level report to avoid verbose debug report created for - // event-level attribution. - event_trigger_data: [{}], aggregatable_trigger_data: [{ key_piece: '0x400', source_keys: ['campaignCounts'], diff --git a/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-insufficient-budget.sub.https.html b/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-insufficient-budget.sub.https.html index a81095b373ee4..de162832b8cbc 100644 --- a/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-insufficient-budget.sub.https.html +++ b/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-insufficient-budget.sub.https.html @@ -23,9 +23,6 @@ }); const trigger = { - // Create event-level report to avoid verbose debug report created for - // event-level attribution. - event_trigger_data: [{}], aggregatable_trigger_data: [{ key_piece: '0x400', source_keys: ['campaignCounts'], diff --git a/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-no-contributions.sub.https.html b/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-no-contributions.sub.https.html index 285f43ac1dbdb..06f800c8ff1db 100644 --- a/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-no-contributions.sub.https.html +++ b/third_party/blink/web_tests/wpt_internal/attribution-reporting/aggregatable-report-no-contributions.sub.https.html @@ -23,9 +23,6 @@ await registerAttributionSrc(t, { trigger : { - // Create event-level report to avoid verbose debug report created for - // event-level attribution. - event_trigger_data: [{}], aggregatable_values: { geoValue: 32768, }, diff --git a/third_party/blink/web_tests/wpt_internal/attribution-reporting/simple-verbose-debug-report.sub.https.html b/third_party/blink/web_tests/wpt_internal/attribution-reporting/simple-verbose-debug-report.sub.https.html index 4fd1a31935655..512e9d46dacc5 100644 --- a/third_party/blink/web_tests/wpt_internal/attribution-reporting/simple-verbose-debug-report.sub.https.html +++ b/third_party/blink/web_tests/wpt_internal/attribution-reporting/simple-verbose-debug-report.sub.https.html @@ -16,6 +16,7 @@ await registerAttributionSrc(t, { trigger: { + event_trigger_data: [{}], debug_reporting: true, debug_key: '456', }, diff --git a/third_party/blink/web_tests/wpt_internal/attribution-reporting/top-level-filter-data-debug-report.sub.https.html b/third_party/blink/web_tests/wpt_internal/attribution-reporting/top-level-filter-data-debug-report.sub.https.html index e9af7d9e3da61..020d08dd3bb83 100644 --- a/third_party/blink/web_tests/wpt_internal/attribution-reporting/top-level-filter-data-debug-report.sub.https.html +++ b/third_party/blink/web_tests/wpt_internal/attribution-reporting/top-level-filter-data-debug-report.sub.https.html @@ -18,6 +18,7 @@ await registerAttributionSrc(t, { trigger: { + event_trigger_data: [{}], filters: {a: ['c']}, debug_reporting: true, }, diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index 59f07a13cc9d6..10e049c614812 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml @@ -19377,6 +19377,7 @@ Called by update_net_error_codes.py.--> + diff --git a/tools/metrics/histograms/metadata/others/histograms.xml b/tools/metrics/histograms/metadata/others/histograms.xml index eedeb07c4ea78..747a06192f3a0 100644 --- a/tools/metrics/histograms/metadata/others/histograms.xml +++ b/tools/metrics/histograms/metadata/others/histograms.xml @@ -3623,7 +3623,7 @@ chromium-metrics-reviews@google.com. - apaseltiner@chromium.org johnidel@chromium.org