Reland "Implement copy-on-transfer semantics for GPUMappedArrayBuffer"

This reverts commit 7146831. Reason for revert: Fixed instead by https://chromium-review.googlesource.com/c/chromium/src/+/3499755 Original change's description: > Revert "Implement copy-on-transfer semantics for GPUMappedArrayBuffer" > > This reverts commit 2d525d2. > > Reason for revert: WebGPUExpectations update needs to re-run generator. > > Original change's description: > > Implement copy-on-transfer semantics for GPUMappedArrayBuffer > > > > See gpuweb/gpuweb#2072 (comment) > > Passes updated tests in gpuweb/cts#1027 > > > > Bug: 1243842 > > Change-Id: I3024b0185b4bdff3daf2ed32ab0f7286a4dbd1ff > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3496685 > > Reviewed-by: Kentaro Hara <haraken@chromium.org> > > Reviewed-by: Yuki Shiino <yukishiino@chromium.org> > > Reviewed-by: Kai Ninomiya <kainino@chromium.org> > > Commit-Queue: Austin Eng <enga@chromium.org> > > Cr-Commit-Position: refs/heads/main@{#976902} > > Bug: 1243842 > Change-Id: I20459fce5efe4987040f5e3dd9cc1b4338acd27f > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3501104 > Auto-Submit: Austin Eng <enga@chromium.org> > Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> > Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org> > Owners-Override: Nidhi Jaju <nidhijaju@chromium.org> > Cr-Commit-Position: refs/heads/main@{#976943} Bug: 1243842 Change-Id: Ie124d05f59c330f8f5a4b1ba223d9608bc786cce Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3500883 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Austin Eng <enga@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org> Cr-Commit-Position: refs/heads/main@{#976959}
chromium · Mar 3, 2022 · 68365a9 · 68365a9
1 parent d167ed7
commit 68365a9
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 20 deletions.
diff --git a/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.h b/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.h
@@ -64,7 +64,7 @@ class CORE_EXPORT DOMArrayBuffer : public DOMArrayBufferBase {
 
   // Transfer the ArrayBuffer if it is detachable, otherwise make a copy and
   // transfer that.
-  bool Transfer(v8::Isolate*, ArrayBufferContents& result);
+  virtual bool Transfer(v8::Isolate*, ArrayBufferContents& result);
 
   // Share the ArrayBuffer, even if it is non-shared. Such sharing is necessary
   // for e.g. WebAudio which uses a separate thread for processing the

diff --git a/third_party/blink/renderer/modules/webgpu/gpu_buffer.cc b/third_party/blink/renderer/modules/webgpu/gpu_buffer.cc
@@ -76,6 +76,33 @@ class GPUMappedDOMArrayBuffer : public DOMArrayBuffer {
 
   GPUMappedDOMArrayBuffer(GPUBuffer* owner, ArrayBufferContents contents)
       : DOMArrayBuffer(std::move(contents)), owner_(owner) {}
+  ~GPUMappedDOMArrayBuffer() override = default;
+
+  // Override Transfer such that a copy of the contents is always made. The
+  // backing store will still be detached for this ArrayBuffer, but the
+  // result will be a copy of the contents, not a reference to
+  // the same backing store. This is required by the WebGPU specification so
+  // that the mapped backing store may not be shared cross-thread.
+  bool Transfer(v8::Isolate* isolate, ArrayBufferContents& result) override {
+    // Transfer into |contents| which will detach |this| and all views of
+    // |this|.
+    ArrayBufferContents contents;
+    bool did_detach = DOMArrayBuffer::Transfer(isolate, contents);
+    if (!did_detach) {
+      return false;
+    }
+
+    // Copy the contents into the result.
+    contents.CopyTo(result);
+    return true;
+  }
+
+  bool DetachContents(v8::Isolate* isolate) {
+    // Detach the array buffer by transferring the contents out and dropping
+    // them.
+    ArrayBufferContents contents;
+    return DOMArrayBuffer::Transfer(isolate, contents);
+  }
 
   void Trace(Visitor* visitor) const override {
     DOMArrayBuffer::Trace(visitor);
@@ -365,23 +392,7 @@ void GPUBuffer::DetachMappedArrayBuffers(v8::Isolate* isolate) {
     GPUMappedDOMArrayBuffer* array_buffer = mapped_array_buffer.Release();
     DCHECK(array_buffer->IsDetachable(isolate));
 
-    // Detach the array buffer by transferring the contents out and dropping
-    // them.
-    ArrayBufferContents contents;
-    bool did_detach = array_buffer->Transfer(isolate, contents);
-
-    // |did_detach| would be false if the buffer were already detached.
-    //   Crash if it was, as this indicates that unmapping could alias the
-    //   backing store, or possibly even free it out from under the
-    //   ArrayBuffer. It might be difficult to be 100% certain about this
-    //   invariant, so we CHECK, even in release builds. (Actually, it would be
-    //   fine if the ArrayBuffer was detached without being transferred, but
-    //   this isn't a common case, so it can be revisited if needed.)
-    // TODO(crbug.com/1243842): This CHECK can currently be hit easily by JS
-    //   code. We need to validate against this case by preventing the
-    //   ArrayBuffer from being transferred/detached by outside code.
-    CHECK(did_detach)
-        << "An ArrayBuffer from getMappedRange() was detached before unmap()";
+    array_buffer->DetachContents(isolate);
     DCHECK(array_buffer->IsDetached());
   }
   mapped_array_buffers_.clear();

diff --git a/third_party/blink/web_tests/WebGPUExpectations b/third_party/blink/web_tests/WebGPUExpectations
@@ -163,8 +163,8 @@ crbug.com/dawn/1025 wpt_internal/webgpu/cts.https.html?q=webgpu:api,validation,r
 ### Platform-independent failures
 ###
 
-# Intentionally hits a CHECK.
-crbug.com/1243842 wpt_internal/webgpu/cts.https.html?q=webgpu:api,operation,buffers,map_ArrayBuffer:postMessage:transfer=true;* [ Crash ]
+# Test is being updated in https://github.com/gpuweb/cts/pull/1027
+crbug.com/1243842 wpt_internal/webgpu/cts.https.html?q=webgpu:api,operation,buffers,map_ArrayBuffer:postMessage:transfer=true;* [ Failure ]
 
 # Our automated build does not support mp4 currently (fails on Linux, Mac, and Win Intel)
 wpt_internal/webgpu/cts.https.html?q=webgpu:web_platform,external_texture,video:importExternalTexture,sample:videoSource="red-green.mp4" [ Failure ]