[conversions] Add conversions attributes to window features

What: Parse Attribution Reporting API attributes from the features string provided to window.open and register impressions. window.open support was removed previously due to conflicting with existing window.open usage. Parsing the attributes from features will avoid running into this issue, see the discussion here for rationale: WICG/attribution-reporting-api#130 This behavior is covered in the Attribution Reporting Explainer: https://github.com/WICG/conversion-measurement-api#registering-attribution-sources-for-windowopen-navigations Note that while the explainer names are updated, the blink implementation still uses structures which use the old naming scheme. This is being addressed separately. Bug: 1204575 Change-Id: I4768e36108dc9564f907a0857cedffa1af345ba8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2862265 Commit-Queue: John Delaney <johnidel@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Nate Chapin <japhet@chromium.org> Cr-Commit-Position: refs/heads/master@{#879011}
Mu-L · May 4, 2021 · 358e053 · 358e053
1 parent 4f780e7
commit 358e053
Show file tree

Hide file tree

Showing 10 changed files with 212 additions and 20 deletions.
diff --git a/content/browser/conversions/conversions_browsertest.cc b/content/browser/conversions/conversions_browsertest.cc
@@ -6,6 +6,7 @@
 
 #include "base/command_line.h"
 #include "base/sequenced_task_runner.h"
+#include "base/strings/strcat.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "content/browser/conversions/conversion_manager_impl.h"
@@ -161,7 +162,7 @@ IN_PROC_BROWSER_TEST_F(ConversionsBrowserTest,
 }
 
 IN_PROC_BROWSER_TEST_F(ConversionsBrowserTest,
-                       WindowOpenImpressionConversion_ReportSent) {
+                       WindowOpenDeprecatedAPI_NoException) {
   // Expected reports must be registered before the server starts.
   ExpectedReportWaiter expected_report(
       GURL(
@@ -202,6 +203,43 @@ IN_PROC_BROWSER_TEST_F(ConversionsBrowserTest,
   EXPECT_FALSE(expected_report.HasRequest());
 }
 
+IN_PROC_BROWSER_TEST_F(ConversionsBrowserTest,
+                       WindowOpenImpressionConversion_ReportSent) {
+  // Expected reports must be registered before the server starts.
+  ExpectedReportWaiter expected_report(
+      GURL(
+          "https://a.test/.well-known/"
+          "register-conversion?impression-data=1&conversion-data=7&credit=100"),
+      https_server());
+  ASSERT_TRUE(https_server()->Start());
+
+  GURL impression_url = https_server()->GetURL(
+      "a.test", "/conversions/page_with_impression_creator.html");
+  EXPECT_TRUE(NavigateToURL(web_contents(), impression_url));
+
+  GURL conversion_url = https_server()->GetURL(
+      "b.test", "/conversions/page_with_conversion_redirect.html");
+
+  // We can't use `JsReplace` directly to input the origin as it will use string
+  // literals which shouldn't be provided in the window features string.
+  std::string window_features =
+      base::StrCat({"attributionsourceeventid=1,attributiondestination=",
+                    url::Origin::Create(conversion_url).Serialize()});
+
+  TestNavigationObserver observer(web_contents());
+  EXPECT_TRUE(
+      ExecJs(web_contents(), JsReplace(R"(window.open($1, '_top', $2);)",
+                                       conversion_url, window_features)));
+  observer.Wait();
+
+  // Register a conversion with the original page as the reporting origin.
+  EXPECT_TRUE(
+      ExecJs(web_contents(), JsReplace("registerConversionForOrigin(7, $1)",
+                                       url::Origin::Create(impression_url))));
+
+  EXPECT_EQ(expected_report.expected_url, expected_report.WaitForRequestUrl());
+}
+
 IN_PROC_BROWSER_TEST_F(ConversionsBrowserTest,
                        ImpressionFromCrossOriginSubframe_ReportSent) {
   ExpectedReportWaiter expected_report(

diff --git a/content/browser/conversions/impression_declaration_browsertest.cc b/content/browser/conversions/impression_declaration_browsertest.cc
@@ -807,4 +807,83 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_EQ(0u, host->num_impressions());
 }
 
+IN_PROC_BROWSER_TEST_F(ImpressionDeclarationBrowserTest,
+                       WindowOpenImpression_ImpressionReceived) {
+  ImpressionObserver impression_observer(web_contents());
+  GURL page_url =
+      https_server()->GetURL("b.test", "/page_with_impression_creator.html");
+  EXPECT_TRUE(NavigateToURL(web_contents(), page_url));
+
+  // Navigate the page using window.open and set an impression.
+  EXPECT_TRUE(ExecJs(web_contents(), R"(
+    window.open("https://a.com", "_top",
+    "attributionsourceeventid=1,attributiondestination=https://a.com,\
+    attributionreportto=https://report.com,attributionexpiry=1000");)"));
+
+  // Wait for the impression to be seen by the observer.
+  blink::Impression last_impression = impression_observer.Wait();
+
+  // Verify the attributes of the impression are set as expected.
+  EXPECT_EQ(1UL, last_impression.impression_data);
+  EXPECT_EQ(url::Origin::Create(GURL("https://a.com")),
+            last_impression.conversion_destination);
+  EXPECT_EQ(url::Origin::Create(GURL("https://report.com")),
+            last_impression.reporting_origin);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(1000), *last_impression.expiry);
+}
+
+IN_PROC_BROWSER_TEST_F(ImpressionDeclarationBrowserTest,
+                       WindowOpenAttributionSourceFeatures_FeaturesHandled) {
+  struct {
+    std::string features;
+    bool expected;
+  } kTestCases[] = {
+      {"", false},
+      {"attributionsourceeventid=1", false},
+      {"attributiondestination=1", false},
+      {"attributionexpiry=1", false},
+      {"attributionsourceeventid=1,attributiondestination=1234", false},
+      {"attributionsourceeventid=1,attributiondestination=abcdefg", false},
+      {"attributionsourceeventid=1,attributiondestination=http://a.com", false},
+      {"attributionsourceeventid=1,attributiondestination=https://a.com", true},
+      {"attributionsourceeventid=bb,attributiondestination=https://a.com",
+       true},
+  };
+
+  for (const auto& test_case : kTestCases) {
+    ImpressionObserver impression_observer(web_contents());
+    GURL page_url =
+        https_server()->GetURL("b.test", "/page_with_impression_creator.html");
+    EXPECT_TRUE(NavigateToURL(web_contents(), page_url));
+
+    // Navigate the page using window.open and set an impression.
+    EXPECT_TRUE(ExecJs(web_contents(),
+                       JsReplace(R"(window.open("https://a.com", "_top", $1);)",
+                                 test_case.features)));
+
+    // Wait for the impression to be seen by the observer.
+    if (test_case.expected)
+      impression_observer.Wait();
+    else
+      EXPECT_TRUE(impression_observer.WaitForNavigationWithNoImpression());
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(ImpressionDeclarationBrowserTest,
+                       WindowOpenNoUserGesture_NoImpression) {
+  ImpressionObserver impression_observer(web_contents());
+  GURL page_url =
+      https_server()->GetURL("b.test", "/page_with_impression_creator.html");
+  EXPECT_TRUE(NavigateToURL(web_contents(), page_url));
+
+  // Navigate the page using window.open and set an impression, but do not give
+  // a user gesture.
+  EXPECT_TRUE(ExecJs(web_contents(), R"(
+    window.open("https://a.com", "_top",
+    "attributionsourceeventid=1,attributiondestination=https://a.com");)",
+                     EXECUTE_SCRIPT_NO_USER_GESTURE));
+
+  EXPECT_TRUE(impression_observer.WaitForNavigationWithNoImpression());
+}
+
 }  // namespace content
diff --git a/third_party/blink/public/web/web_window_features.h b/third_party/blink/public/web/web_window_features.h
@@ -31,6 +31,10 @@
 #ifndef THIRD_PARTY_BLINK_PUBLIC_WEB_WEB_WINDOW_FEATURES_H_
 #define THIRD_PARTY_BLINK_PUBLIC_WEB_WEB_WINDOW_FEATURES_H_
 
+#include "base/optional.h"
+
+#include "third_party/blink/public/platform/web_impression.h"
+
 namespace blink {
 
 struct WebWindowFeatures {
@@ -60,6 +64,10 @@ struct WebWindowFeatures {
   bool noreferrer = false;
   bool background = false;
   bool persistent = false;
+
+  // Represents the attribution source declared by Attribution Reporting related
+  // window features, if any.
+  base::Optional<WebImpression> impression;
 };
 
 }  // namespace blink

diff --git a/third_party/blink/renderer/core/frame/local_dom_window.cc b/third_party/blink/renderer/core/frame/local_dom_window.cc
@@ -1979,7 +1979,8 @@ DOMWindow* LocalDOMWindow::open(v8::Isolate* isolate,
     return nullptr;
   }
 
-  WebWindowFeatures window_features = GetWindowFeaturesFromString(features);
+  WebWindowFeatures window_features =
+      GetWindowFeaturesFromString(features, incumbent_window);
 
   FrameLoadRequest frame_request(incumbent_window,
                                  ResourceRequest(completed_url));
@@ -2003,6 +2004,10 @@ DOMWindow* LocalDOMWindow::open(v8::Isolate* isolate,
   frame_request.GetResourceRequest().SetHasUserGesture(has_user_gesture);
   GetFrame()->MaybeLogAdClickNavigation();
 
+  if (has_user_gesture && window_features.impression) {
+    frame_request.SetImpression(*window_features.impression);
+  }
+
   FrameTree::FindResult result =
       GetFrame()->Tree().FindOrCreateFrameForNavigation(
           frame_request, target.IsEmpty() ? "_blank" : target);

diff --git a/third_party/blink/renderer/core/html/conversion_measurement_parsing.cc b/third_party/blink/renderer/core/html/conversion_measurement_parsing.cc
@@ -25,6 +25,12 @@ namespace blink {
 
 namespace {
 
+base::Optional<uint64_t> ParseExpiry(const String& expiry) {
+  bool expiry_is_valid = false;
+  uint64_t parsed_expiry = expiry.ToUInt64Strict(&expiry_is_valid);
+  return expiry_is_valid ? base::make_optional(parsed_expiry) : base::nullopt;
+}
+
 base::Optional<WebImpression> GetImpression(
     ExecutionContext* execution_context,
     const String& impression_data_string,
@@ -149,13 +155,9 @@ base::Optional<WebImpression> GetImpressionForAnchor(
     HTMLAnchorElement* element) {
   base::Optional<uint64_t> expiry;
   if (element->hasAttribute(html_names::kImpressionexpiryAttr)) {
-    bool expiry_is_valid = false;
-    uint64_t expiry_milliseconds =
-        element->FastGetAttribute(html_names::kImpressionexpiryAttr)
-            .GetString()
-            .ToUInt64Strict(&expiry_is_valid);
-    if (expiry_is_valid)
-      expiry = expiry_milliseconds;
+    expiry =
+        ParseExpiry(element->FastGetAttribute(html_names::kImpressionexpiryAttr)
+                        .GetString());
   }
 
   DCHECK(element->hasAttribute(html_names::kConversiondestinationAttr));
@@ -174,6 +176,23 @@ base::Optional<WebImpression> GetImpressionForAnchor(
       expiry, element);
 }
 
+base::Optional<WebImpression> GetImpressionFromWindowFeatures(
+    ExecutionContext* execution_context,
+    const ImpressionFeatures& features) {
+  if (features.impression_data.IsNull() ||
+      features.conversion_destination.IsNull())
+    return base::nullopt;
+
+  return GetImpression(
+      execution_context, features.impression_data,
+      features.conversion_destination,
+      !features.reporting_origin.IsNull()
+          ? base::make_optional(features.reporting_origin)
+          : base::nullopt,
+      !features.expiry.IsNull() ? ParseExpiry(features.expiry) : base::nullopt,
+      nullptr);
+}
+
 base::Optional<WebImpression> GetImpressionForParams(
     ExecutionContext* execution_context,
     const ImpressionParams* params) {

diff --git a/third_party/blink/renderer/core/html/conversion_measurement_parsing.h b/third_party/blink/renderer/core/html/conversion_measurement_parsing.h
@@ -18,12 +18,27 @@ class ExecutionContext;
 class HTMLAnchorElement;
 class ImpressionParams;
 
+// Dummy struct to pass un-parsed Attribution Reporting window features into the
+// parsing utilities below.
+struct ImpressionFeatures {
+  String impression_data;
+  String conversion_destination;
+  String reporting_origin;
+  String expiry;
+};
+
 // Returns the WebImpression struct with all data declared by impression
 // related attributes on |element|. If the impression attributes do not contain
 // allowed values, base::nullopt is returned.
 base::Optional<WebImpression> GetImpressionForAnchor(
     HTMLAnchorElement* element);
 
+// Same as GetImpressionForAnchor(), but gets an impression specified by the
+// features string associated with a window.open call.
+base::Optional<WebImpression> GetImpressionFromWindowFeatures(
+    ExecutionContext* execution_context,
+    const ImpressionFeatures& features);
+
 // Same as GetImpressionForAnchor(), but gets an impression specified by an
 // ImpressionParams dictionary associated with a window.open call.
 base::Optional<WebImpression> GetImpressionForParams(

diff --git a/third_party/blink/renderer/core/loader/navigation_policy_test.cc b/third_party/blink/renderer/core/loader/navigation_policy_test.cc
@@ -235,8 +235,8 @@ TEST_F(NavigationPolicyTest, NoOpener) {
 
   for (const auto& test : kCases) {
     EXPECT_EQ(test.policy,
-              NavigationPolicyForCreateWindow(
-                  GetWindowFeaturesFromString(test.feature_string)))
+              NavigationPolicyForCreateWindow(GetWindowFeaturesFromString(
+                  test.feature_string, nullptr /* dom_window */)))
         << "Testing '" << test.feature_string << "'";
   }
 }
@@ -258,8 +258,8 @@ TEST_F(NavigationPolicyTest, NoOpenerAndNoReferrer) {
 
   for (const auto& test : kCases) {
     EXPECT_EQ(test.policy,
-              NavigationPolicyForCreateWindow(
-                  GetWindowFeaturesFromString(test.feature_string)))
+              NavigationPolicyForCreateWindow(GetWindowFeaturesFromString(
+                  test.feature_string, nullptr /* dom_window */)))
         << "Testing '" << test.feature_string << "'";
   }
 }
@@ -281,8 +281,8 @@ TEST_F(NavigationPolicyTest, NoReferrer) {
 
   for (const auto& test : kCases) {
     EXPECT_EQ(test.policy,
-              NavigationPolicyForCreateWindow(
-                  GetWindowFeaturesFromString(test.feature_string)))
+              NavigationPolicyForCreateWindow(GetWindowFeaturesFromString(
+                  test.feature_string, nullptr /* dom_window */)))
         << "Testing '" << test.feature_string << "'";
   }
 }

diff --git a/third_party/blink/renderer/core/page/create_window.cc b/third_party/blink/renderer/core/page/create_window.cc
@@ -44,6 +44,7 @@
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/frame/local_frame_client.h"
+#include "third_party/blink/renderer/core/html/conversion_measurement_parsing.h"
 #include "third_party/blink/renderer/core/inspector/console_message.h"
 #include "third_party/blink/renderer/core/loader/frame_load_request.h"
 #include "third_party/blink/renderer/core/page/chrome_client.h"
@@ -65,9 +66,15 @@ static bool IsWindowFeaturesSeparator(UChar c) {
          c == ',' || c == '\f';
 }
 
-WebWindowFeatures GetWindowFeaturesFromString(const String& feature_string) {
+WebWindowFeatures GetWindowFeaturesFromString(const String& feature_string,
+                                              LocalDOMWindow* dom_window) {
   WebWindowFeatures window_features;
 
+  ImpressionFeatures impression_features;
+  bool conversion_measurement_enabled =
+      dom_window &&
+      RuntimeEnabledFeatures::ConversionMeasurementEnabled(dom_window);
+
   // This code follows the HTML spec, specifically
   // https://html.spec.whatwg.org/C/#concept-window-open-features-tokenize
   if (feature_string.IsEmpty())
@@ -189,12 +196,27 @@ WebWindowFeatures GetWindowFeaturesFromString(const String& feature_string) {
       window_features.background = true;
     } else if (key_string == "persistent") {
       window_features.persistent = true;
+    } else if (conversion_measurement_enabled) {
+      if (key_string == "attributionsourceeventid") {
+        impression_features.impression_data = value_string.ToString();
+      } else if (key_string == "attributiondestination") {
+        impression_features.conversion_destination = value_string.ToString();
+      } else if (key_string == "attributionreportto") {
+        impression_features.reporting_origin = value_string.ToString();
+      } else if (key_string == "attributionexpiry") {
+        impression_features.expiry = value_string.ToString();
+      }
     }
   }
 
   if (window_features.noreferrer)
     window_features.noopener = true;
 
+  if (conversion_measurement_enabled) {
+    window_features.impression =
+        GetImpressionFromWindowFeatures(dom_window, impression_features);
+  }
+
   return window_features;
 }
 

diff --git a/third_party/blink/renderer/core/page/create_window.h b/third_party/blink/renderer/core/page/create_window.h
@@ -33,14 +33,17 @@
 
 namespace blink {
 class Frame;
+class LocalDOMWindow;
 class LocalFrame;
 struct FrameLoadRequest;
 
 Frame* CreateNewWindow(LocalFrame& opener_frame,
                        FrameLoadRequest&,
                        const AtomicString& name);
 
-CORE_EXPORT WebWindowFeatures GetWindowFeaturesFromString(const String&);
+CORE_EXPORT WebWindowFeatures
+GetWindowFeaturesFromString(const String& feature_string,
+                            LocalDOMWindow* dom_window);
 
 }  // namespace blink
 

diff --git a/third_party/blink/renderer/core/page/window_features_test.cc b/third_party/blink/renderer/core/page/window_features_test.cc
@@ -28,8 +28,9 @@ TEST_F(WindowFeaturesTest, NoOpener) {
   };
 
   for (const auto& test : kCases) {
-    EXPECT_EQ(test.noopener,
-              GetWindowFeaturesFromString(test.feature_string).noopener)
+    EXPECT_EQ(test.noopener, GetWindowFeaturesFromString(
+                                 test.feature_string, nullptr /* dom_window */)
+                                 .noopener)
         << "Testing '" << test.feature_string << "'";
   }
 }
@@ -59,7 +60,9 @@ TEST_F(WindowFeaturesTest, NoReferrer) {
 
   for (const auto& test : kCases) {
     EXPECT_EQ(test.noreferrer,
-              GetWindowFeaturesFromString(test.feature_string).noreferrer)
+              GetWindowFeaturesFromString(test.feature_string,
+                                          nullptr /* dom_window */)
+                  .noreferrer)
         << "Testing '" << test.feature_string << "'";
   }
 }