Reland "[wasm] Fix incorrect check for growing shared WebAssembly.memory"

dtig · Commit Bot · commit 44788bfad5be · 2019-10-28T19:07:41.000Z
This reverts commit a4c5136. Reason for revert: CLs to Skip test on Android, predictable builds reviewed, and will land following this CL. Original change's description: > Revert "[wasm] Fix incorrect check for growing shared WebAssembly.memory" > > This reverts commit 2599d3c. > > Reason for revert: Test fails with OOM on Arm64 - N5X (https://ci.chromium.org/p/v8/builders/ci/V8%20Android%20Arm64%20-%20N5X/6514) and is racy on predictable builds (https://ci.chromium.org/p/v8/builders/ci/V8%20Linux%20-%20predictable/27044) > > Original change's description: > > [wasm] Fix incorrect check for growing shared WebAssembly.memory > > > > Bug: chromium:1010272 > > Change-Id: Ieff61089255ee088fad45f15a0f1a8f93eeec94b > > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1869077 > > Commit-Queue: Deepti Gandluri <gdeepti@chromium.org> > > Reviewed-by: Andreas Haas <ahaas@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#64525} > > TBR=mstarzinger@chromium.org,gdeepti@chromium.org,ahaas@chromium.org > > Change-Id: I738a4021a80202c9b822815b922de31f95054fe6 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: chromium:1010272 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1879513 > Reviewed-by: Shu-yu Guo <syg@chromium.org> > Commit-Queue: Shu-yu Guo <syg@chromium.org> > Cr-Commit-Position: refs/heads/master@{#64554} TBR=mstarzinger@chromium.org,gdeepti@chromium.org,ahaas@chromium.org,syg@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: chromium:1010272 Change-Id: Ifbe32854a3d67063e43e2b07a8e649a4850a77d5 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1884411 Reviewed-by: Deepti Gandluri <gdeepti@chromium.org> Reviewed-by: Shu-yu Guo <syg@chromium.org> Commit-Queue: Deepti Gandluri <gdeepti@chromium.org> Cr-Commit-Position: refs/heads/master@{#64598}
diff --git a/src/wasm/wasm-objects.cc b/src/wasm/wasm-objects.cc
@@ -1031,7 +1031,12 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
                                                     new_pages);
         // Broadcasting the update should update this memory object too.
         CHECK_NE(*old_buffer, memory_object->array_buffer());
-        CHECK_EQ(new_byte_length, memory_object->array_buffer().byte_length());
+        // This is a less than check, as it is not guaranteed that the SAB
+        // length here will be equal to the stashed length above as calls to
+        // grow the same memory object can come in from different workers.
+        // It is also possible that a call to Grow was in progress when
+        // handling this call.
+        CHECK_LE(new_byte_length, memory_object->array_buffer().byte_length());
         return static_cast<int32_t>(old_pages);  // success
       }
     }
diff --git a/test/mjsunit/regress/wasm/regress-1010272.js b/test/mjsunit/regress/wasm/regress-1010272.js
@@ -0,0 +1,30 @@
+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --wasm-grow-shared-memory --experimental-wasm-threads
+
+const kNumWorkers = 100;
+const kNumMessages = 50;
+
+function AllocMemory(initial, maximum = initial) {
+  return new WebAssembly.Memory({initial : initial, maximum : maximum, shared : true});
+}
+
+(function RunTest() {
+  let worker = [];
+  for (let w = 0; w < kNumWorkers; w++) {
+    worker[w] = new Worker(
+        `onmessage =
+        function(msg) {
+          msg.memory.grow(1);
+        }`, {type : 'string'});
+  }
+
+  for (let i = 0; i < kNumMessages; i++) {
+    let memory = AllocMemory(1, 128);
+    for (let w = 0; w < kNumWorkers; w++) {
+      worker[w].postMessage({memory : memory});
+    }
+  }
+})();

Original file line number	Diff line number	Diff line change
`@@ -1031,7 +1031,12 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,`
`1031`	`1031`	`new_pages);`
`1032`	`1032`	`// Broadcasting the update should update this memory object too.`
`1033`	`1033`	`CHECK_NE(*old_buffer, memory_object->array_buffer());`
`1034`		`- CHECK_EQ(new_byte_length, memory_object->array_buffer().byte_length());`
	`1034`	`+ // This is a less than check, as it is not guaranteed that the SAB`
	`1035`	`+ // length here will be equal to the stashed length above as calls to`
	`1036`	`+ // grow the same memory object can come in from different workers.`
	`1037`	`+ // It is also possible that a call to Grow was in progress when`
	`1038`	`+ // handling this call.`
	`1039`	`+ CHECK_LE(new_byte_length, memory_object->array_buffer().byte_length());`
`1035`	`1040`	`return static_cast<int32_t>(old_pages); // success`
`1036`	`1041`	`}`
`1037`	`1042`	`}`