Skip verbose debug report if no event trigger data is provided

WICG/attribution-reporting-api#682 Bug: 1409148 Change-Id: I2862e2bc01425499bf2e07692a2824f9f9fac9de Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189318 Commit-Queue: Nan Lin <linnan@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/main@{#1096985}
chromium · Jan 25, 2023 · d65fff6 · d65fff6
1 parent a8d68e8
commit d65fff6
Show file tree

Hide file tree

Showing 19 changed files with 73 additions and 36 deletions.
diff --git a/content/browser/attribution_reporting/attribution_debug_report.cc b/content/browser/attribution_reporting/attribution_debug_report.cc
@@ -86,6 +86,7 @@ absl::optional<DebugDataType> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<uint64_t> 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
@@ -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<attribution_reporting::AggregatableTriggerData>
       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
@@ -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
@@ -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/...ata/attribution_reporting/register_trigger_headers_debug_reporting.html.mock-http-headers b/...ata/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/...tests/wpt_internal/attribution-reporting/aggregatable-report-deduplication.sub.https.html b/...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/...wpt_internal/attribution-reporting/aggregatable-report-insufficient-budget.sub.https.html b/...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/...ts/wpt_internal/attribution-reporting/aggregatable-report-no-contributions.sub.https.html b/...ts/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/...k/web_tests/wpt_internal/attribution-reporting/simple-verbose-debug-report.sub.https.html b/...k/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/...ests/wpt_internal/attribution-reporting/top-level-filter-data-debug-report.sub.https.html b/...ests/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
@@ -19377,6 +19377,7 @@ Called by update_net_error_codes.py.-->
   <int value="13" label="Excessive reports"/>
   <int value="14" label="Falsely attributed source"/>
   <int value="15" label="Report window has passed"/>
+  <int value="16" label="Not registered"/>
 </enum>
 
 <enum name="ConversionStorageSourceStatus">

diff --git 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.
   </summary>
 </histogram>
 
-<histogram name="Conversions.CreateReportStatus6"
+<histogram name="Conversions.CreateReportStatus7"
     enum="ConversionStorageCreateReportStatus" expires_after="M117">
   <owner>apaseltiner@chromium.org</owner>
   <owner>johnidel@chromium.org</owner>