Add prioritization of ad click conversions and cleaning of sent ad click conversions
authorwilander@apple.com <wilander@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Apr 2019 21:47:04 +0000 (21:47 +0000)
committerwilander@apple.com <wilander@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Apr 2019 21:47:04 +0000 (21:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196934
<rdar://problem/49917773>

Reviewed by Chris Dumez.

Source/WebCore:

Tests: http/tests/adClickAttribution/second-attribution-converted-with-higher-priority.html
       http/tests/adClickAttribution/second-attribution-converted-with-lower-priority.html
       http/tests/adClickAttribution/second-conversion-with-higher-priority.html
       http/tests/adClickAttribution/second-conversion-with-lower-priority.html

* loader/AdClickAttribution.cpp:
(WebCore::AdClickAttribution::hasHigherPriorityThan const):
    Added to facilitate priority comparison between two attributions.
* loader/AdClickAttribution.h:
(WebCore::AdClickAttribution::Destination::Destination):
    Added a WTF::HashTableDeletedValueType constructor and changed the copy constructor to
    a move constructor.
(WebCore::AdClickAttribution::isEmpty const):

Source/WebKit:

In this description, by "pair" I mean { AdClickAttribution::Source, AdClickAttribution::Destination }.

This patch adds handling of prioritization of conversions according to these rules:
- If we have a matching unconverted attribution, convert it. This consumes the conversion.
- If we have no previously converted attribution for this pair, just store.
- If we have a previously converted attribution for this pair, replace it if the new one has higher priority.
- If we had no matching unconverted attribution but do have a previously converted attribution for this
pair, re-convert the previously converted attribution to make sure the highest priority gets set.

This handling is in part done by dividing the previous m_adClickAttributionMap into
m_unconvertedAdClickAttributionMap and m_convertedAdClickAttributionMap, which now use a std::pair
as key instead of a nested HashMap.

This patch also changes AdClickAttributionManager::firePendingConversionRequests() so that it now
removes attributions which have been sent out.

Finally, AdClickAttributionManager::clear() no longer clears m_conversionBaseURLForTesting and
m_isRunningTest since doing so caused test flakiness. It is now up to the test case that sets these
members to also clear them when done.

* NetworkProcess/AdClickAttributionManager.cpp:
(WebKit::AdClickAttributionManager::storeUnconverted):
(WebKit::AdClickAttributionManager::convert):
(WebKit::AdClickAttributionManager::firePendingConversionRequests):
(WebKit::AdClickAttributionManager::clear):
(WebKit::AdClickAttributionManager::toString const):
(WebKit::AdClickAttributionManager::setConversionURLForTesting):
(WebKit::AdClickAttributionManager::ensureDestinationMapForSource): Deleted.
(WebKit::AdClickAttributionManager::store): Deleted.
* NetworkProcess/AdClickAttributionManager.h:
(WebKit::AdClickAttributionManager::AdClickAttributionManager):
(WebKit::AdClickAttributionManager::setConversionURLForTesting): Deleted.
* NetworkProcess/NetworkSession.cpp:
(WebKit::NetworkSession::storeAdClickAttribution):

LayoutTests:

* http/tests/adClickAttribution/attribution-conversion-through-cross-site-image-redirect-expected.txt:
* http/tests/adClickAttribution/attribution-conversion-through-image-redirect-with-priority-expected.txt:
* http/tests/adClickAttribution/attribution-conversion-through-image-redirect-without-priority-expected.txt:
* http/tests/adClickAttribution/resources/getConversionData.php:
* http/tests/adClickAttribution/resources/redirectToConversion.php:
* http/tests/adClickAttribution/second-attribution-converted-with-higher-priority-expected.txt: Added.
* http/tests/adClickAttribution/second-attribution-converted-with-higher-priority.html: Added.
* http/tests/adClickAttribution/second-attribution-converted-with-lower-priority-expected.txt: Added.
* http/tests/adClickAttribution/second-attribution-converted-with-lower-priority.html: Added.
* http/tests/adClickAttribution/second-conversion-with-higher-priority-expected.txt: Added.
* http/tests/adClickAttribution/second-conversion-with-higher-priority.html: Added.
* http/tests/adClickAttribution/second-conversion-with-lower-priority-expected.txt: Added.
* http/tests/adClickAttribution/second-conversion-with-lower-priority.html: Added.
* http/tests/adClickAttribution/send-attribution-conversion-request-expected.txt:
* http/tests/adClickAttribution/send-attribution-conversion-request.html:
* http/tests/adClickAttribution/store-ad-click-attribution-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244402 268f45cc-cd09-0410-ab3c-d52691b4dbfc

24 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/adClickAttribution/attribution-conversion-through-cross-site-image-redirect-expected.txt
LayoutTests/http/tests/adClickAttribution/attribution-conversion-through-image-redirect-with-priority-expected.txt
LayoutTests/http/tests/adClickAttribution/attribution-conversion-through-image-redirect-without-priority-expected.txt
LayoutTests/http/tests/adClickAttribution/resources/getConversionData.php
LayoutTests/http/tests/adClickAttribution/resources/redirectToConversion.php
LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-higher-priority-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-higher-priority.html [new file with mode: 0644]
LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-lower-priority-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-lower-priority.html [new file with mode: 0644]
LayoutTests/http/tests/adClickAttribution/second-conversion-with-higher-priority-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/adClickAttribution/second-conversion-with-higher-priority.html [new file with mode: 0644]
LayoutTests/http/tests/adClickAttribution/second-conversion-with-lower-priority-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/adClickAttribution/second-conversion-with-lower-priority.html [new file with mode: 0644]
LayoutTests/http/tests/adClickAttribution/send-attribution-conversion-request-expected.txt
LayoutTests/http/tests/adClickAttribution/send-attribution-conversion-request.html
LayoutTests/http/tests/adClickAttribution/store-ad-click-attribution-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/loader/AdClickAttribution.cpp
Source/WebCore/loader/AdClickAttribution.h
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp
Source/WebKit/NetworkProcess/AdClickAttributionManager.h
Source/WebKit/NetworkProcess/NetworkSession.cpp

index 4d4dcff..ef78a26 100644 (file)
@@ -1,3 +1,28 @@
+2019-04-17  John Wilander  <wilander@apple.com>
+
+        Add prioritization of ad click conversions and cleaning of sent ad click conversions
+        https://bugs.webkit.org/show_bug.cgi?id=196934
+        <rdar://problem/49917773>
+
+        Reviewed by Chris Dumez.
+
+        * http/tests/adClickAttribution/attribution-conversion-through-cross-site-image-redirect-expected.txt:
+        * http/tests/adClickAttribution/attribution-conversion-through-image-redirect-with-priority-expected.txt:
+        * http/tests/adClickAttribution/attribution-conversion-through-image-redirect-without-priority-expected.txt:
+        * http/tests/adClickAttribution/resources/getConversionData.php:
+        * http/tests/adClickAttribution/resources/redirectToConversion.php:
+        * http/tests/adClickAttribution/second-attribution-converted-with-higher-priority-expected.txt: Added.
+        * http/tests/adClickAttribution/second-attribution-converted-with-higher-priority.html: Added.
+        * http/tests/adClickAttribution/second-attribution-converted-with-lower-priority-expected.txt: Added.
+        * http/tests/adClickAttribution/second-attribution-converted-with-lower-priority.html: Added.
+        * http/tests/adClickAttribution/second-conversion-with-higher-priority-expected.txt: Added.
+        * http/tests/adClickAttribution/second-conversion-with-higher-priority.html: Added.
+        * http/tests/adClickAttribution/second-conversion-with-lower-priority-expected.txt: Added.
+        * http/tests/adClickAttribution/second-conversion-with-lower-priority.html: Added.
+        * http/tests/adClickAttribution/send-attribution-conversion-request-expected.txt:
+        * http/tests/adClickAttribution/send-attribution-conversion-request.html:
+        * http/tests/adClickAttribution/store-ad-click-attribution-expected.txt:
+
 2019-04-17  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         REGRESSION (r244220): fast/forms/ios/inputmode-change-update-keyboard.html times out
index 99af8d8..6f1124b 100644 (file)
@@ -33,6 +33,7 @@ if ($conversionFileFound) {
         echo "<br>";
         echo trim($line);
     }
+    echo "<br>";
     fclose($conversionFile);
     unlink($conversionFilePath);
 } else {
@@ -41,8 +42,11 @@ if ($conversionFileFound) {
 
 if (isset($_GET['endTest'])) {
     echo "<script>";
-    echo "if (window.testRunner)";
+    echo "if (window.testRunner) {";
     echo "    testRunner.notifyDone();";
+    echo "    testRunner.setAdClickAttributionOverrideTimerForTesting(false);";
+    echo "    testRunner.setAdClickAttributionConversionURLForTesting('');";
+    echo "}";
     echo "</script>";
 }
 
index 3d71a83..70ceb46 100644 (file)
@@ -1,5 +1,5 @@
 <?php
-header("HTTP/1.0 302 Found");
+header("HTTP/1.1 302 Found");
 header("Cache-Control: no-cache, no-store, must-revalidate");
 if (isset($_GET["conversionData"]) && isset($_GET["priority"])) {
   header("Location: /.well-known/ad-click-attribution/" . $_GET["conversionData"] . "/" . $_GET["priority"]);
diff --git a/LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-higher-priority-expected.txt b/LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-higher-priority-expected.txt
new file mode 100644 (file)
index 0000000..a90fcbc
--- /dev/null
@@ -0,0 +1,12 @@
+Tests that a second attribution conversion with higher priority replaces an older with lower priority.
+
+
+Converted Ad Click Attributions:
+WebCore::AdClickAttribution 1
+Source: 127.0.0.1
+Destination: localhost
+Campaign ID: 4
+Conversion data: 12
+Conversion priority: 4
+Conversion earliest time to send: Within 24-48 hours
+Conversion request sent: false
diff --git a/LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-higher-priority.html b/LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-higher-priority.html
new file mode 100644 (file)
index 0000000..49200f7
--- /dev/null
@@ -0,0 +1,95 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AdClickAttributionEnabled=true ] -->
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+    <script src="/js-test-resources/ui-helper.js"></script>
+</head>
+<body onload="setTimeout(runTest, 0)">
+<div id="description">Tests that a second attribution conversion with higher priority replaces an older with lower priority.</div>
+<a id="targetLink">Link</a><br>
+<div id="output"></div>
+<script>
+    const path = "/adClickAttribution/second-attribution-converted-with-higher-priority.html";
+    const configuration = [
+        {
+            href: "http://localhost:8000" + path + "?stepTwo",
+            adcampaignid: "3",
+            addestination: "http://localhost:8000"
+        },
+        {
+            href: "http://localhost:8000" + path + "?stepFour",
+            adcampaignid: "4",
+            addestination: "http://localhost:8000"
+        }
+    ];
+
+    function configureLink(index) {
+        let linkElement = document.getElementById("targetLink");
+        linkElement.setAttribute("href", configuration[index].href);
+        linkElement.setAttribute("adcampaignid", configuration[index].adcampaignid);
+        linkElement.setAttribute("addestination", configuration[index].addestination);
+    }
+
+    if (window.testRunner) {
+        testRunner.waitUntilDone();
+        testRunner.dumpAsText();
+        testRunner.setAllowsAnySSLCertificate(true);
+    }
+
+    function activateElement(elementID) {
+        var element = document.getElementById(elementID);
+        var centerX = element.offsetLeft + element.offsetWidth / 2;
+        var centerY = element.offsetTop + element.offsetHeight / 2;
+        UIHelper.activateAt(centerX, centerY).then(
+            function () {
+            },
+            function () {
+                document.getElementById("output").innerText = "FAIL Promise rejected.";
+                testRunner.notifyDone();
+            }
+        );
+    }
+
+    function runTest() {
+        if (window.testRunner) {
+            if (window.location.search === "") {
+                // First ad click 127.0.0.1 –> localhost.
+                configureLink(0);
+                activateElement("targetLink");
+            } else if (window.location.search === "?stepTwo") {
+                // Convert the first ad click with priority 3 and navigate back to 127.0.0.1.
+                let imageElement = document.createElement("img");
+                imageElement.src = "https://127.0.0.1:8443/adClickAttribution/resources/redirectToConversion.php?conversionData=12&priority=03";
+                imageElement.id = "pixel";
+                imageElement.onerror = function(e) {
+                    document.location.href = "http://127.0.0.1:8000" + path + "?stepThree";
+                };
+                document.body.appendChild(imageElement);
+            } else if (window.location.search === "?stepThree") {
+                // Second ad click 127.0.0.1 –> localhost.
+                configureLink(1);
+                activateElement("targetLink");
+            } else if (window.location.search === "?stepFour") {
+                // Convert the second ad click with priority 4 and finish.
+                let imageElement = document.createElement("img");
+                imageElement.src = "https://127.0.0.1:8443/adClickAttribution/resources/redirectToConversion.php?conversionData=12&priority=04";
+                imageElement.id = "pixel";
+                imageElement.onerror = function(e) {
+                    testRunner.dumpAdClickAttribution();
+                    document.body.removeChild(document.getElementById("targetLink"));
+                    document.body.removeChild(document.getElementById("pixel"));
+                    testRunner.notifyDone();
+                };
+                document.body.appendChild(imageElement);
+            } else {
+                document.getElementById("output").innerText = "FAIL Unknown window.location.search == " + window.location.search + ".";
+                testRunner.notifyDone();
+            }
+        } else {
+            document.getElementById("output").innerText = "FAIL No testRunner.";
+        }
+    }
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-lower-priority-expected.txt b/LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-lower-priority-expected.txt
new file mode 100644 (file)
index 0000000..a7e2df7
--- /dev/null
@@ -0,0 +1,12 @@
+Tests that a second attribution conversion with lower priority does not replace an older with higher priority.
+
+
+Converted Ad Click Attributions:
+WebCore::AdClickAttribution 1
+Source: 127.0.0.1
+Destination: localhost
+Campaign ID: 3
+Conversion data: 12
+Conversion priority: 4
+Conversion earliest time to send: Within 24-48 hours
+Conversion request sent: false
diff --git a/LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-lower-priority.html b/LayoutTests/http/tests/adClickAttribution/second-attribution-converted-with-lower-priority.html
new file mode 100644 (file)
index 0000000..c3e1883
--- /dev/null
@@ -0,0 +1,95 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AdClickAttributionEnabled=true ] -->
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+    <script src="/js-test-resources/ui-helper.js"></script>
+</head>
+<body onload="setTimeout(runTest, 0)">
+<div id="description">Tests that a second attribution conversion with lower priority does not replace an older with higher priority.</div>
+<a id="targetLink">Link</a><br>
+<div id="output"></div>
+<script>
+    const path = "/adClickAttribution/second-attribution-converted-with-lower-priority.html";
+    const configuration = [
+        {
+            href: "http://localhost:8000" + path + "?stepTwo",
+            adcampaignid: "3",
+            addestination: "http://localhost:8000"
+        },
+        {
+            href: "http://localhost:8000" + path + "?stepFour",
+            adcampaignid: "4",
+            addestination: "http://localhost:8000"
+        }
+    ];
+
+    function configureLink(index) {
+        let linkElement = document.getElementById("targetLink");
+        linkElement.setAttribute("href", configuration[index].href);
+        linkElement.setAttribute("adcampaignid", configuration[index].adcampaignid);
+        linkElement.setAttribute("addestination", configuration[index].addestination);
+    }
+
+    if (window.testRunner) {
+        testRunner.waitUntilDone();
+        testRunner.dumpAsText();
+        testRunner.setAllowsAnySSLCertificate(true);
+    }
+
+    function activateElement(elementID) {
+        var element = document.getElementById(elementID);
+        var centerX = element.offsetLeft + element.offsetWidth / 2;
+        var centerY = element.offsetTop + element.offsetHeight / 2;
+        UIHelper.activateAt(centerX, centerY).then(
+            function () {
+            },
+            function () {
+                document.getElementById("output").innerText = "FAIL Promise rejected.";
+                testRunner.notifyDone();
+            }
+        );
+    }
+
+    function runTest() {
+        if (window.testRunner) {
+            if (window.location.search === "") {
+                // First ad click 127.0.0.1 –> localhost.
+                configureLink(0);
+                activateElement("targetLink");
+            } else if (window.location.search === "?stepTwo") {
+                // Convert the first ad click with priority 4 and navigate back to 127.0.0.1.
+                let imageElement = document.createElement("img");
+                imageElement.src = "https://127.0.0.1:8443/adClickAttribution/resources/redirectToConversion.php?conversionData=12&priority=04";
+                imageElement.id = "pixel";
+                imageElement.onerror = function(e) {
+                    document.location.href = "http://127.0.0.1:8000" + path + "?stepThree";
+                };
+                document.body.appendChild(imageElement);
+            } else if (window.location.search === "?stepThree") {
+                // Second ad click 127.0.0.1 –> localhost.
+                configureLink(1);
+                activateElement("targetLink");
+            } else if (window.location.search === "?stepFour") {
+                // Convert the second ad click with priority 3 and finish.
+                let imageElement = document.createElement("img");
+                imageElement.src = "https://127.0.0.1:8443/adClickAttribution/resources/redirectToConversion.php?conversionData=12&priority=03";
+                imageElement.id = "pixel";
+                imageElement.onerror = function(e) {
+                    testRunner.dumpAdClickAttribution();
+                    document.body.removeChild(document.getElementById("targetLink"));
+                    document.body.removeChild(document.getElementById("pixel"));
+                    testRunner.notifyDone();
+                };
+                document.body.appendChild(imageElement);
+            } else {
+                document.getElementById("output").innerText = "FAIL Unknown window.location.search == " + window.location.search + ".";
+                testRunner.notifyDone();
+            }
+        } else {
+            document.getElementById("output").innerText = "FAIL No testRunner.";
+        }
+    }
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/adClickAttribution/second-conversion-with-higher-priority-expected.txt b/LayoutTests/http/tests/adClickAttribution/second-conversion-with-higher-priority-expected.txt
new file mode 100644 (file)
index 0000000..3e1b1ba
--- /dev/null
@@ -0,0 +1,12 @@
+Tests that the attribution is updated if it gets a second conversion with higher priority.
+
+
+Converted Ad Click Attributions:
+WebCore::AdClickAttribution 1
+Source: 127.0.0.1
+Destination: localhost
+Campaign ID: 3
+Conversion data: 12
+Conversion priority: 4
+Conversion earliest time to send: Within 24-48 hours
+Conversion request sent: false
diff --git a/LayoutTests/http/tests/adClickAttribution/second-conversion-with-higher-priority.html b/LayoutTests/http/tests/adClickAttribution/second-conversion-with-higher-priority.html
new file mode 100644 (file)
index 0000000..c8efe14
--- /dev/null
@@ -0,0 +1,88 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AdClickAttributionEnabled=true ] -->
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+    <script src="/js-test-resources/ui-helper.js"></script>
+</head>
+<body onload="setTimeout(runTest, 0)">
+<div id="description">Tests that the attribution is updated if it gets a second conversion with higher priority.</div>
+<a id="targetLink">Link</a><br>
+<div id="output"></div>
+<script>
+    const path = "/adClickAttribution/second-conversion-with-higher-priority.html";
+    const configuration = [
+        {
+            href: "http://localhost:8000" + path + "?stepTwo",
+            adcampaignid: "3",
+            addestination: "http://localhost:8000"
+        }
+    ];
+
+    function configureLink(index) {
+        let linkElement = document.getElementById("targetLink");
+        linkElement.setAttribute("href", configuration[index].href);
+        linkElement.setAttribute("adcampaignid", configuration[index].adcampaignid);
+        linkElement.setAttribute("addestination", configuration[index].addestination);
+    }
+
+    if (window.testRunner) {
+        testRunner.waitUntilDone();
+        testRunner.dumpAsText();
+        testRunner.setAllowsAnySSLCertificate(true);
+    }
+
+    function activateElement(elementID) {
+        var element = document.getElementById(elementID);
+        var centerX = element.offsetLeft + element.offsetWidth / 2;
+        var centerY = element.offsetTop + element.offsetHeight / 2;
+        UIHelper.activateAt(centerX, centerY).then(
+            function () {
+            },
+            function () {
+                document.getElementById("output").innerText = "FAIL Promise rejected.";
+                testRunner.notifyDone();
+            }
+        );
+    }
+
+    function convert(priority, callback) {
+        let pixelElement = document.getElementById("pixel");
+        if (pixelElement)
+            document.body.removeChild(pixelElement);
+
+        let imageElement = document.createElement("img");
+        imageElement.src = "https://127.0.0.1:8443/adClickAttribution/resources/redirectToConversion.php?conversionData=12&priority=" + priority;
+        imageElement.id = "pixel";
+        imageElement.onerror = callback;
+        document.body.appendChild(imageElement);
+    }
+
+    function runTest() {
+        if (window.testRunner) {
+            if (window.location.search === "") {
+                // Ad click 127.0.0.1 –> localhost.
+                configureLink(0);
+                activateElement("targetLink");
+            } else if (window.location.search === "?stepTwo") {
+                // Convert the ad click with priority 3.
+                convert("03", function() {
+                    // Convert the ad click with priority 4.
+                    convert("04", function() {
+                        testRunner.dumpAdClickAttribution();
+                        document.body.removeChild(document.getElementById("targetLink"));
+                        document.body.removeChild(document.getElementById("pixel"));
+                        testRunner.notifyDone();
+                    });
+                });
+            } else {
+                document.getElementById("output").innerText = "FAIL Unknown window.location.search == " + window.location.search + ".";
+                testRunner.notifyDone();
+            }
+        } else {
+            document.getElementById("output").innerText = "FAIL No testRunner.";
+        }
+    }
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/adClickAttribution/second-conversion-with-lower-priority-expected.txt b/LayoutTests/http/tests/adClickAttribution/second-conversion-with-lower-priority-expected.txt
new file mode 100644 (file)
index 0000000..703cfdb
--- /dev/null
@@ -0,0 +1,12 @@
+Tests that the attribution is not updated if it gets a second conversion with lower priority.
+
+
+Converted Ad Click Attributions:
+WebCore::AdClickAttribution 1
+Source: 127.0.0.1
+Destination: localhost
+Campaign ID: 3
+Conversion data: 12
+Conversion priority: 4
+Conversion earliest time to send: Within 24-48 hours
+Conversion request sent: false
diff --git a/LayoutTests/http/tests/adClickAttribution/second-conversion-with-lower-priority.html b/LayoutTests/http/tests/adClickAttribution/second-conversion-with-lower-priority.html
new file mode 100644 (file)
index 0000000..b6efba1
--- /dev/null
@@ -0,0 +1,88 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true internal:AdClickAttributionEnabled=true ] -->
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+    <script src="/js-test-resources/ui-helper.js"></script>
+</head>
+<body onload="setTimeout(runTest, 0)">
+<div id="description">Tests that the attribution is not updated if it gets a second conversion with lower priority.</div>
+<a id="targetLink">Link</a><br>
+<div id="output"></div>
+<script>
+    const path = "/adClickAttribution/second-conversion-with-lower-priority.html";
+    const configuration = [
+        {
+            href: "http://localhost:8000" + path + "?stepTwo",
+            adcampaignid: "3",
+            addestination: "http://localhost:8000"
+        }
+    ];
+
+    function configureLink(index) {
+        let linkElement = document.getElementById("targetLink");
+        linkElement.setAttribute("href", configuration[index].href);
+        linkElement.setAttribute("adcampaignid", configuration[index].adcampaignid);
+        linkElement.setAttribute("addestination", configuration[index].addestination);
+    }
+
+    if (window.testRunner) {
+        testRunner.waitUntilDone();
+        testRunner.dumpAsText();
+        testRunner.setAllowsAnySSLCertificate(true);
+    }
+
+    function activateElement(elementID) {
+        var element = document.getElementById(elementID);
+        var centerX = element.offsetLeft + element.offsetWidth / 2;
+        var centerY = element.offsetTop + element.offsetHeight / 2;
+        UIHelper.activateAt(centerX, centerY).then(
+            function () {
+            },
+            function () {
+                document.getElementById("output").innerText = "FAIL Promise rejected.";
+                testRunner.notifyDone();
+            }
+        );
+    }
+
+    function convert(priority, callback) {
+        let pixelElement = document.getElementById("pixel");
+        if (pixelElement)
+            document.body.removeChild(pixelElement);
+
+        let imageElement = document.createElement("img");
+        imageElement.src = "https://127.0.0.1:8443/adClickAttribution/resources/redirectToConversion.php?conversionData=12&priority=" + priority;
+        imageElement.id = "pixel";
+        imageElement.onerror = callback;
+        document.body.appendChild(imageElement);
+    }
+
+    function runTest() {
+        if (window.testRunner) {
+            if (window.location.search === "") {
+                // Ad click 127.0.0.1 –> localhost.
+                configureLink(0);
+                activateElement("targetLink");
+            } else if (window.location.search === "?stepTwo") {
+                // Convert the ad click with priority 4.
+                convert("04", function() {
+                    // Convert the ad click with priority 3.
+                    convert("03", function() {
+                        testRunner.dumpAdClickAttribution();
+                        document.body.removeChild(document.getElementById("targetLink"));
+                        document.body.removeChild(document.getElementById("pixel"));
+                        testRunner.notifyDone();
+                    });
+                });
+            } else {
+                document.getElementById("output").innerText = "FAIL Unknown window.location.search == " + window.location.search + ".";
+                testRunner.notifyDone();
+            }
+        } else {
+            document.getElementById("output").innerText = "FAIL No testRunner.";
+        }
+    }
+</script>
+</body>
+</html>
index 3ea9012..77e28b0 100644 (file)
@@ -9,11 +9,5 @@ Conversion received.
 HTTP_HOST: 127.0.0.1:8000
 REQUEST_URI: /adClickAttribution/resources/conversionReport.php?conversion=12&campaign=3
 No cookies in conversion request.
-WebCore::AdClickAttribution 1
-Source: 127.0.0.1
-Destination: localhost
-Campaign ID: 3
-Conversion data: 12
-Conversion priority: 0
-Conversion earliest time to send: Within 24-48 hours
-Conversion request sent: true
+
+No stored Ad Click Attribution data.
index 000987f..ea22698 100644 (file)
@@ -18,6 +18,8 @@
         testRunner.waitUntilDone();
         testRunner.dumpChildFramesAsText();
         testRunner.setAllowsAnySSLCertificate(true);
+        testRunner.setAdClickAttributionConversionURLForTesting("http://127.0.0.1:8000/adClickAttribution/resources/conversionReport.php?nonce=" + nonce);
+        testRunner.setAdClickAttributionOverrideTimerForTesting(true);
     }
 
     function activateElement(elementID) {
@@ -53,8 +55,6 @@
                 imageElement.onerror = function() {
                     appendConversionDataIframeAndFinish();
                 };
-                testRunner.setAdClickAttributionConversionURLForTesting("http://127.0.0.1:8000/adClickAttribution/resources/conversionReport.php?nonce=" + nonce);
-                testRunner.setAdClickAttributionOverrideTimerForTesting(true);
                 document.body.appendChild(imageElement);
             } else {
                 activateElement("targetLink");
index 883bb7b..cbe4747 100644 (file)
@@ -1,3 +1,25 @@
+2019-04-17  John Wilander  <wilander@apple.com>
+
+        Add prioritization of ad click conversions and cleaning of sent ad click conversions
+        https://bugs.webkit.org/show_bug.cgi?id=196934
+        <rdar://problem/49917773>
+
+        Reviewed by Chris Dumez.
+
+        Tests: http/tests/adClickAttribution/second-attribution-converted-with-higher-priority.html
+               http/tests/adClickAttribution/second-attribution-converted-with-lower-priority.html
+               http/tests/adClickAttribution/second-conversion-with-higher-priority.html
+               http/tests/adClickAttribution/second-conversion-with-lower-priority.html
+
+        * loader/AdClickAttribution.cpp:
+        (WebCore::AdClickAttribution::hasHigherPriorityThan const):
+            Added to facilitate priority comparison between two attributions.
+        * loader/AdClickAttribution.h:
+        (WebCore::AdClickAttribution::Destination::Destination):
+            Added a WTF::HashTableDeletedValueType constructor and changed the copy constructor to
+            a move constructor.
+        (WebCore::AdClickAttribution::isEmpty const):
+
 2019-04-17  Devin Rousso  <drousso@apple.com>
 
         AX: AccessibilityObject::parentObject() doesn't need to be pure virtual
index f16c94c..3222c89 100644 (file)
@@ -93,6 +93,17 @@ Optional<Seconds> AdClickAttribution::convertAndGetEarliestTimeToSend(Conversion
     return seconds;
 }
 
+bool AdClickAttribution::hasHigherPriorityThan(const AdClickAttribution& other) const
+{
+    if (!other.m_conversion)
+        return true;
+    
+    if (!m_conversion)
+        return false;
+
+    return m_conversion->priority > other.m_conversion->priority;
+}
+
 URL AdClickAttribution::url() const
 {
     if (!isValid())
index 0ef30dd..5c67938 100644 (file)
@@ -135,11 +135,16 @@ public:
         {
         }
 
-        explicit Destination(const RegistrableDomain& domain)
-            : registrableDomain { domain }
+        explicit Destination(WTF::HashTableDeletedValueType)
+            : registrableDomain { WTF::HashTableDeletedValue }
         {
         }
 
+        explicit Destination(RegistrableDomain&& domain)
+            : registrableDomain { WTFMove(domain) }
+        {
+        }
+        
         bool operator==(const Destination& other) const
         {
             return registrableDomain == other.registrableDomain;
@@ -236,6 +241,7 @@ public:
 
     WEBCORE_EXPORT static Optional<Conversion> parseConversionRequest(const URL& redirectURL);
     WEBCORE_EXPORT Optional<Seconds> convertAndGetEarliestTimeToSend(Conversion&&);
+    WEBCORE_EXPORT bool hasHigherPriorityThan(const AdClickAttribution&) const;
     WEBCORE_EXPORT URL url() const;
     WEBCORE_EXPORT URL urlForTesting(const URL& baseURLForTesting) const;
     WEBCORE_EXPORT URL referrer() const;
@@ -245,6 +251,8 @@ public:
     WEBCORE_EXPORT void markConversionAsSent();
     WEBCORE_EXPORT bool wasConversionSent() const;
 
+    bool isEmpty() const { return m_source.registrableDomain.isEmpty(); };
+
     WEBCORE_EXPORT String toString() const;
 
     template<class Encoder> void encode(Encoder&) const;
index f544207..6866547 100644 (file)
@@ -1,3 +1,46 @@
+2019-04-17  John Wilander  <wilander@apple.com>
+
+        Add prioritization of ad click conversions and cleaning of sent ad click conversions
+        https://bugs.webkit.org/show_bug.cgi?id=196934
+        <rdar://problem/49917773>
+
+        Reviewed by Chris Dumez.
+
+        In this description, by "pair" I mean { AdClickAttribution::Source, AdClickAttribution::Destination }.
+
+        This patch adds handling of prioritization of conversions according to these rules:
+        - If we have a matching unconverted attribution, convert it. This consumes the conversion.
+        - If we have no previously converted attribution for this pair, just store.
+        - If we have a previously converted attribution for this pair, replace it if the new one has higher priority.
+        - If we had no matching unconverted attribution but do have a previously converted attribution for this
+        pair, re-convert the previously converted attribution to make sure the highest priority gets set.
+
+        This handling is in part done by dividing the previous m_adClickAttributionMap into 
+        m_unconvertedAdClickAttributionMap and m_convertedAdClickAttributionMap, which now use a std::pair
+        as key instead of a nested HashMap.
+
+        This patch also changes AdClickAttributionManager::firePendingConversionRequests() so that it now
+        removes attributions which have been sent out.
+
+        Finally, AdClickAttributionManager::clear() no longer clears m_conversionBaseURLForTesting and
+        m_isRunningTest since doing so caused test flakiness. It is now up to the test case that sets these
+        members to also clear them when done.
+
+        * NetworkProcess/AdClickAttributionManager.cpp:
+        (WebKit::AdClickAttributionManager::storeUnconverted):
+        (WebKit::AdClickAttributionManager::convert):
+        (WebKit::AdClickAttributionManager::firePendingConversionRequests):
+        (WebKit::AdClickAttributionManager::clear):
+        (WebKit::AdClickAttributionManager::toString const):
+        (WebKit::AdClickAttributionManager::setConversionURLForTesting):
+        (WebKit::AdClickAttributionManager::ensureDestinationMapForSource): Deleted.
+        (WebKit::AdClickAttributionManager::store): Deleted.
+        * NetworkProcess/AdClickAttributionManager.h:
+        (WebKit::AdClickAttributionManager::AdClickAttributionManager):
+        (WebKit::AdClickAttributionManager::setConversionURLForTesting): Deleted.
+        * NetworkProcess/NetworkSession.cpp:
+        (WebKit::NetworkSession::storeAdClickAttribution):
+
 2019-04-17  Tim Horton  <timothy_horton@apple.com>
 
         UI↔Web deadlock when printing with a JavaScript alert visible
index d74b006..45358a9 100644 (file)
@@ -43,17 +43,9 @@ using Destination = AdClickAttribution::Destination;
 using DestinationMap = HashMap<Destination, AdClickAttribution>;
 using Conversion = AdClickAttribution::Conversion;
 
-DestinationMap& AdClickAttributionManager::ensureDestinationMapForSource(const Source& source)
+void AdClickAttributionManager::storeUnconverted(AdClickAttribution&& attribution)
 {
-    return m_adClickAttributionMap.ensure(source, [] {
-        return DestinationMap { };
-    }).iterator->value;
-}
-
-void AdClickAttributionManager::store(AdClickAttribution&& adClickAttribution)
-{
-    auto& destinationMapForSource = ensureDestinationMapForSource(adClickAttribution.source());
-    destinationMapForSource.add(adClickAttribution.destination(), WTFMove(adClickAttribution));
+    m_unconvertedAdClickAttributionMap.set(std::make_pair(attribution.source(), attribution.destination()), WTFMove(attribution));
 }
 
 void AdClickAttributionManager::startTimer(Seconds seconds)
@@ -66,23 +58,38 @@ void AdClickAttributionManager::convert(const Source& source, const Destination&
     if (!conversion.isValid())
         return;
 
-    auto sourceIter = m_adClickAttributionMap.find(source);
-    if (sourceIter == m_adClickAttributionMap.end())
-        return;
+    auto secondsUntilSend = Seconds::infinity();
 
-    auto destinationIter = sourceIter->value.find(destination);
-    if (destinationIter == sourceIter->value.end())
-        return;
+    auto pair = std::make_pair(source, destination);
+    auto previouslyUnconvertedAttribution = m_unconvertedAdClickAttributionMap.take(pair);
+    auto previouslyConvertedAttributionIter = m_convertedAdClickAttributionMap.find(pair);
 
-    auto secondsUntilSend = destinationIter->value.convertAndGetEarliestTimeToSend(WTFMove(conversion));
-    
-    if (!secondsUntilSend)
+    if (!previouslyUnconvertedAttribution.isEmpty()) {
+        // Always convert the pending attribution and remove it from the unconverted map.
+        if (auto optionalSecondsUntilSend = previouslyUnconvertedAttribution.convertAndGetEarliestTimeToSend(WTFMove(conversion))) {
+            secondsUntilSend = *optionalSecondsUntilSend;
+            ASSERT(secondsUntilSend != Seconds::infinity());
+        }
+        // If there is no previously converted attribution for this pair, add the new one.
+        // If the newly converted attribution has higher priority, replace the old one.
+        if (previouslyConvertedAttributionIter == m_convertedAdClickAttributionMap.end()
+            || previouslyUnconvertedAttribution.hasHigherPriorityThan(previouslyConvertedAttributionIter->value))
+            m_convertedAdClickAttributionMap.set(pair, WTFMove(previouslyUnconvertedAttribution));
+    } else if (previouslyConvertedAttributionIter != m_convertedAdClickAttributionMap.end()) {
+        // If we have no newly converted attribution, re-convert the old one to respect the new priority.
+        if (auto optionalSecondsUntilSend = previouslyConvertedAttributionIter->value.convertAndGetEarliestTimeToSend(WTFMove(conversion))) {
+            secondsUntilSend = *optionalSecondsUntilSend;
+            ASSERT(secondsUntilSend != Seconds::infinity());
+        }
+    }
+
+    if (secondsUntilSend == Seconds::infinity())
         return;
     
-    if (m_firePendingConversionRequestsTimer.isActive() && m_firePendingConversionRequestsTimer.nextFireInterval() < *secondsUntilSend)
+    if (m_firePendingConversionRequestsTimer.isActive() && m_firePendingConversionRequestsTimer.nextFireInterval() < secondsUntilSend)
         return;
     
-    startTimer(*secondsUntilSend);
+    startTimer(secondsUntilSend);
 }
 
 void AdClickAttributionManager::fireConversionRequest(const AdClickAttribution& attribution)
@@ -133,59 +140,83 @@ void AdClickAttributionManager::fireConversionRequest(const AdClickAttribution&
 void AdClickAttributionManager::firePendingConversionRequests()
 {
     auto nextTimeToFire = Seconds::infinity();
-    for (auto& innerMap : m_adClickAttributionMap.values()) {
-        for (auto& attribution : innerMap.values()) {
-            if (attribution.wasConversionSent()) {
-                ASSERT_NOT_REACHED();
-                continue;
-            }
-            auto earliestTimeToSend = attribution.earliestTimeToSend();
-            if (!earliestTimeToSend)
-                continue;
-
-            auto now = WallTime::now();
-            if (*earliestTimeToSend <= now || m_isRunningTest) {
-                fireConversionRequest(attribution);                
-                attribution.markConversionAsSent();
-                continue;
-            }
-
-            auto seconds = *earliestTimeToSend - now;
-            nextTimeToFire = std::min(nextTimeToFire, seconds);
+    for (auto& attribution : m_convertedAdClickAttributionMap.values()) {
+        if (attribution.wasConversionSent()) {
+            ASSERT_NOT_REACHED();
+            continue;
+        }
+        auto earliestTimeToSend = attribution.earliestTimeToSend();
+        if (!earliestTimeToSend) {
+            ASSERT_NOT_REACHED();
+            continue;
+        }
+
+        auto now = WallTime::now();
+        if (*earliestTimeToSend <= now || m_isRunningTest) {
+            fireConversionRequest(attribution);
+            attribution.markConversionAsSent();
+            continue;
         }
+
+        auto seconds = *earliestTimeToSend - now;
+        nextTimeToFire = std::min(nextTimeToFire, seconds);
     }
 
+    m_convertedAdClickAttributionMap.removeIf([](auto& keyAndValue) {
+        return keyAndValue.value.wasConversionSent();
+    });
+
     if (nextTimeToFire < Seconds::infinity())
         startTimer(nextTimeToFire);
-
-    // FIXME: Add cleaning of sent conversions.
 }
 
 void AdClickAttributionManager::clear(CompletionHandler<void()>&& completionHandler)
 {
-    m_adClickAttributionMap.clear();
-    m_conversionBaseURLForTesting = { };
-    m_isRunningTest = false;
+    m_firePendingConversionRequestsTimer.stop();
+    m_unconvertedAdClickAttributionMap.clear();
+    m_convertedAdClickAttributionMap.clear();
     completionHandler();
 }
 
 void AdClickAttributionManager::toString(CompletionHandler<void(String)>&& completionHandler) const
 {
-    if (m_adClickAttributionMap.isEmpty())
-        return completionHandler("No stored Ad Click Attribution data."_s);
+    if (m_unconvertedAdClickAttributionMap.isEmpty() && m_convertedAdClickAttributionMap.isEmpty())
+        return completionHandler("No stored Ad Click Attribution data.\n"_s);
 
+    unsigned unconvertedAttributionNumber = 0;
     StringBuilder builder;
-    for (auto& innerMap : m_adClickAttributionMap.values()) {
-        unsigned attributionNumber = 1;
-        for (auto& attribution : innerMap.values()) {
-            builder.appendLiteral("WebCore::AdClickAttribution ");
-            builder.appendNumber(attributionNumber++);
+    for (auto& attribution : m_unconvertedAdClickAttributionMap.values()) {
+        if (!unconvertedAttributionNumber)
+            builder.appendLiteral("Unconverted Ad Click Attributions:\n");
+        else
             builder.append('\n');
-            builder.append(attribution.toString());
-        }
+        builder.appendLiteral("WebCore::AdClickAttribution ");
+        builder.appendNumber(++unconvertedAttributionNumber);
+        builder.append('\n');
+        builder.append(attribution.toString());
+}
+
+    unsigned convertedAttributionNumber = 0;
+    for (auto& attribution : m_convertedAdClickAttributionMap.values()) {
+        if (!convertedAttributionNumber)
+            builder.appendLiteral("Converted Ad Click Attributions:\n");
+        else
+            builder.append('\n');
+        builder.appendLiteral("WebCore::AdClickAttribution ");
+        builder.appendNumber(++convertedAttributionNumber + unconvertedAttributionNumber);
+        builder.append('\n');
+        builder.append(attribution.toString());
     }
 
     completionHandler(builder.toString());
 }
 
+void AdClickAttributionManager::setConversionURLForTesting(URL&& testURL)
+{
+    if (testURL.isEmpty())
+        m_conversionBaseURLForTesting = { };
+    else
+        m_conversionBaseURLForTesting = WTFMove(testURL);
+}
+
 } // namespace WebKit
index 7d98fb5..0b24cab 100644 (file)
@@ -44,33 +44,32 @@ public:
     using AdClickAttribution = WebCore::AdClickAttribution;
     using Source = WebCore::AdClickAttribution::Source;
     using Destination = WebCore::AdClickAttribution::Destination;
-    using DestinationMap = HashMap<Destination, AdClickAttribution>;
     using Conversion = WebCore::AdClickAttribution::Conversion;
 
     AdClickAttributionManager()
         : m_firePendingConversionRequestsTimer(*this, &AdClickAttributionManager::firePendingConversionRequests)
-    , m_pingLoadFunction([](NetworkResourceLoadParameters&& params, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)>&& completionHandler) {
-        UNUSED_PARAM(params);
-        completionHandler(WebCore::ResourceError(), WebCore::ResourceResponse());
-    })
+        , m_pingLoadFunction([](NetworkResourceLoadParameters&& params, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)>&& completionHandler) {
+            UNUSED_PARAM(params);
+            completionHandler(WebCore::ResourceError(), WebCore::ResourceResponse());
+        })
     {
     }
 
-    void store(AdClickAttribution&&);
+    void storeUnconverted(AdClickAttribution&&);
     void convert(const Source&, const Destination&, Conversion&&);
     void clear(CompletionHandler<void()>&&);
     void toString(CompletionHandler<void(String)>&&) const;
     void setPingLoadFunction(Function<void(NetworkResourceLoadParameters&&, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)>&&)>&& pingLoadFunction) { m_pingLoadFunction = WTFMove(pingLoadFunction); }
     void setOverrideTimerForTesting(bool value) { m_isRunningTest = value; }
-    void setConversionURLForTesting(URL&& testURL) { m_conversionBaseURLForTesting = WTFMove(testURL); }
+    void setConversionURLForTesting(URL&&);
 
 private:
-    DestinationMap& ensureDestinationMapForSource(const Source&);
     void startTimer(Seconds);
     void fireConversionRequest(const AdClickAttribution&);
     void firePendingConversionRequests();
 
-    HashMap<Source, DestinationMap> m_adClickAttributionMap;
+    HashMap<std::pair<Source, Destination>, AdClickAttribution> m_unconvertedAdClickAttributionMap;
+    HashMap<std::pair<Source, Destination>, AdClickAttribution> m_convertedAdClickAttributionMap;
     WebCore::Timer m_firePendingConversionRequestsTimer;
     Function<void(NetworkResourceLoadParameters&&, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)>&&)> m_pingLoadFunction;
     bool m_isRunningTest { false };
index ed138c2..f706213 100644 (file)
@@ -145,7 +145,7 @@ void NetworkSession::registrableDomainsWithWebsiteData(OptionSet<WebsiteDataType
 
 void NetworkSession::storeAdClickAttribution(WebCore::AdClickAttribution&& adClickAttribution)
 {
-    m_adClickAttribution->store(WTFMove(adClickAttribution));
+    m_adClickAttribution->storeUnconverted(WTFMove(adClickAttribution));
 }
 
 void NetworkSession::convertAdClickAttribution(const WebCore::AdClickAttribution::Source& source, const WebCore::AdClickAttribution::Destination& destination, WebCore::AdClickAttribution::Conversion&& conversion)