[wasm] Refine installation of the WebAssembly.Tag constructor

jakobkummerow · V8 LUCI CQ · commit f473f10ef339 · 2022-04-13T12:20:49.000Z
This makes the installation sequence of WebAssembly.Tag slightly shorter, slightly faster, slightly cleaner in corner-case semantics, and slightly better documented. To allow testing this code, Isolate::InstallConditionalFeatures is exposed as d8.test.installConditionalFeatures(). Fixed: chromium:1314616 Change-Id: I44285e398b8797e0e7d2d8c782cecec3ba68a503 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3582382 Commit-Queue: Jakob Kummerow <jkummerow@chromium.org> Reviewed-by: Clemens Backes <clemensb@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/main@{#79956}
diff --git a/src/d8/d8.cc b/src/d8/d8.cc
@@ -2183,6 +2183,12 @@ void Shell::TestVerifySourcePositions(
   }
 }
 
+void Shell::InstallConditionalFeatures(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+  isolate->InstallConditionalFeatures(isolate->GetCurrentContext());
+}
+
 // async_hooks.createHook() registers functions to be called for different
 // lifetime events of each async operation.
 void Shell::AsyncHooksCreateHook(
@@ -3140,6 +3146,11 @@ Local<ObjectTemplate> Shell::CreateD8Template(Isolate* isolate) {
       test_template->Set(isolate, "LeafInterfaceType",
                          Shell::CreateLeafInterfaceTypeTemplate(isolate));
     }
+    // Allows testing code paths that are triggered when Origin Trials are
+    // added in the browser.
+    test_template->Set(
+        isolate, "installConditionalFeatures",
+        FunctionTemplate::New(isolate, Shell::InstallConditionalFeatures));
 
     d8_template->Set(isolate, "test", test_template);
   }
diff --git a/src/d8/d8.h b/src/d8/d8.h
@@ -557,6 +557,9 @@ class Shell : public i::AllStatic {
   static void TestVerifySourcePositions(
       const v8::FunctionCallbackInfo<v8::Value>& args);
 
+  static void InstallConditionalFeatures(
+      const v8::FunctionCallbackInfo<v8::Value>& args);
+
   static void AsyncHooksCreateHook(
       const v8::FunctionCallbackInfo<v8::Value>& args);
   static void AsyncHooksExecutionAsyncId(
diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc
@@ -2992,23 +2992,25 @@ void WasmJs::InstallConditionalFeatures(Isolate* isolate,
     MaybeHandle<Object> maybe_webassembly =
         JSObject::GetProperty(isolate, global, "WebAssembly");
     Handle<Object> webassembly_obj;
-    if (!maybe_webassembly.ToHandle(&webassembly_obj)) {
-      // There is not {WebAssembly} object. We just return without adding the
-      // {Tag} constructor.
-      return;
-    }
-    if (!webassembly_obj->IsJSObject()) {
-      // The {WebAssembly} object is invalid. As we cannot add the {Tag}
-      // constructor, we just return.
+    if (!maybe_webassembly.ToHandle(&webassembly_obj) ||
+        !webassembly_obj->IsJSObject()) {
+      // There is no {WebAssembly} object, or it's not what we expect.
+      // Just return without adding the {Tag} constructor.
       return;
     }
     Handle<JSObject> webassembly = Handle<JSObject>::cast(webassembly_obj);
-    // Setup Exception
+    // Setup Tag.
     Handle<String> tag_name = v8_str(isolate, "Tag");
+    // The {WebAssembly} object may already have been modified. The following
+    // code is designed to:
+    //  - check for existing {Tag} properties on the object itself, and avoid
+    //    overwriting them or adding duplicate properties
+    //  - disregard any setters or read-only properties on the prototype chain
+    //  - only make objects accessible to user code after all internal setup
+    //    has been completed.
     if (JSObject::HasOwnProperty(isolate, webassembly, tag_name)
             .FromMaybe(true)) {
-      // The {Exception} constructor already exists, there is nothing more to
-      // do.
+      // Existing property, or exception.
       return;
     }
 
@@ -3017,21 +3019,19 @@ void WasmJs::InstallConditionalFeatures(Isolate* isolate,
         CreateFunc(isolate, tag_name, WebAssemblyTag, has_prototype,
                    SideEffectType::kHasNoSideEffect);
     tag_constructor->shared().set_length(1);
-    auto result =
-        Object::SetProperty(isolate, webassembly, tag_name, tag_constructor,
-                            StoreOrigin::kNamed, Just(ShouldThrow::kDontThrow));
-    if (result.is_null()) {
-      // Setting the {Tag} constructor failed. We just bail out.
-      return;
-    }
-    // Install the constructor on the context.
     context->set_wasm_tag_constructor(*tag_constructor);
     Handle<JSObject> tag_proto =
         SetupConstructor(isolate, tag_constructor, i::WASM_TAG_OBJECT_TYPE,
                          WasmTagObject::kHeaderSize, "WebAssembly.Tag");
     if (enabled_features.has_type_reflection()) {
       InstallFunc(isolate, tag_proto, "type", WebAssemblyTagType, 0);
     }
+    LookupIterator it(isolate, webassembly, tag_name, LookupIterator::OWN);
+    Maybe<bool> result = JSObject::DefineOwnPropertyIgnoreAttributes(
+        &it, tag_constructor, DONT_ENUM, Just(kDontThrow));
+    // This could still fail if the object was non-extensible, but now we
+    // return anyway so there's no need to even check.
+    USE(result);
   }
 }
 #undef ASSIGN

Original file line number	Diff line number	Diff line change
`@@ -2183,6 +2183,12 @@ void Shell::TestVerifySourcePositions(`
`2183`	`2183`	`}`
`2184`	`2184`	`}`
`2185`	`2185`
	`2186`	`+void Shell::InstallConditionalFeatures(`
	`2187`	`+ const v8::FunctionCallbackInfo<v8::Value>& args) {`
	`2188`	`+ Isolate* isolate = args.GetIsolate();`
	`2189`	`+ isolate->InstallConditionalFeatures(isolate->GetCurrentContext());`
	`2190`	`+}`
	`2191`	`+`
`2186`	`2192`	`// async_hooks.createHook() registers functions to be called for different`
`2187`	`2193`	`// lifetime events of each async operation.`
`2188`	`2194`	`void Shell::AsyncHooksCreateHook(`
`@@ -3140,6 +3146,11 @@ Local<ObjectTemplate> Shell::CreateD8Template(Isolate* isolate) {`
`3140`	`3146`	`test_template->Set(isolate, "LeafInterfaceType",`
`3141`	`3147`	`Shell::CreateLeafInterfaceTypeTemplate(isolate));`
`3142`	`3148`	`}`
	`3149`	`+ // Allows testing code paths that are triggered when Origin Trials are`
	`3150`	`+ // added in the browser.`
	`3151`	`+ test_template->Set(`
	`3152`	`+ isolate, "installConditionalFeatures",`
	`3153`	`+ FunctionTemplate::New(isolate, Shell::InstallConditionalFeatures));`
`3143`	`3154`
`3144`	`3155`	`d8_template->Set(isolate, "test", test_template);`
`3145`	`3156`	`}`