From 2b0ddf868435bad0894269acd458fcfe4fae99cb Mon Sep 17 00:00:00 2001 From: Kagami Sascha Rosylight Date: Tue, 7 Jan 2025 18:32:23 +0000 Subject: [PATCH] Bug 1933826 - Commit pull-into descriptors after filling from queue r=arai,smaug This splits ReadableByteStreamControllerCommitPullIntoDescriptor from ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue so that close/chunk steps will run only after processing all descriptors, thus preventing user JS from potentially modifying anything during the process. Corresponding to https://github.com/whatwg/streams/pull/1326 with some cleanup in https://github.com/whatwg/streams/pull/1336. Differential Revision: https://phabricator.services.mozilla.com/D232663 --- dom/streams/ReadableByteStreamController.cpp | 248 +++++++++++++----- .../patched-global.any.js.ini | 53 +--- .../streams/readable-streams/global.html.ini | 3 - .../read-task-handling.window.js.ini | 5 - .../transfer-with-messageport.window.js.ini | 6 - 5 files changed, 194 insertions(+), 121 deletions(-) delete mode 100644 testing/web-platform/meta/streams/readable-streams/global.html.ini delete mode 100644 testing/web-platform/meta/streams/readable-streams/read-task-handling.window.js.ini delete mode 100644 testing/web-platform/meta/streams/transferable/transfer-with-messageport.window.js.ini diff --git a/dom/streams/ReadableByteStreamController.cpp b/dom/streams/ReadableByteStreamController.cpp index a440331c323b..b6ceaa68b081 100644 --- a/dom/streams/ReadableByteStreamController.cpp +++ b/dom/streams/ReadableByteStreamController.cpp @@ -751,22 +751,25 @@ void ReadableByteStreamControllerCommitPullIntoDescriptor( MOZ_CAN_RUN_SCRIPT void ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue( JSContext* aCx, ReadableByteStreamController* aController, - ErrorResult& aRv) { + nsTArray>& aFilledPullIntos, ErrorResult& aRv) { // Step 1. Assert: controller.[[closeRequested]] is false. MOZ_ASSERT(!aController->CloseRequested()); - // Step 2. While controller.[[pendingPullIntos]] is not empty, + // Step 2. Let filledPullIntos be a new empty list. + MOZ_ASSERT(aFilledPullIntos.IsEmpty()); + + // Step 3. While controller.[[pendingPullIntos]] is not empty, while (!aController->PendingPullIntos().isEmpty()) { - // Step 2.1 If controller.[[queueTotalSize]] is 0, return. + // Step 3.1. If controller.[[queueTotalSize]] is 0, then break. if (aController->QueueTotalSize() == 0) { - return; + break; } - // Step 2.2. Let pullIntoDescriptor be controller.[[pendingPullIntos]][0]. + // Step 3.2. Let pullIntoDescriptor be controller.[[pendingPullIntos]][0]. RefPtr pullIntoDescriptor = aController->PendingPullIntos().getFirst(); - // Step 2.3. If + // Step 3.3. If // !ReadableByteStreamControllerFillPullIntoDescriptorFromQueue(controller, // pullIntoDescriptor) is true, bool ready = ReadableByteStreamControllerFillPullIntoDescriptorFromQueue( @@ -776,22 +779,17 @@ void ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue( } if (ready) { - // Step 2.3.1. Perform + // Step 3.3.1. Perform // !ReadableByteStreamControllerShiftPendingPullInto(controller). RefPtr discardedPullIntoDescriptor = ReadableByteStreamControllerShiftPendingPullInto(aController); - // Step 2.3.2. Perform - // !ReadableByteStreamControllerCommitPullIntoDescriptor(controller.[[stream]], - // pullIntoDescriptor). - RefPtr stream(aController->Stream()); - ReadableByteStreamControllerCommitPullIntoDescriptor( - aCx, stream, pullIntoDescriptor, aRv); - if (aRv.Failed()) { - return; - } + // Step 3.3.2. Append pullIntoDescriptor to filledPullIntos. + aFilledPullIntos.AppendElement(pullIntoDescriptor); } } + + // Step 4: Return filledPullIntos. (Implicit) } MOZ_CAN_RUN_SCRIPT @@ -1026,14 +1024,27 @@ void ReadableByteStreamControllerEnqueue( ReadableByteStreamControllerEnqueueChunkToQueue( aController, transferredBuffer, byteOffset, byteLength); - // Step 10.2 Perform ! + // Step 10.2. Let filledPullIntos be the result of performing ! // ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller). + nsTArray> filledPullIntos; ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue( - aCx, aController, aRv); + aCx, aController, filledPullIntos, aRv); if (aRv.Failed()) { return; } + // Step 10.3. For each filledPullInto of filledPullIntos, + for (auto& filledPullInto : filledPullIntos) { + // Step 10.3.1. Perform ! + // ReadableByteStreamControllerCommitPullIntoDescriptor(stream, + // filledPullInto). + ReadableByteStreamControllerCommitPullIntoDescriptor( + aCx, stream, MOZ_KnownLive(filledPullInto), aRv); + if (aRv.Failed()) { + return; + } + } + // Step 11. Otherwise, } else { // Step 11.1. Assert: ! IsReadableStreamLocked(stream) is false. @@ -1352,18 +1363,32 @@ static void ReadableByteStreamControllerRespondInClosedState( // Step 4. If ! ReadableStreamHasBYOBReader(stream) is true, if (ReadableStreamHasBYOBReader(stream)) { - // Step 4.1. While ! ReadableStreamGetNumReadIntoRequests(stream) > 0, - while (ReadableStreamGetNumReadIntoRequests(stream) > 0) { - // Step 4.1.1. Let pullIntoDescriptor be ! + // Step 4.1. Let filledPullIntos be a new empty list. + nsTArray> filledPullIntos; + + // Step 4.2. While |filledPullInto|'s [=list/size=] < ! + // ReadableStreamGetNumReadIntoRequests(stream), + while (filledPullIntos.Length() < + ReadableStreamGetNumReadIntoRequests(stream)) { + // Step 4.2.1. Let pullIntoDescriptor be ! // ReadableByteStreamControllerShiftPendingPullInto(controller). RefPtr pullIntoDescriptor = ReadableByteStreamControllerShiftPendingPullInto(aController); - // Step 4.1.2. Perform ! + // Step 4.2.2. Append pullIntoDescriptor to filledPullIntos. + filledPullIntos.AppendElement(pullIntoDescriptor); + } + + // Step 4.3. For each filledPullInto of filledPullIntos, + for (auto& filledPullInto : filledPullIntos) { + // Step 4.3.1. Perform ! // ReadableByteStreamControllerCommitPullIntoDescriptor(stream, - // pullIntoDescriptor). + // filledPullInto). ReadableByteStreamControllerCommitPullIntoDescriptor( - aCx, stream, pullIntoDescriptor, aRv); + aCx, stream, MOZ_KnownLive(filledPullInto), aRv); + if (aRv.Failed()) { + return; + } } } } @@ -1413,12 +1438,29 @@ static void ReadableByteStreamControllerRespondInReadableState( return; } - // Step 3.2. Perform ! + // Step 3.2. Let filledPullIntos be the result of performing ! // ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller). + nsTArray> filledPullIntos; ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue( - aCx, aController, aRv); + aCx, aController, filledPullIntos, aRv); + if (aRv.Failed()) { + return; + } - // Step 3.3. Return. + // Step 3.3. For each filledPullInto of filledPullIntos, + for (auto& filledPullInto : filledPullIntos) { + // Step 3.3.1. Perform ! + // ReadableByteStreamControllerCommitPullIntoDescriptor(controller.[[stream]], + // filledPullInto). + ReadableByteStreamControllerCommitPullIntoDescriptor( + aCx, MOZ_KnownLive(aController->Stream()), + MOZ_KnownLive(filledPullInto), aRv); + if (aRv.Failed()) { + return; + } + } + + // Step 3.4. Return. return; } @@ -1463,7 +1505,16 @@ static void ReadableByteStreamControllerRespondInReadableState( aPullIntoDescriptor->SetBytesFilled(aPullIntoDescriptor->BytesFilled() - remainderSize); - // Step 9. Perform + // Step 9. Let filledPullIntos be the result of performing ! + // ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller). + nsTArray> filledPullIntos; + ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue( + aCx, aController, filledPullIntos, aRv); + if (aRv.Failed()) { + return; + } + + // Step 10. Perform // !ReadableByteStreamControllerCommitPullIntoDescriptor(controller.[[stream]], // pullIntoDescriptor). RefPtr stream(aController->Stream()); @@ -1473,10 +1524,17 @@ static void ReadableByteStreamControllerRespondInReadableState( return; } - // Step 10. Perform - // !ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller). - ReadableByteStreamControllerProcessPullIntoDescriptorsUsingQueue( - aCx, aController, aRv); + // Step 11. For each filledPullInto of filledPullIntos, + for (auto& filledPullInto : filledPullIntos) { + // Step 11.1. Perform ! + // ReadableByteStreamControllerCommitPullIntoDescriptor(controller.[[stream]], + // filledPullInto). + ReadableByteStreamControllerCommitPullIntoDescriptor( + aCx, stream, MOZ_KnownLive(filledPullInto), aRv); + if (aRv.Failed()) { + return; + } + } } // https://streams.spec.whatwg.org/#readable-byte-stream-controller-respond-internal @@ -1669,6 +1727,60 @@ void ReadableByteStreamControllerRespondWithNewView( aRv); } +#ifdef DEBUG +// https://streams.spec.whatwg.org/#abstract-opdef-cancopydatablockbytes +bool CanCopyDataBlockBytes(JS::Handle aToBuffer, size_t aToIndex, + JS::Handle aFromBuffer, size_t aFromIndex, + size_t aCount) { + // Step 1. Assert: toBuffer is an Object. + // Step 2. Assert: toBuffer has an [[ArrayBufferData]] internal slot. + MOZ_ASSERT(JS::IsArrayBufferObject(aToBuffer)); + + // Step 3. Assert: fromBuffer is an Object. + // Step 4. Assert: fromBuffer has an [[ArrayBufferData]] internal slot. + MOZ_ASSERT(JS::IsArrayBufferObject(aFromBuffer)); + + // Step 5. If toBuffer is fromBuffer, return false. + // Note: JS API makes it safe to just compare the pointers. + if (aToBuffer == aFromBuffer) { + return false; + } + + // Step 6. If ! IsDetachedBuffer(toBuffer) is true, return false. + if (JS::IsDetachedArrayBufferObject(aToBuffer)) { + return false; + } + + // Step 7. If ! IsDetachedBuffer(fromBuffer) is true, return false. + if (JS::IsDetachedArrayBufferObject(aFromBuffer)) { + return false; + } + + // Step 8. If toIndex + count > toBuffer.[[ArrayBufferByteLength]], return + // false. + if (aToIndex + aCount > JS::GetArrayBufferByteLength(aToBuffer)) { + return false; + } + // (Not in the spec) also check overflow. + if (aToIndex + aCount < aToIndex) { + return false; + } + + // Step 9. If fromIndex + count > fromBuffer.[[ArrayBufferByteLength]], return + // false. + if (aFromIndex + aCount > JS::GetArrayBufferByteLength(aFromBuffer)) { + return false; + } + // (Not in the spec) also check overflow. + if (aFromIndex + aCount < aFromIndex) { + return false; + } + + // Step 10. Return true. + return true; +} +#endif + // https://streams.spec.whatwg.org/#readable-byte-stream-controller-fill-pull-into-descriptor-from-queue bool ReadableByteStreamControllerFillPullIntoDescriptorFromQueue( JSContext* aCx, ReadableByteStreamController* aController, @@ -1690,105 +1802,119 @@ bool ReadableByteStreamControllerFillPullIntoDescriptorFromQueue( // Step 4. Let ready be false. bool ready = false; - // Step 5. Assert: pullIntoDescriptor’s bytes filled < pullIntoDescriptor’s + // Step 5. Assert: ! IsDetachedBuffer(pullIntoDescriptor ’s buffer) is false. + MOZ_ASSERT(!JS::IsDetachedArrayBufferObject(aPullIntoDescriptor->Buffer())); + + // Step 6. Assert: pullIntoDescriptor’s bytes filled < pullIntoDescriptor’s // minimum fill. MOZ_ASSERT(aPullIntoDescriptor->BytesFilled() < aPullIntoDescriptor->MinimumFill()); - // Step 6. Let remainderBytes be the remainder after dividing maxBytesFilled + // Step 7. Let remainderBytes be the remainder after dividing maxBytesFilled // by pullIntoDescriptor’s element size. size_t remainderBytes = maxBytesFilled % aPullIntoDescriptor->ElementSize(); - // Step 7. Let maxAlignedBytes be maxBytesFilled − remainderBytes. + // Step 8. Let maxAlignedBytes be maxBytesFilled − remainderBytes. size_t maxAlignedBytes = maxBytesFilled - remainderBytes; - // Step 8. If maxAlignedBytes ≥ pullIntoDescriptor’s minimum fill, + // Step 9. If maxAlignedBytes ≥ pullIntoDescriptor’s minimum fill, if (maxAlignedBytes >= aPullIntoDescriptor->MinimumFill()) { - // Step 8.1. Set totalBytesToCopyRemaining to maxAlignedBytes − + // Step 9.1. Set totalBytesToCopyRemaining to maxAlignedBytes − // pullIntoDescriptor’s bytes filled. totalBytesToCopyRemaining = maxAlignedBytes - aPullIntoDescriptor->BytesFilled(); - // Step 8.2. Set ready to true. + // Step 9.2. Set ready to true. ready = true; } - // Step 9. Let queue be controller.[[queue]]. + // Step 10. Let queue be controller.[[queue]]. LinkedList>& queue = aController->Queue(); - // Step 10. While totalBytesToCopyRemaining > 0, + // Step 11. While totalBytesToCopyRemaining > 0, while (totalBytesToCopyRemaining > 0) { - // Step 10.1 Let headOfQueue be queue[0]. + // Step 11.1 Let headOfQueue be queue[0]. ReadableByteStreamQueueEntry* headOfQueue = queue.getFirst(); - // Step 10.2. Let bytesToCopy be min(totalBytesToCopyRemaining, + // Step 11.2. Let bytesToCopy be min(totalBytesToCopyRemaining, // headOfQueue’s byte length). size_t bytesToCopy = std::min(totalBytesToCopyRemaining, headOfQueue->ByteLength()); - // Step 10.3. Let destStart be pullIntoDescriptor’s byte offset + + // Step 11.3. Let destStart be pullIntoDescriptor’s byte offset + // pullIntoDescriptor’s bytes filled. size_t destStart = aPullIntoDescriptor->ByteOffset() + aPullIntoDescriptor->BytesFilled(); - // Step 10.4. Perform !CopyDataBlockBytes(pullIntoDescriptor’s - // buffer.[[ArrayBufferData]], destStart, headOfQueue’s - // buffer.[[ArrayBufferData]], headOfQueue’s byte offset, - // bytesToCopy). + // Step 11.4. Let descriptorBuffer be pullIntoDescriptor’s buffer. JS::Rooted descriptorBuffer(aCx, aPullIntoDescriptor->Buffer()); + + // Step 11.5. Let queueBuffer be headOfQueue’s buffer. JS::Rooted queueBuffer(aCx, headOfQueue->Buffer()); + + // Step 11.6. Let queueByteOffset be headOfQueue’s byte offset. + size_t queueByteOffset = headOfQueue->ByteOffset(); + + // Step 11.7. Assert: ! CanCopyDataBlockBytes(descriptorBuffer, destStart, + // queueBuffer, queueByteOffset, bytesToCopy) is true. + MOZ_ASSERT(CanCopyDataBlockBytes(descriptorBuffer, destStart, queueBuffer, + queueByteOffset, bytesToCopy)); + + // Step 11.8. Perform ! + // CopyDataBlockBytes(descriptorBuffer.[[ArrayBufferData]], destStart, + // queueBuffer.[[ArrayBufferData]], queueByteOffset, bytesToCopy). if (!JS::ArrayBufferCopyData(aCx, descriptorBuffer, destStart, queueBuffer, - headOfQueue->ByteOffset(), bytesToCopy)) { + queueByteOffset, bytesToCopy)) { aRv.StealExceptionFromJSContext(aCx); return false; } - // Step 10.5. If headOfQueue’s byte length is bytesToCopy, + // Step 11.9. If headOfQueue’s byte length is bytesToCopy, if (headOfQueue->ByteLength() == bytesToCopy) { - // Step 10.5.1. Remove queue[0]. + // Step 11.9.1. Remove queue[0]. queue.popFirst(); } else { - // Step 10.6. Otherwise, + // Step 11.10. Otherwise, - // Step 10.6.1 Set headOfQueue’s byte offset to + // Step 11.10.1 Set headOfQueue’s byte offset to // headOfQueue’s byte offset + bytesToCopy. headOfQueue->SetByteOffset(headOfQueue->ByteOffset() + bytesToCopy); - // Step 10.6.2 Set headOfQueue’s byte length to + // Step 11.10.2 Set headOfQueue’s byte length to // headOfQueue’s byte length − bytesToCopy. headOfQueue->SetByteLength(headOfQueue->ByteLength() - bytesToCopy); } - // Step 10.7. Set controller.[[queueTotalSize]] to + // Step 11.11. Set controller.[[queueTotalSize]] to // controller.[[queueTotalSize]] − bytesToCopy. aController->SetQueueTotalSize(aController->QueueTotalSize() - (double)bytesToCopy); - // Step 10.8, Perform + // Step 11.12, Perform // !ReadableByteStreamControllerFillHeadPullIntoDescriptor(controller, // bytesToCopy, pullIntoDescriptor). ReadableByteStreamControllerFillHeadPullIntoDescriptor( aController, bytesToCopy, aPullIntoDescriptor); - // Step 10.9. Set totalBytesToCopyRemaining to totalBytesToCopyRemaining − + // Step 11.13. Set totalBytesToCopyRemaining to totalBytesToCopyRemaining − // bytesToCopy. totalBytesToCopyRemaining = totalBytesToCopyRemaining - bytesToCopy; } - // Step 11. If ready is false, + // Step 12. If ready is false, if (!ready) { - // Step 11.1. Assert: controller.[[queueTotalSize]] is 0. + // Step 12.1. Assert: controller.[[queueTotalSize]] is 0. MOZ_ASSERT(aController->QueueTotalSize() == 0); - // Step 11.2. Assert: pullIntoDescriptor’s bytes filled > 0. + // Step 12.2. Assert: pullIntoDescriptor’s bytes filled > 0. MOZ_ASSERT(aPullIntoDescriptor->BytesFilled() > 0); - // Step 11.3. Assert: pullIntoDescriptor’s bytes filled < + // Step 12.3. Assert: pullIntoDescriptor’s bytes filled < // pullIntoDescriptor’s minimum fill. MOZ_ASSERT(aPullIntoDescriptor->BytesFilled() < aPullIntoDescriptor->MinimumFill()); } - // Step 12. Return ready. + // Step 13. Return ready. return ready; } diff --git a/testing/web-platform/meta/streams/readable-byte-streams/patched-global.any.js.ini b/testing/web-platform/meta/streams/readable-byte-streams/patched-global.any.js.ini index 28b129c849d5..3aca9a59ce28 100644 --- a/testing/web-platform/meta/streams/readable-byte-streams/patched-global.any.js.ini +++ b/testing/web-platform/meta/streams/readable-byte-streams/patched-global.any.js.ini @@ -1,59 +1,20 @@ [patched-global.any.shadowrealm.html] - expected: [ERROR, CRASH] - -[patched-global.any.html] - expected: - if debug: CRASH - [Patched then() sees byobRequest after filling all pending pull-into descriptors] - expected: FAIL - - -[patched-global.any.sharedworker.html] - expected: - if debug: CRASH - [Patched then() sees byobRequest after filling all pending pull-into descriptors] - expected: FAIL - - -[patched-global.any.serviceworker.html] - expected: - if debug: CRASH - if not debug: [OK, ERROR, CRASH] - [ERROR, CRASH] - [Patched then() sees byobRequest after filling all pending pull-into descriptors] - expected: FAIL - - -[patched-global.any.worker.html] - expected: [OK, ERROR, CRASH] - [Patched then() sees byobRequest after filling all pending pull-into descriptors] - expected: FAIL - + expected: ERROR [patched-global.any.shadowrealm-in-window.html] - expected: - if debug and not sessionHistoryInParent: [CRASH, ERROR] - [ERROR, CRASH] + expected: ERROR [patched-global.any.shadowrealm-in-dedicatedworker.html] - expected: - if debug and (os == "mac"): [CRASH, ERROR] - [ERROR, CRASH] + expected: ERROR [patched-global.https.any.shadowrealm-in-audioworklet.html] - expected: [ERROR, CRASH] + expected: ERROR [patched-global.any.shadowrealm-in-shadowrealm.html] - expected: [ERROR, CRASH] + expected: ERROR [patched-global.any.shadowrealm-in-sharedworker.html] - expected: - if debug and (os == "linux") and not fission: [CRASH, ERROR] - if debug and (os == "android") and sessionHistoryInParent: [CRASH, ERROR] - [ERROR, CRASH] + expected: ERROR [patched-global.https.any.shadowrealm-in-serviceworker.html] - expected: - if (os == "win") and debug and (processor == "x86"): [ERROR, CRASH, TIMEOUT] - if (os == "mac") and debug: [ERROR, TIMEOUT, CRASH] - [ERROR, TIMEOUT] + expected: ERROR diff --git a/testing/web-platform/meta/streams/readable-streams/global.html.ini b/testing/web-platform/meta/streams/readable-streams/global.html.ini deleted file mode 100644 index 4449f665e9ed..000000000000 --- a/testing/web-platform/meta/streams/readable-streams/global.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[global.html] - expected: - if (os == "win") and debug and (processor == "x86_64"): CRASH diff --git a/testing/web-platform/meta/streams/readable-streams/read-task-handling.window.js.ini b/testing/web-platform/meta/streams/readable-streams/read-task-handling.window.js.ini deleted file mode 100644 index 61bb4e72f9df..000000000000 --- a/testing/web-platform/meta/streams/readable-streams/read-task-handling.window.js.ini +++ /dev/null @@ -1,5 +0,0 @@ -[read-task-handling.window.html] - expected: - if debug and (os == "win") and (processor == "x86_64"): CRASH - if debug and (os == "linux") and fission: CRASH - if debug and (os == "android"): CRASH diff --git a/testing/web-platform/meta/streams/transferable/transfer-with-messageport.window.js.ini b/testing/web-platform/meta/streams/transferable/transfer-with-messageport.window.js.ini deleted file mode 100644 index 400e8bfc4096..000000000000 --- a/testing/web-platform/meta/streams/transferable/transfer-with-messageport.window.js.ini +++ /dev/null @@ -1,6 +0,0 @@ -[transfer-with-messageport.window.html] - expected: - if not debug and not asan and (os == "mac"): [OK, CRASH] - if not debug and not asan and (os == "android"): [CRASH, OK] - if not debug and asan: CRASH - if debug: CRASH