Title: [279518] trunk
Revision
279518
Author
cdu...@apple.com
Date
2021-07-02 15:26:31 -0700 (Fri, 02 Jul 2021)

Log Message

[MacOS wk1] crypto/workers/subtle/hrsa-postMessage-worker.html is a flaky failure
https://bugs.webkit.org/show_bug.cgi?id=227540
<rdar://problem/79977662>

Reviewed by Geoffrey Garen.

Source/WebCore:

The test was sending 2 messages to the worker, the first one with the public key
and the second one for the private key. As the worker would receive those messages,
it would do some checks synchronously and post a message back to the main thread
script with the result. As we could see from the test's expected results, the
main thread script would usually (although flakily) receive the result for the
private key *before* the result for the public key, which was wrong.

The reason for this was that serializing / deserializing the crypto key would
spin the worker's run loop and the worker would thus process incoming messages
while serializing / deserializing a crypto key from a previous message.

To address the issue, we now use a BinarySemaphore to wait for the main thread
to finish the wrapping / unwrapping, instead of spinning the run loop.

No new tests, unskipped existing test.

* workers/WorkerGlobalScope.cpp:
(WebCore::WorkerGlobalScope::wrapCryptoKey):
(WebCore::WorkerGlobalScope::unwrapCryptoKey):

LayoutTests:

Rebaseline test now that the output is correct (and non-flaky). Also unmark the test as flaky.

* crypto/workers/subtle/ec-postMessage-worker-expected.txt:
* crypto/workers/subtle/hrsa-postMessage-worker-expected.txt:
* crypto/workers/subtle/hrsa-postMessage-worker.html:
* platform/mac-wk1/TestExpectations:

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (279517 => 279518)


--- trunk/LayoutTests/ChangeLog	2021-07-02 21:19:29 UTC (rev 279517)
+++ trunk/LayoutTests/ChangeLog	2021-07-02 22:26:31 UTC (rev 279518)
@@ -1,5 +1,20 @@
 2021-07-02  Chris Dumez  <cdu...@apple.com>
 
+        [MacOS wk1] crypto/workers/subtle/hrsa-postMessage-worker.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=227540
+        <rdar://problem/79977662>
+
+        Reviewed by Geoffrey Garen.
+
+        Rebaseline test now that the output is correct (and non-flaky). Also unmark the test as flaky.
+
+        * crypto/workers/subtle/ec-postMessage-worker-expected.txt:
+        * crypto/workers/subtle/hrsa-postMessage-worker-expected.txt:
+        * crypto/workers/subtle/hrsa-postMessage-worker.html:
+        * platform/mac-wk1/TestExpectations:
+
+2021-07-02  Chris Dumez  <cdu...@apple.com>
+
         REGRESSION (r279427):  [ Mac wk1 ] imported/w3c/web-platform-tests/html/rendering/replaced-elements/embedded-content/tall-cross-domain-iframe-in-scrolled.sub.html is timing out
         https://bugs.webkit.org/show_bug.cgi?id=227637
         <rdar://problem/80102396>

Modified: trunk/LayoutTests/crypto/workers/subtle/ec-postMessage-worker-expected.txt (279517 => 279518)


--- trunk/LayoutTests/crypto/workers/subtle/ec-postMessage-worker-expected.txt	2021-07-02 21:19:29 UTC (rev 279517)
+++ trunk/LayoutTests/crypto/workers/subtle/ec-postMessage-worker-expected.txt	2021-07-02 22:26:31 UTC (rev 279518)
@@ -3,6 +3,14 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
+PASS All checks passed in worker for public key
+PASS publicKey.type is 'public'
+PASS publicKey.extractable is true
+PASS publicKey.algorithm.name is 'ECDH'
+PASS publicKey.algorithm.namedCurve is 'P-256'
+PASS publicKey.usages is [ ]
+PASS exportedKey.x is publicKeyJSON.x
+PASS exportedKey.y is publicKeyJSON.y
 PASS All checks passed in worker for private key
 PASS privateKey.type is 'private'
 PASS privateKey.extractable is true
@@ -12,14 +20,6 @@
 PASS exportedKey.x is privateKeyJSON.x
 PASS exportedKey.y is privateKeyJSON.y
 PASS exportedKey.d is privateKeyJSON.d
-PASS All checks passed in worker for public key
-PASS publicKey.type is 'public'
-PASS publicKey.extractable is true
-PASS publicKey.algorithm.name is 'ECDH'
-PASS publicKey.algorithm.namedCurve is 'P-256'
-PASS publicKey.usages is [ ]
-PASS exportedKey.x is publicKeyJSON.x
-PASS exportedKey.y is publicKeyJSON.y
 PASS successfullyParsed is true
 
 TEST COMPLETE

Modified: trunk/LayoutTests/crypto/workers/subtle/hrsa-postMessage-worker-expected.txt (279517 => 279518)


--- trunk/LayoutTests/crypto/workers/subtle/hrsa-postMessage-worker-expected.txt	2021-07-02 21:19:29 UTC (rev 279517)
+++ trunk/LayoutTests/crypto/workers/subtle/hrsa-postMessage-worker-expected.txt	2021-07-02 22:26:31 UTC (rev 279518)
@@ -3,6 +3,14 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
+PASS All checks passed in worker for public key
+PASS publicKey.type is 'public'
+PASS publicKey.extractable is true
+PASS publicKey.algorithm.name is 'RSASSA-PKCS1-v1_5'
+PASS publicKey.algorithm.modulusLength is 2048
+PASS bytesToHexString(publicKey.algorithm.publicExponent) is '010001'
+PASS publicKey.algorithm.hash.name is 'SHA-256'
+PASS publicKey.usages is ['verify']
 PASS All checks passed in worker for private key
 PASS privateKey.type is 'private'
 PASS privateKey.extractable is false
@@ -11,14 +19,6 @@
 PASS bytesToHexString(privateKey.algorithm.publicExponent) is '010001'
 PASS privateKey.algorithm.hash.name is 'SHA-256'
 PASS privateKey.usages is ['sign']
-PASS All checks passed in worker for public key
-PASS publicKey.type is 'public'
-PASS publicKey.extractable is true
-PASS publicKey.algorithm.name is 'RSASSA-PKCS1-v1_5'
-PASS publicKey.algorithm.modulusLength is 2048
-PASS bytesToHexString(publicKey.algorithm.publicExponent) is '010001'
-PASS publicKey.algorithm.hash.name is 'SHA-256'
-PASS publicKey.usages is ['verify']
 PASS successfullyParsed is true
 
 TEST COMPLETE

Modified: trunk/LayoutTests/crypto/workers/subtle/hrsa-postMessage-worker.html (279517 => 279518)


--- trunk/LayoutTests/crypto/workers/subtle/hrsa-postMessage-worker.html	2021-07-02 21:19:29 UTC (rev 279517)
+++ trunk/LayoutTests/crypto/workers/subtle/hrsa-postMessage-worker.html	2021-07-02 22:26:31 UTC (rev 279518)
@@ -48,6 +48,8 @@
         finishJSTest();
     } else {
         if (publicKey = evt.data.publicKey) {
+            if (count)
+                testFailed("Public key should have been received first");
             testPassed("All checks passed in worker for public key");
             shouldBe("publicKey.type", "'public'");
             shouldBe("publicKey.extractable", "true");
@@ -57,6 +59,8 @@
             shouldBe("publicKey.algorithm.hash.name", "'SHA-256'");
             shouldBe("publicKey.usages", "['verify']");
         } else if (privateKey = evt.data.privateKey) {
+            if (!count)
+                testFailed("Private key should have been received second");
             testPassed("All checks passed in worker for private key");
             shouldBe("privateKey.type", "'private'");
             shouldBe("privateKey.extractable", "false");

Modified: trunk/LayoutTests/crypto/workers/subtle/rsa-postMessage-worker-expected.txt (279517 => 279518)


--- trunk/LayoutTests/crypto/workers/subtle/rsa-postMessage-worker-expected.txt	2021-07-02 21:19:29 UTC (rev 279517)
+++ trunk/LayoutTests/crypto/workers/subtle/rsa-postMessage-worker-expected.txt	2021-07-02 22:26:31 UTC (rev 279518)
@@ -3,6 +3,16 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
+PASS All checks passed in worker for public key
+PASS publicKey.type is 'public'
+PASS publicKey.extractable is true
+PASS publicKey.algorithm.name is 'RSAES-PKCS1-v1_5'
+PASS publicKey.algorithm.modulusLength is 2048
+PASS bytesToHexString(publicKey.algorithm.publicExponent) is '010001'
+PASS publicKey.algorithm.hash is undefined.
+PASS publicKey.usages is ['encrypt']
+PASS exportedKey.n is publicKeyJSON.n
+PASS exportedKey.e is publicKeyJSON.e
 PASS All checks passed in worker for private key
 PASS privateKey.type is 'private'
 PASS privateKey.extractable is true
@@ -19,16 +29,6 @@
 PASS exportedKey.dp is privateKeyJSON.dp
 PASS exportedKey.dq is privateKeyJSON.dq
 PASS exportedKey.qi is privateKeyJSON.qi
-PASS All checks passed in worker for public key
-PASS publicKey.type is 'public'
-PASS publicKey.extractable is true
-PASS publicKey.algorithm.name is 'RSAES-PKCS1-v1_5'
-PASS publicKey.algorithm.modulusLength is 2048
-PASS bytesToHexString(publicKey.algorithm.publicExponent) is '010001'
-PASS publicKey.algorithm.hash is undefined.
-PASS publicKey.usages is ['encrypt']
-PASS exportedKey.n is publicKeyJSON.n
-PASS exportedKey.e is publicKeyJSON.e
 PASS successfullyParsed is true
 
 TEST COMPLETE

Modified: trunk/LayoutTests/platform/mac-wk1/TestExpectations (279517 => 279518)


--- trunk/LayoutTests/platform/mac-wk1/TestExpectations	2021-07-02 21:19:29 UTC (rev 279517)
+++ trunk/LayoutTests/platform/mac-wk1/TestExpectations	2021-07-02 22:26:31 UTC (rev 279518)
@@ -1311,5 +1311,3 @@
 webkit.org/b/227366 [ BigSur ] http/tests/preload/onload_event.html [ Pass Timeout ]
 
 webkit.org/b/227516 fast/shadow-dom/style-resolver-sharing.html [ Pass Failure ]
-
-webkit.org/b/227540 [ Debug ] crypto/workers/subtle/hrsa-postMessage-worker.html [ Pass Failure ]

Modified: trunk/Source/WebCore/ChangeLog (279517 => 279518)


--- trunk/Source/WebCore/ChangeLog	2021-07-02 21:19:29 UTC (rev 279517)
+++ trunk/Source/WebCore/ChangeLog	2021-07-02 22:26:31 UTC (rev 279518)
@@ -1,3 +1,31 @@
+2021-07-02  Chris Dumez  <cdu...@apple.com>
+
+        [MacOS wk1] crypto/workers/subtle/hrsa-postMessage-worker.html is a flaky failure
+        https://bugs.webkit.org/show_bug.cgi?id=227540
+        <rdar://problem/79977662>
+
+        Reviewed by Geoffrey Garen.
+
+        The test was sending 2 messages to the worker, the first one with the public key
+        and the second one for the private key. As the worker would receive those messages,
+        it would do some checks synchronously and post a message back to the main thread
+        script with the result. As we could see from the test's expected results, the
+        main thread script would usually (although flakily) receive the result for the
+        private key *before* the result for the public key, which was wrong.
+
+        The reason for this was that serializing / deserializing the crypto key would
+        spin the worker's run loop and the worker would thus process incoming messages
+        while serializing / deserializing a crypto key from a previous message.
+
+        To address the issue, we now use a BinarySemaphore to wait for the main thread
+        to finish the wrapping / unwrapping, instead of spinning the run loop.
+
+        No new tests, unskipped existing test.
+
+        * workers/WorkerGlobalScope.cpp:
+        (WebCore::WorkerGlobalScope::wrapCryptoKey):
+        (WebCore::WorkerGlobalScope::unwrapCryptoKey):
+
 2021-07-02  Alan Bujtas  <za...@apple.com>
 
         [LFC][TFC] Keep track of both the fixed and percent maximum values for each column

Modified: trunk/Source/WebCore/workers/WorkerGlobalScope.cpp (279517 => 279518)


--- trunk/Source/WebCore/workers/WorkerGlobalScope.cpp	2021-07-02 21:19:29 UTC (rev 279517)
+++ trunk/Source/WebCore/workers/WorkerGlobalScope.cpp	2021-07-02 22:26:31 UTC (rev 279518)
@@ -59,6 +59,7 @@
 #include <_javascript_Core/ScriptCallStack.h>
 #include <wtf/IsoMallocInlines.h>
 #include <wtf/Lock.h>
+#include <wtf/threads/BinarySemaphore.h>
 
 namespace WebCore {
 using namespace Inspector;
@@ -390,69 +391,30 @@
 
 #if ENABLE(WEB_CRYPTO)
 
-class CryptoBufferContainer : public ThreadSafeRefCounted<CryptoBufferContainer> {
-public:
-    static Ref<CryptoBufferContainer> create() { return adoptRef(*new CryptoBufferContainer); }
-    Vector<uint8_t>& buffer() { return m_buffer; }
-
-private:
-    Vector<uint8_t> m_buffer;
-};
-
-class CryptoBooleanContainer : public ThreadSafeRefCounted<CryptoBooleanContainer> {
-public:
-    static Ref<CryptoBooleanContainer> create() { return adoptRef(*new CryptoBooleanContainer); }
-    bool boolean() const { return m_boolean; }
-    void setBoolean(bool boolean) { m_boolean = boolean; }
-
-private:
-    std::atomic<bool> m_boolean { false };
-};
-
 bool WorkerGlobalScope::wrapCryptoKey(const Vector<uint8_t>& key, Vector<uint8_t>& wrappedKey)
 {
-    Ref<WorkerGlobalScope> protectedThis(*this);
-    auto resultContainer = CryptoBooleanContainer::create();
-    auto doneContainer = CryptoBooleanContainer::create();
-    auto wrappedKeyContainer = CryptoBufferContainer::create();
-    thread().workerLoaderProxy().postTaskToLoader([resultContainer, key, wrappedKeyContainer, doneContainer, workerMessagingProxy = makeRef(downcast<WorkerMessagingProxy>(thread().workerLoaderProxy()))](ScriptExecutionContext& context) {
-        resultContainer->setBoolean(context.wrapCryptoKey(key, wrappedKeyContainer->buffer()));
-        doneContainer->setBoolean(true);
-        workerMessagingProxy->postTaskForModeToWorkerOrWorkletGlobalScope([](ScriptExecutionContext& context) {
-            ASSERT_UNUSED(context, context.isWorkerGlobalScope());
-        }, WorkerRunLoop::defaultMode());
+    Ref protectedThis { *this };
+    bool success = false;
+    BinarySemaphore semaphore;
+    thread().workerLoaderProxy().postTaskToLoader([&semaphore, &success, &key, &wrappedKey](auto& context) {
+        success = context.wrapCryptoKey(key, wrappedKey);
+        semaphore.signal();
     });
-
-    auto waitResult = MessageQueueMessageReceived;
-    while (!doneContainer->boolean() && waitResult != MessageQueueTerminated)
-        waitResult = thread().runLoop().runInMode(this, WorkerRunLoop::defaultMode());
-
-    if (doneContainer->boolean())
-        wrappedKey.swap(wrappedKeyContainer->buffer());
-    return resultContainer->boolean();
+    semaphore.wait();
+    return success;
 }
 
 bool WorkerGlobalScope::unwrapCryptoKey(const Vector<uint8_t>& wrappedKey, Vector<uint8_t>& key)
 {
-    Ref<WorkerGlobalScope> protectedThis(*this);
-    auto resultContainer = CryptoBooleanContainer::create();
-    auto doneContainer = CryptoBooleanContainer::create();
-    auto keyContainer = CryptoBufferContainer::create();
-    thread().workerLoaderProxy().postTaskToLoader([resultContainer, wrappedKey, keyContainer, doneContainer, workerMessagingProxy = makeRef(downcast<WorkerMessagingProxy>(thread().workerLoaderProxy()))](ScriptExecutionContext& context) {
-        resultContainer->setBoolean(context.unwrapCryptoKey(wrappedKey, keyContainer->buffer()));
-        doneContainer->setBoolean(true);
-        workerMessagingProxy->postTaskForModeToWorkerOrWorkletGlobalScope([](ScriptExecutionContext& context) {
-            ASSERT_UNUSED(context, context.isWorkerGlobalScope());
-        }, WorkerRunLoop::defaultMode());
+    Ref protectedThis { *this };
+    bool success = false;
+    BinarySemaphore semaphore;
+    thread().workerLoaderProxy().postTaskToLoader([&semaphore, &success, &key, &wrappedKey](auto& context) {
+        success = context.unwrapCryptoKey(wrappedKey, key);
+        semaphore.signal();
     });
-
-    auto waitResult = MessageQueueMessageReceived;
-    while (!doneContainer->boolean() && waitResult != MessageQueueTerminated)
-        waitResult = thread().runLoop().runInMode(this, WorkerRunLoop::defaultMode());
-
-    if (doneContainer->boolean())
-        key.swap(keyContainer->buffer());
-    return resultContainer->boolean();
+    semaphore.wait();
+    return success;
 }
 
 #endif // ENABLE(WEB_CRYPTO)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to