[wasm] Fix thread_in_wasm_flag in exception handling

The flag should not be set after an exception is thrown in a runtime
function. The unwinder still runs after the destructor, and should take
care of setting the flag depending on the catching frame.

R=ahaas@chromium.org,jkummerow@chromium.org

Bug: chromium:1180690
Change-Id: I0013c90f759a5145309f6e08d61ed36aeecbac63
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2713103Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72972}

src/execution/isolate.cc

@@ -1563,6 +1563,8 @@ Handle<JSMessageObject> Isolate::CreateMessageOrAbort(
 
 Object Isolate::ThrowInternal(Object raw_exception, MessageLocation* location) {
   DCHECK(!has_pending_exception());
+  DCHECK_IMPLIES(trap_handler::IsTrapHandlerEnabled(),
+                 !trap_handler::IsThreadInWasm());
 
   HandleScope scope(this);
   Handle<Object> exception(raw_exception, this);
@@ -1658,6 +1660,8 @@ Object Isolate::ReThrow(Object exception) {
 
 Object Isolate::UnwindAndFindHandler() {
   Object exception = pending_exception();
+  DCHECK_IMPLIES(trap_handler::IsTrapHandlerEnabled(),
+                 !trap_handler::IsThreadInWasm());
 
   auto FoundHandler = [&](Context context, Address instruction_start,
                           intptr_t handler_offset,
@@ -1731,11 +1735,6 @@ Object Isolate::UnwindAndFindHandler() {
       }
 
       case StackFrame::WASM: {
-        if (trap_handler::IsThreadInWasm()) {
-          // TODO(thibaudm): The flag should be cleared already.
-          trap_handler::ClearThreadInWasm();
-        }
-
         if (!catchable_by_wasm) break;
 
         // For WebAssembly frames we perform a lookup in the handler table.
@@ -1755,8 +1754,7 @@ Object Isolate::UnwindAndFindHandler() {
                             StandardFrameConstants::kFixedFrameSizeAboveFp -
                             wasm_code->stack_slots() * kSystemPointerSize;
 
-        // This is going to be handled by Wasm, so we need to set the TLS flag
-        // again. It was cleared above assuming the frame would be unwound.
+        // This is going to be handled by Wasm, so we need to set the TLS flag.
         trap_handler::SetThreadInWasm();
 
         return FoundHandler(Context(), wasm_code->instruction_start(), offset,

src/runtime/runtime-wasm.cc

@@ -65,7 +65,7 @@ Context GetNativeContextFromWasmInstanceOnStackTop(Isolate* isolate) {
 
 class V8_NODISCARD ClearThreadInWasmScope {
  public:
-  ClearThreadInWasmScope() {
+  explicit ClearThreadInWasmScope(Isolate* isolate) : isolate_(isolate) {
     DCHECK_IMPLIES(trap_handler::IsTrapHandlerEnabled(),
                    trap_handler::IsThreadInWasm());
     trap_handler::ClearThreadInWasm();
@@ -73,8 +73,15 @@ class V8_NODISCARD ClearThreadInWasmScope {
   ~ClearThreadInWasmScope() {
     DCHECK_IMPLIES(trap_handler::IsTrapHandlerEnabled(),
                    !trap_handler::IsThreadInWasm());
+    if (!isolate_->has_pending_exception()) {
       trap_handler::SetThreadInWasm();
     }
+    // Otherwise we only want to set the flag if the exception is caught in
+    // wasm. This is handled by the unwinder.
+  }
+
+ private:
+  Isolate* isolate_;
 };
 
 Object ThrowWasmError(Isolate* isolate, MessageTemplate message) {
@@ -108,7 +115,7 @@ RUNTIME_FUNCTION(Runtime_WasmIsValidRefValue) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmMemoryGrow) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -124,24 +131,25 @@ RUNTIME_FUNCTION(Runtime_WasmMemoryGrow) {
 }
 
 RUNTIME_FUNCTION(Runtime_ThrowWasmError) {
-  ClearThreadInWasmScope clear_wasm_flag;
+  ClearThreadInWasmScope flag_scope(isolate);
   DCHECK_EQ(1, args.length());
   CONVERT_SMI_ARG_CHECKED(message_id, 0);
   return ThrowWasmError(isolate, MessageTemplateFromInt(message_id));
 }
 
 RUNTIME_FUNCTION(Runtime_ThrowWasmStackOverflow) {
-  ClearThreadInWasmScope clear_wasm_flag;
+  ClearThreadInWasmScope clear_wasm_flag(isolate);
   SealHandleScope shs(isolate);
   DCHECK_LE(0, args.length());
   return isolate->StackOverflow();
 }
 
 RUNTIME_FUNCTION(Runtime_WasmThrowJSTypeError) {
-  // This runtime function is called both from wasm and from e.g. js-to-js
-  // functions. Hence the "thread in wasm" flag can be either set or not. Both
-  // is OK, since throwing will trigger unwinding anyway, which sets the flag
-  // correctly depending on the handler.
+  // The caller may be wasm or JS. Only clear the thread_in_wasm flag if the
+  // caller is wasm, and let the unwinder set it back depending on the handler.
+  if (trap_handler::IsTrapHandlerEnabled() && trap_handler::IsThreadInWasm()) {
+    trap_handler::ClearThreadInWasm();
+  }
   HandleScope scope(isolate);
   DCHECK_EQ(0, args.length());
   THROW_NEW_ERROR_RETURN_FAILURE(
@@ -149,9 +157,7 @@ RUNTIME_FUNCTION(Runtime_WasmThrowJSTypeError) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmThrow) {
-  // Do not clear the flag in a scope. The unwinder will set it if the exception
-  // is caught by a wasm frame, otherwise we keep it cleared.
-  trap_handler::ClearThreadInWasm();
+  ClearThreadInWasmScope clear_wasm_flag(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   isolate->set_context(GetNativeContextFromWasmInstanceOnStackTop(isolate));
@@ -178,9 +184,7 @@ RUNTIME_FUNCTION(Runtime_WasmThrow) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmReThrow) {
-  // Do not clear the flag in a scope. The unwinder will set it if the exception
-  // is caught by a wasm frame, otherwise we keep it cleared.
-  trap_handler::ClearThreadInWasm();
+  ClearThreadInWasmScope clear_wasm_flag(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
   isolate->wasm_engine()->SampleRethrowEvent(isolate);
@@ -188,7 +192,7 @@ RUNTIME_FUNCTION(Runtime_WasmReThrow) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmStackGuard) {
-  ClearThreadInWasmScope wasm_flag;
+  ClearThreadInWasmScope wasm_flag(isolate);
   SealHandleScope shs(isolate);
   DCHECK_EQ(0, args.length());
 
@@ -200,7 +204,7 @@ RUNTIME_FUNCTION(Runtime_WasmStackGuard) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmCompileLazy) {
-  ClearThreadInWasmScope wasm_flag;
+  ClearThreadInWasmScope wasm_flag(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -306,7 +310,7 @@ RUNTIME_FUNCTION(Runtime_WasmTriggerTierUp) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmAtomicNotify) {
-  ClearThreadInWasmScope clear_wasm_flag;
+  ClearThreadInWasmScope clear_wasm_flag(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(3, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -322,7 +326,7 @@ RUNTIME_FUNCTION(Runtime_WasmAtomicNotify) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmI32AtomicWait) {
-  ClearThreadInWasmScope clear_wasm_flag;
+  ClearThreadInWasmScope clear_wasm_flag(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(4, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -345,7 +349,7 @@ RUNTIME_FUNCTION(Runtime_WasmI32AtomicWait) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmI64AtomicWait) {
-  ClearThreadInWasmScope clear_wasm_flag;
+  ClearThreadInWasmScope clear_wasm_flag(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(4, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -383,7 +387,7 @@ Object ThrowTableOutOfBounds(Isolate* isolate,
 }  // namespace
 
 RUNTIME_FUNCTION(Runtime_WasmRefFunc) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -397,7 +401,7 @@ RUNTIME_FUNCTION(Runtime_WasmRefFunc) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmFunctionTableGet) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(3, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -421,7 +425,7 @@ RUNTIME_FUNCTION(Runtime_WasmFunctionTableGet) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmFunctionTableSet) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(4, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -448,7 +452,7 @@ RUNTIME_FUNCTION(Runtime_WasmFunctionTableSet) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmTableInit) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(6, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -470,7 +474,7 @@ RUNTIME_FUNCTION(Runtime_WasmTableInit) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmTableCopy) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(6, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
@@ -492,7 +496,7 @@ RUNTIME_FUNCTION(Runtime_WasmTableCopy) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmTableGrow) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(3, args.length());
   auto instance =
@@ -511,7 +515,7 @@ RUNTIME_FUNCTION(Runtime_WasmTableGrow) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmTableFill) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(4, args.length());
   auto instance =
@@ -544,7 +548,7 @@ RUNTIME_FUNCTION(Runtime_WasmTableFill) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(0, args.length());
   FrameFinder<WasmFrame, StackFrame::EXIT, StackFrame::WASM_DEBUG_BREAK>
@@ -618,7 +622,7 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
 }
 
 RUNTIME_FUNCTION(Runtime_WasmAllocateRtt) {
-  ClearThreadInWasmScope flag_scope;
+  ClearThreadInWasmScope flag_scope(isolate);
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   CONVERT_UINT32_ARG_CHECKED(type_index, 0);

test/mjsunit/regress/wasm/regress-1180690.js

+// Copyright 2021 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-test-streaming --wasm-lazy-compilation --wasm-lazy-validation
+
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+
+(function f1() {
+  const builder = new WasmModuleBuilder();
+  builder.addMemory(1, 1);
+  builder.addFunction('main', kSig_i_i).addBody([
+      kExprLocalGet, 0,
+      kExprI32LoadMem, 0, 0
+  ]).exportFunc();
+  const instance = builder.instantiate();
+  instance.exports.main();
+})();
+
+(function f2() {
+  const builder = new WasmModuleBuilder();
+  builder.addFunction('id', kSig_i_i).addBody([]).exportFunc();
+  const buffer = builder.toBuffer();
+  const instance = builder.instantiate();
+  try {
+    instance.exports.id();
+  } catch {}
+})();